-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2023-06-13] [$1000] LHN - App displays 'This is beginning...' message for few seconds when we send strikethrough text in code block #19539
Comments
Triggered auto assignment to @laurenreidexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue.When you send What is the root cause of that problem?The reason for this is the
The What changes do you think we should make in order to solve the problem?I'd like to know the logic used to format the One way to solve this (I'm assuming this is what the server does too) is to return Str.htmlDecode(String(lastMessageText)).trim().replace(CONST.REGEX.AFTER_FIRST_LINE_BREAK, '').substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH).trim(); What alternative solutions did you explore? (Optional)Another way to solve this problem is to replace all |
Job added to Upwork: https://www.upwork.com/jobs/~011ba23426f30f15c7 |
Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
Triggered auto assignment to @stitesExpensify ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.When you send message like What is the root cause of that problem?The above string is converted to html App/src/libs/actions/Report.js Line 213 in c53c89b
The value of Line 1217 in c53c89b
As you can see from htmlToText rules in ExpensiMark, we add new line before and after the text for That's why the value of The Line 644 in c53c89b
So the last message is empty string. When last message is empty string, we set the Lines 297 to 315 in c53c89b
But the server returns the correct string( This is the root cause of this issue. What changes do you think we should make in order to solve the problem?We can trim all white spaces in
This works perfectly. Resultmac_chrome_19539.mp4What alternative solutions did you explore? (Optional)As described above, we add in the rules here as follows |
@sobitneupane Bump ^^ |
Thanks for the proposals. Proposal from @therealsujitk looks good to me. Let's make sure to include tests for various types of messages (like highlighted ( 🎀👀🎀 C+ reviewed |
bump @stitesExpensify for hire - are we good to move forward? thanks |
Yep, let's move forward with @therealsujitk proposal. If there is flickering when you are testing, let me know and I will dive into the backend code! |
📣 @therealsujitk You have been assigned to this job by @stitesExpensify! |
note to self - offers sent to @dhanashree-sawant @therealsujitk @sobitneupane in upwork |
@stitesExpensify after some testing I ran into an issue. Take a look at these two test cases,
The second test case works when Screencast.from.01-06-23.04.50.53.PM.IST.webmIt looks like I'll need the logic that is used in the back-end. - Slack Discussion |
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Triggered auto assignment to @anmurali ( |
@anmurali pls pay this one next week when it's due - I am OOO so won't be able to complete |
@stitesExpensify can a bonus be considered for this issue even though the PR was merged late since it was reviewed and approved with no further changes quite early on within the 3 day period? |
Yes, if there were no changes and the reviews were the only delay, then the bonus should apply |
@sobitneupane can you please complete teh steps above re: regression, thank you All payments have been issued in Upwork, including bonuses, |
@sobitneupane bump ^^ thanks |
Regression Test Proposal
|
It is such a small bug. It is very difficult to find the exact PR which resulted in this issue. It might have been caused by change in expensify-common or some other small changes in app. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
~```hello```~
Expected Result:
App should directly display the text in LHN as it normally does for all other types of messages
Actual Result:
App displays 'This is beginning...' text for few seconds in LHN on sending strikethrough text in 3 backticks codeblock
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.17.1
Reproducible in staging?: yes
Reproducible in production?: yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
LHN.wrong.text.on.strikethrough.codeblock.mp4
Recording.2801.mp4
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684423358039509
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: