-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[Due for payment 2025-03-13] [$250] Unapprove button is showing instead of Cancel payment button on reports queued for ACH reimbursement (and incorrect sys message when actioned on from OldDot) #56989
Comments
Job added to Upwork: https://www.upwork.com/jobs/~021891677107780037168 |
Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-d ( |
CC'ing @Gonals for vis here, as I believe you worked on the initial implementation of |
ProposalPlease re-state the problem that we are trying to solve in this issue.Unapprove button is showing instead of Cancel payment button on reports queued for ACH reimbursement (and incorrect sys message when actioned on from OldDot) What is the root cause of that problem?
So when we're getting right here it will return null or Lines 2927 to 2936 in 1eb00b0
What changes do you think we should make in order to solve the problem?
const shouldShowCancelPaymentButton = caseID === CASES.MONEY_REPORT && isPayer && moneyRequestReport?.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED && isExpenseReportUtil(moneyRequestReport); Note If we change the isSettled function inside the ReportUtils to return true even if
const canUnapproveRequest = isExpenseReportUtil(report) && (isReportManagerUtil(report) || isPolicyAdmin) && isReportApprovedUtil({report}) && !isSubmitAndClose(policy) && !isSettled;
// or
const canUnapproveRequest = isExpenseReportUtil(report) && (isReportManagerUtil(report) || isPolicyAdmin) && isReportApprovedUtil({report}) && !isSubmitAndClose(policy) && moneyRequestReport?.statusNum !== CONST.REPORT.STATUS_NUM.REIMBURSED;
function getReimbursementDeQueuedActionMessage(
reportAction: OnyxEntry<ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.REIMBURSEMENT_DEQUEUED>>,
reportOrID: OnyxEntry<Report> | string | SearchReport,
isLHNPreview = false,
): string {
const report = typeof reportOrID === 'string' ? getReport(reportOrID, allReports) : reportOrID;
const originalMessage = getOriginalMessage(reportAction);
const {totalDisplaySpend: totalReportDisplaySpend} = getMoneyRequestSpendBreakdown(report);
const amount = originalMessage?.amount ?? totalReportDisplaySpend;
const currency = originalMessage?.currency ?? report?.currency;
const formattedAmount = convertToDisplayString(amount, currency);
if (originalMessage?.cancellationReason === CONST.REPORT.CANCEL_PAYMENT_REASONS.ADMIN) {
const payerOrApproverName = report?.managerID === currentUserAccountID || !isLHNPreview ? '' : getDisplayNameForParticipant({accountID: report?.managerID, shouldUseShortForm: true});
return translateLocal('iou.adminCanceledRequest', {manager: payerOrApproverName, amount: formattedAmount});
}
const submitterDisplayName = getDisplayNameForParticipant({accountID: report?.ownerAccountID, shouldUseShortForm: true}) ?? '';
return translateLocal('iou.canceledRequest', {submitterDisplayName, amount: formattedAmount});
} What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?We should create unit test function to cover this cases
What alternative solutions did you explore? (Optional) |
🚨 Edited by proposal-police: This proposal was edited at 2025-02-24 03:36:31 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.When paying an expense with a pending bank account, unapprove is shown instead of cancel payment. What is the root cause of that problem?When we are waiting for bank account, the report status is APPROVED. The cancel payment will show if the report is settled. In App/src/pages/ReportDetailsPage.tsx Line 380 in 0b6d1a0
Lines 1265 to 1273 in 011fd6c
However, the logic is never reached because we return false if App/src/pages/ReportDetailsPage.tsx Line 307 in 0b6d1a0
Lines 1213 to 1217 in 011fd6c
What changes do you think we should make in order to solve the problem?First of all, this logic shouldn't be put inside the Lines 1269 to 1273 in 0b6d1a0
To fix this issue, we need to update App/src/pages/ReportDetailsPage.tsx Line 380 in 0b6d1a0
This is consistent with other parts of the app. App/src/components/ReportActionItem/ReportPreview.tsx Lines 389 to 390 in 0b6d1a0
Lines 3993 to 3994 in 0b6d1a0
Second, we need to update App/src/pages/ReportDetailsPage.tsx Line 307 in 0b6d1a0
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?To be able to test it, we can move |
@trjExpensify, @rayane-d Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Reviewing today |
I think the correct proposals is from @bernhardoj 😅, but I think there's an issue with their proposal, you can look at it in my proposal (NOTE markdown) |
@bernhardoj What do you think about this? |
You're right. We shouldn't change It's just the button that we need to conditionally shows based on waiting on bank account status. I've updated my proposal. |
@bernhardoj's proposal looks good to me 🎀👀🎀 C+ reviewed |
Current assignee @Gonals is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
PR is ready cc: @rayane-d |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.9-8 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 2025-03-13. 🎊 For reference, here are some details about the assignees on this issue:
|
@rayane-d @trjExpensify @rayane-d 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] |
BugZero Checklist:
Bug classificationSource of bug:
Where bug was reported:
Who reported the bug:
Regression Test ProposalPrecondition:
Test:
Do we agree 👍 or 👎 |
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: v9.0.99-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: None
Email or phone of affected tester (no customers): tom+vbbaod2nd@trj.chat
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL: None
Issue reported by: @trjExpensify
Slack conversation (hyperlinked to channel name): None, found when I was testing something else.
Action Performed:
Prerequisites:
Unapprove
button shows.Unapprove
button showing on the report in OldDotcanceledRequest
system message is added to the report:canceled the $0.00 payment, because tom+vbbaod2nd@trj.chat did not enable their Expensify Wallet within 30 days
Expected Result:
Unapprove
button shouldn't show on a report that is queued for reimbursement via ACH (as the payment hasn't been canceled yet).adminCanceledRequest
system message should be shown when canceling payments on a workspace.Actual Result:
Unapprove
button incorrectly shows on a report that's queued to be reimbursed via ACH.Workaround:
You can use OldDot to action on canceling the payment at least.
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
2025-02-18_01-54-48.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @trjExpensifyThe text was updated successfully, but these errors were encountered: