-
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
[$500] Expense - System message for paid expense shows payer, while the copied content is workspace #33819
Comments
Job added to Upwork: https://www.upwork.com/jobs/~015bfbfa9935f7c034 |
Triggered auto assignment to @sakluger ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @0xmiroslav ( |
ProposalPlease 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?Line 4106 in fe49e4f
Line 2011 in fe49e4f
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?Lines 4124 to 4133 in cafe831
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 What alternative solutions did you explore? (Optional) |
ProposalPlease 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: Line 4093 in 70967dd
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:
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 Line 4093 in 70967dd
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 |
ProposalPlease 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?
Line 2284 in d338621
Line 4093 in 70967dd
but when we display this action, we use
What changes do you think we should make in order to solve the problem?
Line 2284 in d338621
And then when we get the message here we will pass this param as
Line 4093 in 70967dd
What alternative solutions did you explore? (Optional)For point 2, we can remove the |
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 |
. |
I updated the expected behavior and actual behavior. Expected result is now:
@rrrshtt @gijoe0295 @dukenv0307 would you like to update your proposal based on the new expected result? |
Updated proposal. |
Updated ProposalPlease 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: Line 4093 in 70967dd
What changes do you think we should make in order to solve the problem?Based on the latest expectation:
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
We should use
What alternative solutions did you explore? (Optional)NA |
Proposal |
@0xmiroslav hopefully one of the above proposals does the trick! Mind taking a look when you hae a chance? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
reviewing |
@0xmiroslav friendly bump. |
updating today |
Current assignee @DylanDylann is eligible for the External assigner, not assigning anyone new. |
📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@DylanDylann The PR is ready for review. |
@trjExpensify Currently, I see that in some cases the preview message and the message from the "Copy to clipboard" button are different. Screen.Recording.2024-02-15.at.15.30.42.movIn the above video:
Is this expected? What do you think about displaying The message from "Copy to clipboard" similar to Preview message in LHN |
Yeah, I think generally what you copy should be what you see, so let's copy the message being previewed. P.s why does the receipt thumbnail appear to zoom like that, man? 😂 👀 |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
We are reverting the PR: #36932 I think a few considerations were missed as part of the planning of this |
Left a comment here, I think it might be better to merge the revert PR first and then focus on fixing this issue after. |
@sakluger The PR was deployed to production 2 weeks ago. Please help to process the next step |
@sakluger Bump this comment cc @srikarparsi |
Sorry! All paid out now. |
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: 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:
Note that
Expected Result:
paid $10
rather thanJohn paid $10
orJohn's Workspace paid $10
.paid $10
instead of copyingJohn Doe paid $10
)Actual Result:
paid amount elsewhere
or[admin] paid amount elsewhere
.[Workspace name] paid amount elsewhere
.Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6329831_1704112491311.bandicam_2023-12-30_12-33-56-983.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: