-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 543 error displaying username with underscore #586
Issue 543 error displaying username with underscore #586
Conversation
@@ -0,0 +1,108 @@ | |||
const { expect } = require('chai'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danfercf1 hey Daniel, as I told you in another PR, new files should go in TypeScript, not JavaScript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's a test file, and it will not be executed by the script of package.json if it's a typescript file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test files can be written in typescript too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but they are not supported by the project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree, but itsn't part of this task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, given no .ts tests exists in master branch, you cannot technically create a PR that separates that task completely. I mean, that task is needed only now that you're creating a new test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that is necessary to prepare the configuration and dependencies to achieve that, it's not only an addition of a .ts file, all the stuff necessary could be a new PR and task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey guys, I also agree new files should be .ts, but if we need to some some extra stuff to make it work I consider we should merge this as javascript and then work on migrating it to ts in another issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the admin takes the dispute I got this error and the message with order details is not sent:
[2024-09-05T14:49:35.397-03:00] error: 400: Bad Request: can't parse entities: Can't find end of Italic entity at byte offset 1203 Error: 400: Bad Request: can't parse entities: Can't find end of Italic entity at byte offset 1203
at Telegram.callApi (/home/grunch/dev/bot-danfer/node_modules/telegraf/lib/core/network/client.js:315:19)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async exports.disputeData (/home/grunch/dev/bot-danfer/bot/modules/dispute/messages.js:102:5)
at async exports.takeDispute (/home/grunch/dev/bot-danfer/bot/modules/dispute/actions.js:39:3)
at async execute (/home/grunch/dev/bot-danfer/node_modules/telegraf/lib/composer.js:518:17)
at async /home/grunch/dev/bot-danfer/node_modules/telegraf/lib/composer.js:519:21
@grunch, I will take a look, could you provide me the usernames that you are testing? |
users: @negrunch and @xxx_yyy |
I will take a look, thanks |
…he translation file
The issue was fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already works fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
thank you for your contribution 🚀
After you push this commit it will be merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing job! thank you for your contribution
"sinon": "^11.1.2", | ||
"telegram-test-api": "^2.5.0", | ||
"typescript": "^5.1.6" | ||
"typegram": "^5.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danfercf1 why was this new dependency added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That dependency is imported in start.ts file, and it wasn't added in the package.json file, the bot works because the package-lock.json file has the dependency but it's not a good practice to not have synchronized both files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, next time split that change in a different PR please
Added a method to escape underscores when an user of Telegram has underscore in its username, this is necessary due the configuration for the message when it's set to "{ parse_mode: 'MarkdownV2' }". Additionally I added the necessary tests to verify the fix. I added the telegram id to the translation message for Spanish and English, I can update the other languages after I receive the ok for the current message for these first ones that I updated.
Fixes #543