[HOLD for payment 2024-08-02] [$250] Android - Hm it's not here shown when edit the tax code and save

lanitochka17 commented 3 months ago

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.10-4 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 Email or phone of affected tester (no customers): rybkina+071824@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Launch App and login as user with Control policy
Go to Control policy
Go to taxes page
Tap a tax rate
Edit the tax code and save

Expected Result:

The tax code should edit and and save

Actual Result:

Hm it's not here shown when edit the tax code and save

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/eafa06bf-78fa-469b-9ac7-6d7d05587407

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01ed44e8676aed82e6
Upwork Job ID: 1815419014769507661
Last Price Increase: 2024-07-22
Automatic offers:

eh2077 | Reviewer | 103224942
nkdengineer | Contributor | 103224944

Issue Owner

Current Issue Owner: @joekaufmanexpensify

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @joekaufmanexpensify (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.

joekaufmanexpensify commented 3 months ago

Reproduced. We should probably clean this up. Customers might think the "hm not here" message indicates an issue saving the code they selected.

https://github.com/user-attachments/assets/590b229d-50d8-4f1d-83e9-4c7e34630d6c

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

nkdengineer commented 3 months ago

Proposal

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

Hm it's not here shown when edit the tax code and save

What is the root cause of that problem?

When we edit the tax code we navigate to the tax edit page with taxID param as newTaxCode.

https://github.com/Expensify/App/blob/564156cdb88420a554861f61aee9d12d0b92eab9/src/pages/workspace/taxes/WorkspaceTaxCodePage.tsx#L44-L45

And because there are some delays when we merge the onyx data here, the not found page appears briefly

https://github.com/Expensify/App/blob/564156cdb88420a554861f61aee9d12d0b92eab9/src/pages/workspace/taxes/WorkspaceEditTaxPage.tsx#L62-L64

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

Since we already stored the previousTaxCode in Onyx here, we can use the same solution we apply for category or tag when we edit the name of the category/tag.

We can simply use goBack here instead of dismissModal and navigate

Navigation.goBack(ROUTES.WORKSPACE_TAX_EDIT.getRoute(policyID, currentTaxCode));

https://github.com/Expensify/App/blob/564156cdb88420a554861f61aee9d12d0b92eab9/src/pages/workspace/taxes/WorkspaceTaxCodePage.tsx#L44-L45

Create a util to get the current taxID

function getCurrentTaxID(policy: OnyxEntry<Policy>, taxID: string): string | undefined {
    return Object.keys(policy?.taxRates?.taxes ?? {}).find((taxIDKey) => policy?.taxRates?.taxes?.[taxIDKey].previousTaxCode === taxID || taxIDKey === taxID);
}

In WorkspaceEditTaxPage, change the taxID param to the new taxID param if the currentTaxID is different from taxID

const currentTaxID = PolicyUtils.getCurrentTaxID(policy, taxID);
const currentTaxRate = currentTaxID && policy?.taxRates?.taxes?.[currentTaxID];
useEffect(() => {
    if (currentTaxID === taxID || !currentTaxID) {
        return;
    }
    navigation.setParams({taxID: currentTaxID})
}, [navigation, taxID, currentTaxID])

What alternative solutions did you explore? (Optional)

NA

Result

https://github.com/user-attachments/assets/cf9acd66-def5-4367-81dc-832c6588157a

eh2077 commented 3 months ago

@nkdengineer 's proposal looks good to me. I think it makes sense to apply similar fix as editing tag/catetory to avoid displaying not here page briefly.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @mountiny, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 3 months ago

📣 @eh2077 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 3 months ago

📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

https://github.com/Expensify/App/pull/45983

If no regressions arise, payment will be issued on 2024-08-02. :confetti_ball:

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

@eh2077 requires payment automatic offer (Reviewer)
@nkdengineer requires payment automatic offer (Contributor)

melvin-bot[bot] commented 3 months ago

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:

[x] [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I can't figure out a specific that's responsible for this issue. It's an edge case that we want to polish
[x] [@eh2077] 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: N/A
[x] [@eh2077] A discussion in #expensify-bugs 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: N/A
[x] [@eh2077] Determine if we should create a regression test for this bug.
[x] [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[x] [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: N/A

joekaufmanexpensify commented 3 months ago

@eh2077 please handle BZ checklist so we can prep to issue payment this week

joekaufmanexpensify commented 3 months ago

Bumped in Slack

eh2077 commented 3 months ago

Checklist

[x] [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I can't figure out a specific that's responsible for this issue. It's an edge case that we want to polish
[x] [@eh2077] 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: N/A
[x] [@eh2077] A discussion in #expensify-bugs 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: N/A
[x] [@eh2077] Determine if we should create a regression test for this bug. I'm neutral about adding a regression test

Regression test

Launch App and login as user with Control policy
Go to Control policy
Go to taxes page
Tap a tax rate
Edit the tax code and save
Verify that: The tax code should edit and and save

Do we agree 👍 or 👎

joekaufmanexpensify commented 3 months ago

There's actually already an existing regression test for this so we should be good to go there.

joekaufmanexpensify commented 3 months ago

Checklist is all set! All set to issue payment soon

joekaufmanexpensify commented 3 months ago

All set to issue payment! We need to pay:

@nkdengineer $250 for PR via Upwork.
@eh2077 $250 for C+ via Upwork.

joekaufmanexpensify commented 3 months ago

@nkdengineer $250 sent and contract ended!

joekaufmanexpensify commented 3 months ago

@eh2077 $250 sent and contract ended!

joekaufmanexpensify commented 3 months ago

Upwork job closed.

joekaufmanexpensify commented 3 months ago

All set, thanks everyone!

Expensify / App