-
Notifications
You must be signed in to change notification settings - Fork 3k
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 2024-11-13] [$250] Workspace - In android, WS report displays double spacing before&after user name/id #51409
Comments
Triggered auto assignment to @sakluger ( |
@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
This feels like a pretty small bug, I'm going to set the price at $125. Feel free to let me know if you disgree. |
Job added to Upwork: https://www.upwork.com/jobs/~021849941675546725686 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ishpaul777 ( |
Edited by proposal-police: This proposal was edited at 2024-10-24 14:00:30 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Workspace - In android, WS report displays double spacing before&after user name/id What is the root cause of that problem?There are spaces at the beginning or end of translations here and we add additional spaces during usage here What changes do you think we should make in order to solve the problem?Remove spaces from this translations
What alternative solutions did you explore? (Optional)Do not add spaces on usage here
|
ProposalPlease re-state the problem that we are trying to solve in this issue.WS report displays double spacing before&after user name/id What is the root cause of that problem?The welcome message text contains an extra space because For
Lines 674 to 675 in 652d2ff
Lines 549 to 552 in 652d2ff
2 The welcome message text L596 if welcome message phrase1 that is beginningOfChatHistory Line 673 in 652d2ff
Line 571 in 652d2ff
Line 596 in 652d2ff
For Line 661 in 652d2ff
Line 669 in 652d2ff
Line 614 in 652d2ff
Line 641 in 652d2ff
Line 644 in 652d2ff
What changes do you think we should make in order to solve the problem?To resolve this issue, we should create a function to remove extra spaces because there are many cases where the translation text includes spaces, while some translations do not. We shouldn't remove spaces from src/libs/SidebarUtils.ts#534
+ function ensureSingleSpacing(text: string) {
+ return text.replace(/\s+/g, ' ').trim();
+ }
src/libs/SidebarUtils.ts#552
- welcomeMessage.messageText = `${welcomeMessage.phrase1} ${ReportUtils.getDisplayNameForParticipant(report?.ownerAccountID)} ${welcomeMessage.phrase2} ${ReportUtils.getPolicyName(
- report,
- )} ${welcomeMessage.phrase3}`;
+ welcomeMessage.messageText = ensureSingleSpacing(
+ `${welcomeMessage.phrase1} ${ReportUtils.getDisplayNameForParticipant(report?.ownerAccountID)} ${welcomeMessage.phrase2} ${ReportUtils.getPolicyName(report)} ${
+ welcomeMessage.phrase3
+ }`,
+ );
src/libs/SidebarUtils.ts#602
- welcomeMessage.messageText = displayNamesWithTooltips.length ? `${welcomeMessage.phrase1} ${displayNamesWithTooltipsText}` : '';
+ welcomeMessage.messageText = displayNamesWithTooltips.length ? ensureSingleSpacing(`${welcomeMessage.phrase1} ${displayNamesWithTooltipsText}`) : '';
src/libs/SidebarUtils.ts#644
- welcomeMessage.messageText = `${welcomeMessage.phrase1} ${welcomeMessage.showReportName ? ReportUtils.getReportName(report) : ''} ${welcomeMessage.phrase2 ?? ''}`;
+ welcomeMessage.messageText = ensureSingleSpacing(`${welcomeMessage.phrase1} ${welcomeMessage.showReportName ? ReportUtils.getReportName(report) : ''} ${welcomeMessage.phrase2 ?? ''}`); |
@sakluger, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
I'll review in few hours |
Proposal from @huult is complete and fixes all such double spacing issues hollistically. lets assign them! 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @huult 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Thank you all. I will create a PR within 24 hours |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.57-10 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 2024-11-13. 🎊 For reference, here are some details about the assignees on this issue:
|
@ishpaul777 @sakluger The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button] |
Hey @ishpaul777 please make sure to finish the BZ checklist before the payment date tomorrow. |
Summarizing payment on this issue: Contributor: @huult $250, paid via Upwork |
i'll fill out first thing tmrw 🙇 |
@ishpaul777 any updates? |
sorry was OOO past few days BugZero Checklist:
Bug classificationSource of bug:
Where bug was reported:
Who reported the bug:
For same reason as above no regression test needed also the fix implemented will make sure this doesn't happen anymore with other welcome message copies |
Thanks! I paid out the remaining Upwork contract, we're good to close. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9. 0.53-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
In android and mweb, the LHN text should displayed in same manner
Actual Result:
In android, there is double space before& after email id and user name on Workspace chat text in LHN but in mweb, single space only displayed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @ishpaul777The text was updated successfully, but these errors were encountered: