App icon indicating copy to clipboard operation
App copied to clipboard

[$500] Expense - System message for paid expense shows payer, while the copied content is workspace

Open lanitochka17 opened this issue 1 year ago • 40 comments

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


Version Number: 1.4.20-2 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 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

  1. [Member] Request money in workspace chat
  2. [Admin] Pay the request
  3. [Member] Click on the expense preview to go to expense report
  4. [Member] Right click on the paid system message > Copy to clipboard
  5. [Member] Paste the content Note that

Expected Result:

  1. The system message should always start with the action, not the actor, regardless of who is viewing the chat. So the system message should read paid $10 rather than John paid $10 or John's Workspace paid $10.
  2. When a user copies a system message, we should copy only what is printed, rather than adding the actor (so we should copy paid $10 instead of copying John Doe paid $10)

Actual Result:

  1. Depending on who is viewing the report, the system message shows either paid amount elsewhere or [admin] paid amount elsewhere.
  2. When the user copies and pastes the system message, it shows something different from what was printed originally (it shows [Workspace name] paid amount elsewhere.

Workaround:

Unknown

Platforms:

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

  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/5e7957cd-b471-49d5-be0c-4a263da2187f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015bfbfa9935f7c034
  • Upwork Job ID: 1741887749877055488
  • Last Price Increase: 2024-01-22

lanitochka17 avatar Jan 01 '24 18:01 lanitochka17

Job added to Upwork: https://www.upwork.com/jobs/~015bfbfa9935f7c034

melvin-bot[bot] avatar Jan 01 '24 18:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 01 '24 18:01 melvin-bot[bot]

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[bot] avatar Jan 01 '24 18:01 melvin-bot[bot]

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

melvin-bot[bot] avatar Jan 01 '24 18:01 melvin-bot[bot]

Proposal

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

Clipboard information is different from what you see in chat after the expense paid action.

What is the root cause of that problem?

https://github.com/Expensify/App/blob/fe49e4f7fb9677a5834b53cb776f79fb20f893ce/src/libs/ReportUtils.ts#L4106 https://github.com/Expensify/App/blob/fe49e4f7fb9677a5834b53cb776f79fb20f893ce/src/libs/ReportUtils.ts#L2011 As you can see, in these two lines, we choose payerName for information in the clipboard and also for showing in the system message.

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

https://github.com/Expensify/App/blob/cafe83123d6b94d668238c85fb9a19e7b81c43c6/src/libs/ReportUtils.ts#L4124-L4133 Here are three different options with "PayerName": paidElsewhereWithAmount, paidWithExpensifyWithAmount, payerPaidAmount. Simply remove PayerName in their translation function, and then remove payerName on every usage of these functions (they are also used in another function called getReportPreviewMessage()).

I think there is a reason for a different function. At first glance, I won't remove the function getIOUReportActionDisplayMessage().

What alternative solutions did you explore? (Optional)

rrrshtt avatar Jan 01 '24 18:01 rrrshtt

Proposal

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

System message for paid expense shows payer, while the copied content is workspace

What is the root cause of that problem?

We show policy name instead of payer display name in case of policy:

https://github.com/Expensify/App/blob/70967dd9669db289eefc996145eed2304cdc31a4/src/libs/ReportUtils.ts#L4093

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

The problem here is about the inconsistency between the system message and the copied content. According to this issue:

The reportAction added to the iouReport/expenseReport comes from the reimburser who paid

We no longer need to show workspace name for reimbursement messages because it will always show the actual payer name. And thus we should remove the isExpenseReport check and always show display name:

https://github.com/Expensify/App/blob/70967dd9669db289eefc996145eed2304cdc31a4/src/libs/ReportUtils.ts#L4093

The check (i.e. show workspace or payer name) is now only necessary for LHN or report preview.

What alternative solutions did you explore? (Optional)

NA

gijoe0295 avatar Jan 01 '24 19:01 gijoe0295

Proposal

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

The system message shows paid amount elsewhere, but the copied content is paid amount elsewhere

What is the root cause of that problem?

  1. In getReportPreviewMessage function, we show the payment type for the case request is settled

https://github.com/Expensify/App/blob/d33862194b659f4877102f0db859a32914a7564c/src/libs/ReportUtils.ts#L2284

  1. When we click on copy to clipboard, we use getIOUReportActionDisplayMessage function to get the display message and in this function, we will display the workspace name instead of the actor name.

https://github.com/Expensify/App/blob/70967dd9669db289eefc996145eed2304cdc31a4/src/libs/ReportUtils.ts#L4093

but when we display this action, we use getReportPreviewMessage function to get iouMessage.

https://github.com/Expensify/App/blob/70967dd9669db289eefc996145eed2304cdc31a4/src/pages/home/report/ReportActionItemMessage.tsx#L56

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

  1. In getReportPreviewMessage, for the settled case we can use iou.payerPaidAmount translation key to not display the payment type of the settle message if we get this message for report preview. We can add an extra param to know whether we get the message for the report preview. We should add an extra prop to know whether we should hide the payerName.

https://github.com/Expensify/App/blob/d33862194b659f4877102f0db859a32914a7564c/src/libs/ReportUtils.ts#L2284

And then when we get the message here we will pass this param as true to hide the payerName in system paid message.

https://github.com/Expensify/App/blob/70967dd9669db289eefc996145eed2304cdc31a4/src/pages/home/report/ReportActionItemMessage.tsx#L56

  1. When we click on copy to clipboard, we should copy the same message displayed by using getReportPreviewMessage function in the same way we do here.

https://github.com/Expensify/App/blob/70967dd9669db289eefc996145eed2304cdc31a4/src/libs/ReportUtils.ts#L4093

https://github.com/Expensify/App/blob/70967dd9669db289eefc996145eed2304cdc31a4/src/pages/home/report/ReportActionItemMessage.tsx#L56

What alternative solutions did you explore? (Optional)

For point 2, we can remove the payerName in getIOUReportActionDisplayMessage function. But I'd like to use getReportPreviewMessage rather than update getIOUReportActionDisplayMessage function because if we make an update in getReportPreviewMessage function, we don't need to update other functions anymore.

dukenv0307 avatar Jan 02 '24 09:01 dukenv0307

Something is definitely wrong here, but I'm not sure what the expected behavior is. Should the system message show the admin's name, the workspace's name, or neither? I'm confident that the system message should NOT show the admin's name, but I'm not sure which of the other two it should show.

I asked for the team's feedback in Slack: https://expensify.slack.com/archives/C02MW39LT9N/p1704233160288869

sakluger avatar Jan 02 '24 22:01 sakluger

.

aim-salam avatar Jan 03 '24 04:01 aim-salam

@sakluger You can refer to this issue: https://github.com/Expensify/App/issues/30042.

gijoe0295 avatar Jan 03 '24 05:01 gijoe0295

I updated the expected behavior and actual behavior. Expected result is now:

  1. The system message should always start with the action, not the actor, regardless of who is viewing the chat. So the system message should read paid $10 rather than John paid $10 or John's Workspace paid $10.
  2. When a user copies a system message, we should copy only what is printed, rather than adding the actor (so we should copy paid $10 instead of copying John Doe paid $10)

@rrrshtt @gijoe0295 @dukenv0307 would you like to update your proposal based on the new expected result?

sakluger avatar Jan 04 '24 21:01 sakluger

Updated proposal.

dukenv0307 avatar Jan 04 '24 22:01 dukenv0307

Updated Proposal

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

System message for paid expense shows payer, while the copied content is workspace

What is the root cause of that problem?

We show policy name instead of payer display name in case of policy:

https://github.com/Expensify/App/blob/70967dd9669db289eefc996145eed2304cdc31a4/src/libs/ReportUtils.ts#L4093

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

Based on the latest expectation:

The system message should always start with the action

There are cases that needs getting rid of actor: here, here, here, and here

I'm not sure the expectation with LHN preview but I'm assuming that LHN preview message should still have actor name. Because getReportPreviewMessage is used for LHN preview as well, we should use the check isForListPreview.

We should copy only what is printed

We should use getReportPreviewDisplay for consistency:

https://github.com/Expensify/App/blob/cafe83123d6b94d668238c85fb9a19e7b81c43c6/src/pages/home/report/ContextMenu/ContextMenuActions.js#L285

What alternative solutions did you explore? (Optional)

NA

gijoe0295 avatar Jan 04 '24 22:01 gijoe0295

Proposal

Updated

rrrshtt avatar Jan 04 '24 22:01 rrrshtt

@0xmiroslav hopefully one of the above proposals does the trick! Mind taking a look when you hae a chance?

sakluger avatar Jan 05 '24 20:01 sakluger

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Jan 08 '24 16:01 melvin-bot[bot]

reviewing

0xmiroslav avatar Jan 08 '24 16:01 0xmiroslav

@0xmiroslav friendly bump.

sakluger avatar Jan 10 '24 23:01 sakluger

updating today

0xmiroslav avatar Jan 12 '24 11:01 0xmiroslav

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Jan 15 '24 16:01 melvin-bot[bot]

finalizing review

0xmiroslav avatar Jan 15 '24 16:01 0xmiroslav

@sakluger @0xmiroslav this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Jan 15 '24 17:01 melvin-bot[bot]

@sakluger one more thing to confirm:

payer name should be removed in LHN for ReportPreview as well? (this is workspace chat, not expense report)

Screenshot 2024-01-17 at 8 10 15 am

0xmiroslav avatar Jan 17 '24 07:01 0xmiroslav

Oh good question. I think the answer is yes, but I'm going to double check in Slack.

sakluger avatar Jan 19 '24 19:01 sakluger

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Jan 22 '24 16:01 melvin-bot[bot]

@sakluger @0xmiroslav this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] avatar Jan 22 '24 17:01 melvin-bot[bot]

We have proposals. But just waiting for final confirmation before pick up

0xmiroslav avatar Jan 22 '24 17:01 0xmiroslav

Sorry, still discussing expected LHN behavior in Slack.

sakluger avatar Jan 23 '24 23:01 sakluger

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

melvin-bot[bot] avatar Jan 24 '24 03:01 melvin-bot[bot]

@DylanDylann reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks

mallenexpensify avatar Jan 24 '24 03:01 mallenexpensify