[HOLD for payment 2025-02-05] [$250] Reports - Pay button does not change to Review button after unselecting category

[vincdargento]

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: 9.0.86-0
Reproducible in staging?: Yes
Reproducible in production?: Unable to check (feature added in PR #54847)
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: N/A
Email or phone of affected tester (no customers): Mac 15.0 / Chrome
Issue reported by: Applause Internal Team
Device used: applausetester+100106kh@applause.expensifail.com
App Component: Search

Action Performed:

Precondition:

• "Members must categorize all expenses" is enabled in Categories settings.

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Submit two expenses with category.
4. Go to Reports.
5. Go to Outstanding.
6. Click View on any individual expense from Step 3.
7. Click Category.
8. Click on the selected category to unselect it.
9. Close the RHP.

Expected Result:

Pay button on the grouped expense should change to Review button because one of the expenses has violation.

Actual Result:

Pay button on the grouped expense does not change to Review button after unselecting the category.

It only shows Review button after refreshing the page.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021879572765790104174
• Upwork Job ID: 1879572765790104174
• Last Price Increase: 2025-01-15
• Automatic offers:
• nkdengineer | Contributor | 105775204

Issue Owner

Current Issue Owner: @stephanieelliott

[melvin-bot]

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

Triggered auto assignment to @aldo-expensify (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[vincdargento]

@stephanieelliott 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.

[luacmartins]

I mentioned this case here and it's NAB. We can keep the issue open to get some proposals on how to fix this though

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

Proposal

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

Pay button on the grouped expense does not change to Review button after unselecting the category.

What is the root cause of that problem?

When we create the expense without violation, the violation key doesn't exist in the snapshot data. Then when we update the category, the violation data isn't updated in the snapshot data because the snapshot data of this key doesn't exist.

https://github.com/Expensify/react-native-onyx/blob/9c028634e8b980772913455c8601eee69d4c8953/lib/Onyx.ts#L581-L583

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

We can do the same way we do when we hold/unhold money request by updating the snapshot data manually.

1. Define new param hash in updateMoneyRequestCategory and getUpdateMoneyRequestParams and pass the hash here

App/src/libs/actions/IOU.ts

Line 3654 in 28deaad

const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy, policyTagList, policyCategories);

const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy, policyTagList, policyCategories, undefined, hash);

2. Update the violation to snapshot data if hash exist

const currentTransactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? [];
const violationsOnyxdata = ViolationsUtils.getViolationsOnyxData(
    updatedTransaction,
    currentTransactionViolations,
    policy,
    policyTagList ?? {},
    policyCategories ?? {},
    hasDependentTags(policy, policyTagList ?? {}),
);
optimisticData.push(violationsOnyxdata);

failureData.push({
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`,
    value: currentTransactionViolations,
});

if (hash) {
    optimisticData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`,
        value: {
            data: {
                [`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`]: violationsOnyxdata.value
            }
        }
    })
    failureData.push({
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`,
        value: {
            data: {
                [`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`]: currentTransactionViolations
            }
        }
    })
}

App/src/libs/actions/IOU.ts

Line 3218 in 28deaad

const currentTransactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? [];

3. In IOURequestStepCategory, get the current has from useSearchContext and pass it to updateMoneyRequestCategory function here

const {currentSearchHash} = useSearchContext();
IOU.updateMoneyRequestCategory(transaction.transactionID, report.reportID, updatedCategory, policy, policyTags, policyCategories, currentSearchHash);

App/src/pages/iou/request/step/IOURequestStepCategory.tsx

Line 123 in 28deaad

IOU.updateMoneyRequestCategory(transaction.transactionID, report.reportID, updatedCategory, policy, policyTags, policyCategories);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

NA

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[luacmartins]

@nkdengineer your proposal looks good. @sobitneupane do you have any concerns about it?

[iwiznia]

Define new param hash in updateMoneyRequestCategory and getUpdateMoneyRequestParams and pass the hash here

Why only these 2? There's a ton of commands that modify a transaction and I think all of them could create a violation.

Update the violation to snapshot data if hash exist

Why would it not exist? Shouldn't we be passing hash in all callers of the method, so in all cases we update the search? Is it because we will only include the hash if we are on the search page and if so, wouldn't that mean that search will be outdated if you navigate to it later?

[luacmartins]

Why only these 2? There's a ton of commands that modify a transaction and I think all of them could create a violation.

Well, getUpdateMoneyRequestParams is used by all update commands, so yea, it'd be passed as a param by all those flows.

Update the violation to snapshot data if hash exist

Hmm interesting, I understood this one as if the violation exists in the hash, which might not if the transaction didnt initially have a violation (this is the current cause of the bug). But after re-reading the comment, I see what you mean and I think we need to clarify that.

[nkdengineer]

getUpdateMoneyRequestParams

@luacmartins Yes, this function is used for all update flows. For optimistic violation, we only have tag/category violation so I think we can only pass the hash when updating the snapshot when we update tag/category.

[luacmartins]

Yea, that makes sense.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.90-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Pay button does not change to Review button #55500

If no regressions arise, payment will be issued on 2025-02-05. 🎊

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

• @sobitneupane requires payment through NewDot Manual Requests
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot]

@sobitneupane @stephanieelliott @sobitneupane 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]

[melvin-bot]

@luacmartins, @sobitneupane, @stephanieelliott, @nkdengineer Whoops! This issue is 2 days overdue. Let's get this updated quick!

[luacmartins]

Just pending payment

[stephanieelliott]

@sobitneupane requires payment through NewDot Manual Requests
@nkdengineer requires payment automatic offer (Contributor)

Summarizing payment on this issue:

• Contributor: @nkdengineer $250 via Upwork - PAID
• Contributor+: @sobitneupane $250 via ND - please request!

Upwork job is here: https://www.upwork.com/jobs/~021889214956264201541

[sobitneupane]

Regression Test Proposal

1. Go to workspace chat.
2. Submit two expenses with category.
3. Go to Reports.
4. Go to Outstanding.
5. Click View on any individual expense from Step 3.
6. Click Category.
7. Click on the selected category to unselect it.
8. Close the RHP.
9. Verify that: Pay button on the grouped expense changes to Review button

Do we agree 👍 or 👎

[sobitneupane]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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: Add violations to Search actions #54847

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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:

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

[stephanieelliott]

Regression test created: https://github.com/Expensify/Expensify/issues/469880

[stephanieelliott]

All paid! Closing.