-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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-03-13] [$2000] Sending/Editing message counts the typed characters in the UI & not the html characters which causes the api request to fail. #14268
Comments
Job added to Upwork: https://www.upwork.com/jobs/~010b95e20b95c70ed7 |
Current assignee @flaviadefaria is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
Triggered auto assignment to @puneetlath ( |
Added the external label. Now waiting for proposals. |
This is duplicated with #13988 error.mov |
ProposalProblemAs the description says we don't count the actual sending characters, instead we just count the typed characters to determine a valid length. Solution 1Count the html parsed characters and limit it to 60K as that is the limit that has been set in the backend/DB. Solution 2Send the non-parsed message if the parsed html message exceeds the 60K character limit as that is the limit that has been set in the backend/DB. Also, currently we don't parse text to MD/HTML when the text is above 10K characters so this can be a good enough solution. diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js
index 45b9319a4..706846268 100644
--- a/src/libs/ReportUtils.js
+++ b/src/libs/ReportUtils.js
@@ -651,16 +651,17 @@ function buildOptimisticReportAction(sequenceNumber, text, file) {
// For longer comments, skip parsing and display plaintext for performance reasons. It takes over 40s to parse a 100k long string!!
const parser = new ExpensiMark();
const commentText = text.length < 10000 ? parser.replace(text) : text;
+ const finalCommentText = commentText < 60000 ? commentText : text;
const isAttachment = _.isEmpty(text) && file !== undefined;
const attachmentInfo = isAttachment ? file : {};
- const htmlForNewComment = isAttachment ? 'Uploading Attachment...' : commentText;
+ const htmlForNewComment = isAttachment ? 'Uploading Attachment...' : finalCommentText;
// Remove HTML from text when applying optimistic offline comment
const textForNewComment = isAttachment ? '[Attachment]'
: parser.htmlToText(htmlForNewComment);
return {
- commentText,
+ commentText: finalCommentText,
reportAction: {
reportActionID: NumberUtils.rand64(),
actionName: CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT, |
a duplicate, please close. Edit: This seems slightly a different issue than the one linked, Maybe it should be on hold till we figure out a solution for the first issue |
Yes, I agree let's hold on #13988. Whether or not we can close this I think will depend on the solution we land on in that issue. @flaviadefaria I'm going to remove the External/Help Wanted labels and the assignment for me/Manan. Feel free to re-add the External label to get people assigned once #13988 is resolved. Thanks! |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.78-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-03-13. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
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:
|
Moving this to daily so I can remember to pay it 😅 |
Regression Test Proposal
Do we agree 👍 or 👎 Also @aimane-chnaif, for your steps, I do not believe there will be a PR on the front-end that is the source of this bug. It was an oversight when creating the comment counter, and the source PR would ultimately be on the back-end when the auth limit was instated. |
@redstar504, @francoisl, @flaviadefaria, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick! |
I updated the BZ checklist above, @flaviadefaria we'll just need to issue the payments and create a GH issue to add these regression steps. Thanks! |
@flaviadefaria Can we get payments sorted? The bonus should apply and we still need to be invited to Upwork. Thanks. |
Hope she will be back soon. Last week everyone was busy at ECX 🚀 |
@flaviadefaria Thanks for the quick response. Accepted your offer. |
Sorry for the delay here! I've sent all offers and will issue payment now - $2000 + $1000 bonus = $3000 for @aimane-chnaif and @redstar504 And $250 to @priyeshshah11 for reporting the bug. |
Thanks @flaviadefaria, received the offer but isn't it $1000 for reporting these days? |
Hm I didn't think so, but let me double-check and I'll get back to you. |
@priyeshshah11 I just got confirmation that reporting bugs is still at $250. As soon as you accept the offer I'll issue you the payment. Thanks! |
My bad, Thanks for checking @flaviadefaria 😄 |
No problem! I just issued your payment :) |
ps @priyeshshah11 if you are unsure in the future, we always update the contributing.md file to state the current payout rates for jobs and bug reporting: |
Coming from these updated instructions @aimane-chnaif where in TestRail would you propose these testing steps be added? This is what I need:
I had a quick look in TR and I think we should add it within Copy Function test, but not really sure if in an existing folder or a new one. |
@flaviadefaria Oh, there was a discussion where C+ doesn't need to propose testing steps in TestRail. Can you please confirm? |
Oh interesting, ok let me double-check. |
@aimane-chnaif thanks for linking that discussion. You're correct, our resources were not up to date, but we're updating them now. We're all done here! |
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:
Expected Result:
We should only allow sending the number of characters that BE can accept. i.e while sending we should calculate the length of the final html converted text instead of the typed characters
Actual Result:
The message fails to send/update with "402 Maximum Size Exceeded message" error
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.53-0
Reproducible in staging?: Y
Reproducible in production?: Y
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:
Screen.Recording.2023-01-13.at.3.32.14.AM.mov
Recording.1280.mp4
Expensify/Expensify Issue URL:
Issue reported by: @priyeshshah11
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673541273533429
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: