-
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
[HOLD for payment 2024-07-10] [$250] Track expense - "Delete expense" option appears for paid tracked expense #43591
Comments
Triggered auto assignment to @kevinksullivan ( |
@kevinksullivan 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 |
ProposalPlease re-state the problem that we are trying to solve in this issue."Delete expense" option appears for paid tracked expense What is the root cause of that problem?App/src/components/MoneyReportHeader.tsx Lines 81 to 82 in 9376991
We manually add isTrackExpenseReport condition to display the delete option in paid track expense What changes do you think we should make in order to solve the problem?Before we need to add isTrackExpenseReport condition because canAddOrDeleteTransactions doesn't cover track expense case. At present, let's remove isTrackExpenseReport condition because canAddOrDeleteTransactions also cover track expense case. Consider to make the same fix to MoneyRequestHeader What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.Delete expense option is shown for paid track expense. What is the root cause of that problem?The delete option is always shown for track expense report. App/src/components/MoneyRequestHeader.tsx Line 103 in a451de4
This is useful for self DM track expense report because the App/src/components/MoneyRequestHeader.tsx Line 63 in a451de4
so chat report -> money request report -> transaction thread But in self DM track expense, we only have the track expense report/transaction thread self DM -> track expense report What changes do you think we should make in order to solve the problem?We need to also check whether the track expense is a self DM track or not.
We also have the same condition in MoneyReportHeader that is shown in the money request report, but self DM track doesn't have money request report, so we can remove it (remove ReportUtils.isTrackExpenseReport(report)) App/src/components/MoneyReportHeader.tsx Lines 81 to 82 in a451de4
What alternative solutions did you explore? (Optional)In Lines 1506 to 1509 in a451de4
But I don't like this one because Instead, we can add a new optional param to |
@kevinksullivan Huh... This is 4 days overdue. Who can take care of this? |
Job added to Upwork: https://www.upwork.com/jobs/~01bb7f209695580cf5 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 ( |
Moving to vip-vsb and making external. @eh2077 can you review the proposals above when you get a chance? Thanks! |
We should handle the special case of self DM track expense for the delete option. @bernhardoj 's proposal looks good to me. 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @NikkiWines, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Agreed @bernhardoj's proposal looks good 👍 |
📣 @eh2077 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
PR is ready cc: @eh2077 |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 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-07-10. 🎊 For reference, here are some details about the assignees on this 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:
|
Checklist
Regression test
Do we agree 👍 or 👎 |
I requested the payment in ND. |
@NikkiWines, @kevinksullivan, @bernhardoj, @eh2077 Eep! 4 days overdue now. Issues have feelings too... |
Payment summary:
|
All set paying @eh2077 |
$250 approved for @bernhardoj |
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.82-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: N/A
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
"Delete expense" option will not appear for paid tracked expense
Actual Result:
"Delete expense" option appears for paid tracked expense
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6511017_1718207360141.20240612_234631.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @kevinksullivanThe text was updated successfully, but these errors were encountered: