Skip to content
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-10-09] [HOLD for payment 2023-09-29] [$500] Web - Inconsistency bug: App does not translate copy to clipboard text for requested (works fine for owes) #27114

Closed
1 of 6 tasks
kbecciv opened this issue Sep 10, 2023 · 61 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Sep 10, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open the app
  2. Click on plus and complete request money flow using any amount and any user
  3. Change language to spanish
  4. Open report to which we have requested money in step 2
  5. Copy IOU message of request money and paste anywhere to observe that it is translated correctly
  6. Click on IOU message and click on copy to clipboard on similar IOU message
  7. Paste and observe that it is not translated

Expected Result:

App should translate IOU message copy to clipboard text for 'requested' message as we do for similar IOU parent message

Actual Result:

App does not translate IOU message copy to clipboard text for 'requested' message IOU which we find on click of request money IOU in main report

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.66.3
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: Any additional supporting documentation

translation.copy.to.clipboard.for.inner.IOU.mp4
Recording.4371.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693925728595269

View all open jobs on GitHub

@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 10, 2023
@melvin-bot melvin-bot bot changed the title Web - Inconsistency bug: App does not translate copy to clipboard text for requested (works fine for owes) [$500] Web - Inconsistency bug: App does not translate copy to clipboard text for requested (works fine for owes) Sep 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 10, 2023

Job added to Upwork: https://www.upwork.com/jobs/~014fa137d31ebe2c21

@melvin-bot
Copy link

melvin-bot bot commented Sep 10, 2023

Triggered auto assignment to @maddylewis (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 10, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot
Copy link

melvin-bot bot commented Sep 10, 2023

Triggered auto assignment to @conorpendergrast (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Sep 10, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @narefyev91 (External)

@getusha
Copy link
Contributor

getusha commented Sep 10, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

copy clipboard text for requested is not translated according to preferences.

What is the root cause of that problem?

App/src/libs/ReportUtils.js

Lines 2023 to 2040 in fd043aa

switch (type) {
case CONST.REPORT.ACTIONS.TYPE.APPROVED:
iouMessage = `approved ${amount}`;
break;
case CONST.IOU.REPORT_ACTION_TYPE.CREATE:
iouMessage = `requested ${amount}${comment && ` for ${comment}`}`;
break;
case CONST.IOU.REPORT_ACTION_TYPE.SPLIT:
iouMessage = `split ${amount}${comment && ` for ${comment}`}`;
break;
case CONST.IOU.REPORT_ACTION_TYPE.DELETE:
iouMessage = `deleted the ${amount} request${comment && ` for ${comment}`}`;
break;
case CONST.IOU.REPORT_ACTION_TYPE.PAY:
iouMessage = isSettlingUp ? `paid ${amount}${paymentMethodMessage}` : `sent ${amount}${comment && ` for ${comment}`}${paymentMethodMessage}`;
break;
default:
break;

We have hard coded text which will not be translated dynamically depending on the user language selection.

What changes do you think we should make in order to solve the problem?

Such texts should be put inside en.js and es.js and should be used using Localize.translateLocal, for example we should move the following to en.js & es.js and make it a function to accept required arguments to use like payerOwesAmount

iouMessage = `requested ${amount}${comment && ` for ${comment}`}`;

Will be something like

requestedAmount: ({amount, comment}) => `requested ${amount}${comment && ` for ${comment}`}`

and will be used using the following

Localize.translateLocal('iou.requestedAmount', {payer: comment, amount: formattedAmount})

payerOwesAmount: ({payer, amount}: PayerOwesAmountParams) => `${payer} owes ${amount}`,

What alternative solutions did you explore? (Optional)

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Sep 10, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

The problem we are addressing is that the app does not translate copy-to-clipboard text for requested money actions, although it works fine for IOUs.

What is the root cause of that problem?

The root cause of this issue lies in how we are retrieving and handling the copy-to-clipboard message in the context menu action located in the following here:

{
isAnonymousAction: true,
textTranslateKey: 'reportActionContextMenu.copyToClipboard',
icon: Expensicons.Copy,
successTextTranslateKey: 'reportActionContextMenu.copied',
successIcon: Expensicons.Checkmark,
shouldShow: (type, reportAction) =>
type === CONTEXT_MENU_TYPES.REPORT_ACTION &&
!ReportUtils.isReportMessageAttachment(_.last(lodashGet(reportAction, ['message'], [{}]))) &&
!ReportActionsUtils.isMessageDeleted(reportAction),
// If return value is true, we switch the `text` and `icon` on
// `ContextMenuItem` with `successText` and `successIcon` which will fallback to
// the `text` and `icon`
onPress: (closePopover, {reportAction, selection}) => {
const isTaskAction = ReportActionsUtils.isTaskAction(reportAction);
const isReportPreviewAction = ReportActionsUtils.isReportPreviewAction(reportAction);
const message = _.last(lodashGet(reportAction, 'message', [{}]));
const originalMessage = _.get(reportAction, 'originalMessage', {});
const messageHtml = isTaskAction ? lodashGet(originalMessage, 'html', '') : lodashGet(message, 'html', '');
const isAttachment = _.has(reportAction, 'isAttachment') ? reportAction.isAttachment : ReportUtils.isReportMessageAttachment(message);
if (!isAttachment) {
const content = selection || messageHtml;
if (isReportPreviewAction) {
const iouReport = ReportUtils.getReport(ReportActionsUtils.getIOUReportIDFromReportActionPreview(reportAction));
const displayMessage = ReportUtils.getReportPreviewMessage(iouReport, reportAction);
Clipboard.setString(displayMessage);
} else if (ReportActionsUtils.isModifiedExpenseAction(reportAction)) {
const modifyExpenseMessage = ReportUtils.getModifiedExpenseMessage(reportAction);
Clipboard.setString(modifyExpenseMessage);
} else if (content) {
const parser = new ExpensiMark();
if (!Clipboard.canSetHtml()) {
Clipboard.setString(parser.htmlToMarkdown(content));
} else {
const plainText = parser.htmlToText(content);
Clipboard.setHtml(content, plainText);
}
}
} else {
Clipboard.setString(messageHtml);
}
if (closePopover) {
hideContextMenu(true, ReportActionComposeFocusManager.focus);
}
},
getDescription: () => {},
},

In the case of IOUs (user owes amount), the message is properly translated:
we are getting %user% owes %amount% message here:

if (isReportPreviewAction) {
const iouReport = ReportUtils.getReport(ReportActionsUtils.getIOUReportIDFromReportActionPreview(reportAction));
const displayMessage = ReportUtils.getReportPreviewMessage(iouReport, reportAction);
Clipboard.setString(displayMessage);

In the language file, the translation is defined as follows:

return Localize.translateLocal('iou.payerOwesAmount', {payer: payerName, amount: formattedAmount});

payerOwesAmount: ({payer, amount}: PayerOwesAmountParams) => `${payer} owes ${amount}`,

However, for requested money actions in the format requested %currency% %amount% for %comment% we are obtaining the message directly from the Onyx data here:

} else if (content) {
const parser = new ExpensiMark();
if (!Clipboard.canSetHtml()) {
Clipboard.setString(parser.htmlToMarkdown(content));
} else {
const plainText = parser.htmlToText(content);
Clipboard.setHtml(content, plainText);
}
}

What changes do you think we should make in order to solve the problem?

To resolve this issue, we should modify the copy-to-clipboard context menu action here to include a case for money request actions. We should extract the amount and currency of the request from the Onyx data and create the translated message in the format:
requested %currency% %amount% for %comment% using Localize.translateLocal
Here's the proposed code change:

else if (ReportActionsUtils.isMoneyRequestAction(reportAction)) {
    const transaction = TransactionUtils.getTransaction(originalMessage.IOUTransactionID);
    const {amount, currency, comment} = ReportUtils.getTransactionDetails(transaction);
    const formattedAmount = CurrencyUtils.convertToDisplayString(amount, currency);
    const displaymessage = Localize.translateLocal('iou.requestedAmount', {
        formattedAmount,
        comment,
    });
    Clipboard.setString(displaymessage);
}

Additionally, we need to add the translation for this message in the language files (e.g., en.ts and Spanish).

In the translation file (en.ts), add the following translation:

requestedAmount: ({formattedAmount, comment}: RequestedAmountMessageParams) => `requested ${formattedAmount}${comment ? ` for ${comment}` : ''}`,

Ensure that a similar translation is added for the Spanish language.

requestedAmount: ({formattedAmount, comment}: RequestedAmountMessageParams) => `solicité ${formattedAmount}${comment ? ` para ${comment}` : ''}`,

By making these code changes and adding translations, we will be able to properly translate and copy-to-clipboard the requested money action messages.

What alternative solutions did you explore? (Optional)

@narefyev91
Copy link
Contributor

Proposal from @getusha looks good to me #27114 (comment)
Correctly identified the root case and steps to fix looks good
🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

Triggered auto assignment to @roryabraham, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@paultsimura
Copy link
Contributor

@narefyev91 this proposal will not fix the translation issue dynamically – it will persist the message with the language that was set at the moment of IOU request creation – please see the broader issue with copying the IOU to the clipboard: #26664

This my proposal should fix the issue dynamically (with copying in the locale that's chosen for the user at the moment, not only during creation): #26664 (comment)

@narefyev91
Copy link
Contributor

@narefyev91 this proposal will not fix the translation issue dynamically – it will persist the message with the language that was set at the moment of IOU request creation – please see the broader issue with copying the IOU to the clipboard: #26664

This my proposal should fix the issue dynamically (with copying in the locale that's chosen for the user at the moment, not only during creation): #26664 (comment)

Hey @paultsimura ! Proposal indicates the root case and add logic for pick translation from languages file instead of applying it as it is now. All code changes should be discussed in PR

@paultsimura
Copy link
Contributor

@narefyev91 of course, I did not want to dive deep into code changes.
I just wanted to highlight that the selected proposal does not solve the issue.
You can ask @getusha to check their approach: first, create the IOU, then change the user's locale and try to copy text to clipboard – the localization won't work.

@getusha
Copy link
Contributor

getusha commented Sep 11, 2023

@paultsimura it's obvious after we change the hard coded values to dynamic in en.js and es.js to replace the hard coded texts in all places which the texts are used. and i don't see why it will not work.

@conorpendergrast
Copy link
Contributor

@maddylewis Looks like we both got assigned. Unassigning myself as I'm going on parental leave!

@paultsimura
Copy link
Contributor

@getusha sometimes obvious things are not that obvious. I'm not trying to steal your bounty, moreover my issue was closed as a dupe.
Just please try the steps I've mentioned, and you'll see what I'm talking about.

@getusha
Copy link
Contributor

getusha commented Sep 11, 2023

@paultsimura i understand your concern but as i mentioned the localization will be used every place where it should be wherever the Money Request action exists.

Screen.Recording.2023-09-11.at.11.48.43.AM.mov

@roryabraham
Copy link
Contributor

@getusha The only places where ReportUtils.getIOUReportActionMessage are used is when we're building optimistic IOU actions. We don't want to send localized text to the back-end because our other codebases are not internationalized.

Furthermore it's not dynamic as others have said. I think @rayane-djouah's proposal looks good.

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

@narefyev91, @maddylewis, @roryabraham, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!

@maddylewis
Copy link
Contributor

@roryabraham - did this bug fix cause a regression? #27114 (comment)

just want to confirm while i process payment / if it should be reduced by 50% for the contributor and C+ assigned to the issue.

@melvin-bot melvin-bot bot removed the Overdue label Oct 2, 2023
@maddylewis
Copy link
Contributor

Payments

@maddylewis
Copy link
Contributor

@narefyev91 - will you review this checklist when you have a chance? thanks! #27114 (comment)

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Oct 2, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-09-29] [$500] Web - Inconsistency bug: App does not translate copy to clipboard text for requested (works fine for owes) [HOLD for payment 2023-10-09] [HOLD for payment 2023-09-29] [$500] Web - Inconsistency bug: App does not translate copy to clipboard text for requested (works fine for owes) Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.75-12 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-10-09. 🎊

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.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

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:

  • [@narefyev91] The PR that introduced the bug has been identified. Link to the PR:
  • [@narefyev91] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@narefyev91] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@narefyev91] Determine if we should create a regression test for this bug.
  • [@narefyev91] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@maddylewis] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@mananjadhav
Copy link
Collaborator

@maddylewis @mountiny I had helped with the review of the PR

@mananjadhav
Copy link
Collaborator

@maddylewis Can you please post the payment summary here so that I can request the payout?

@maddylewis maddylewis added Daily KSv2 and removed Weekly KSv2 labels Oct 12, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 12, 2023
@maddylewis
Copy link
Contributor

yes! i will process payments / post an updated payment summary before EOD!

@melvin-bot melvin-bot bot removed the Overdue label Oct 12, 2023
@rayane-djouah
Copy link
Contributor

Friendly reminder @maddylewis

@maddylewis
Copy link
Contributor

maddylewis commented Oct 13, 2023

Payments

Contributor hired on 9/11. PR merged on 9/26.

@maddylewis
Copy link
Contributor

@narefyev91 - will you please review/fill out this checklist when you have a chance? thanks! #27114 (comment)

@maddylewis
Copy link
Contributor

maddylewis commented Oct 13, 2023

offers sent to @dhanashree-sawant and @rayane-djouah

https://www.upwork.com/ab/applicants/1712902083964887040/offers

@rayane-djouah
Copy link
Contributor

Offer accepted @maddylewis

@JmillsExpensify
Copy link

$500 payment approved for @mananjadhav based on BZ summary.

@narefyev91
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR - current PR is more for missing functionality rather than regression fix
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment - N/A
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion - N/A
  • Determine if we should create a regression test for this bug - N/A

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests