[HOLD for payment 2023-12-29] [$500] Tag - Tag selected from another workspace is still preserved when switching workspace

[kbecciv]

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: v1.4.12-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:

Precondition:

• Workspace A has a set of tags A.
• Workspace B has a set of tags B.

1. Click FAB > Request money > Distance.
2. Enter start and finish point.
3. Select Workspace A.
4. Click Tag and select another tag.
5. Click on the back button.
6. Select Workspace B.

Expected Result:

The tag field in the confirmation page will reset when switching workspace.

Actual Result:

The tag selected from Workspace A is still preserved when Workspace B is selected.

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6313137_1702563673701.bandicam_2023-12-14_15-51-20-958.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0194acf0b0a5cd5e3a
• Upwork Job ID: 1735307898523672576
• Last Price Increase: 2023-12-14
• Automatic offers:
• jjcoffee | Reviewer | 28061731
• FitseTLT | Contributor | 28061732

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[FitseTLT]

Proposal

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

Tag selected from another workspace is still preserved when switching workspace

What is the root cause of that problem?

We are not resetting the tag/category on user changing different workspace as a participant or basically we are not resetting them while navigating to the confirmation page in the new refactored participant page IOURequestStepParticipants as we did in MoneyRequestParticipantsPage here

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Lines 90 to 93 in 904cf56

	const navigateToConfirmationStep = (moneyRequestType) => {
	IOU.setMoneyRequestId(moneyRequestType);
	IOU.resetMoneyRequestCategory();
	IOU.resetMoneyRequestTag();

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

We should reset tag and category when user changes workspace (participant) or the same as we did before we can reset them when always navigating to the confirmation page as we did here

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Lines 90 to 93 in 904cf56

	const navigateToConfirmationStep = (moneyRequestType) => {
	IOU.setMoneyRequestId(moneyRequestType);
	IOU.resetMoneyRequestCategory();
	IOU.resetMoneyRequestTag();

so change this

App/src/pages/iou/request/step/IOURequestStepParticipants.js

Lines 72 to 75 in 904cf56

	const goToNextStep = useCallback(() => {
	const nextStepIOUType = numberOfParticipants.current === 1 ? iouType : CONST.IOU.TYPE.SPLIT;
	Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(nextStepIOUType, transactionID, selectedReportID.current || reportID));
	}, [iouType, transactionID, reportID]);

const goToNextStep = useCallback(() => {
        const nextStepIOUType = numberOfParticipants.current === 1 ? iouType : CONST.IOU.TYPE.SPLIT;
        IOU.resetMoneyRequestTag_temporaryForRefactor(transactionID);
        IOU.resetMoneyRequestCategory_temporaryForRefactor(transactionID);
        Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(nextStepIOUType, transactionID, selectedReportID.current || reportID));
    }, [iouType, transactionID, reportID]);
   

and also reset here, here, here , here and here

What alternative solutions did you explore? (Optional)

[DylanDylann]

Proposal

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

The tag selected from Workspace A is still preserved when Workspace B is selected.

What is the root cause of that problem?

when re-select the participant we don't re-set tag and category

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

We need to reset transactionsDraft_1.tag and transactionsDraft_1.category when user re-select participant by calling

            IOU.resetMoneyRequestCategory_temporaryForRefactor(transactionID);
            IOU.resetMoneyRequestTag_temporaryForRefactor(transactionID);

Optional: We also should reset transactionsDraft_1.merchant

What alternative solutions did you explore? (Optional)

NA

[jjcoffee]

@FitseTLT's proposal LGTM! With the caveat that we probably don't want to reset tags and category if the user doesn't select a different workspace (i.e. just going back and forth between without changing the workspace).

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[DylanDylann]

@jjcoffee It seems @FitseTLT copied my proposal. Please check edit history

[DylanDylann]

@FitseTLT's original proposal is invalid

[FitseTLT]

We need to reset transactionsDraft_1.tag and transactionsDraft_1.category when user re-select participant by calling

@DylanDylann this is not even the correct proposal because we shouldn't reset when the participant is not different from the previously selected one that's what @jjcoffee is mentioning in his review and also about my proposal I just commented a draft propsal first with the basic idea and cleaned it doesn't mean I copied from U.

[DylanDylann]

@jjcoffee My proposal is the first one that suggests using

           IOU.resetMoneyRequestCategory_temporaryForRefactor(transactionID);
            IOU.resetMoneyRequestTag_temporaryForRefactor(transactionID);

@FitseTLT's original proposal, he suggests resetting iou.tag and iou.category. It is wrong

[DylanDylann]

@FitseTLT My proposal has been posted after your proposal 2 minutes but It is correct and I tested it well. I think we should encourage contributors to create a correct proposal rather than a quick proposal with wrong logic and then edit that proposal.

[FitseTLT]

@DylanDylann U did the same thing You first commented transactionsDraft_1 .tag and then updated it to IOU.resetMoneyRequestTag_temporaryForRefactor( so you are doing the same thing but additionally the point of the review is we should reset only when we change different workspace.

With the caveat that we probably don't want to reset tags and category if the user doesn't select a different workspace

[DylanDylann]

I edited my proposal to make it clearer but my original proposal is correct. Reset transactionsDraft_1 .tag is that IOU.resetMoneyRequestTag_temporaryForRefactor function will do

[FitseTLT]

Both alternative solutions I gave U didn't. I still think I proposed the best solution let the other's (reviewers) do their job 👍

[jjcoffee]

@DylanDylann Ah I'd actually missed that resetMoneyRequestCategory_temporaryForRefactor resets the draft value in Onyx. In that case I agree that your proposal is the first to include the correct function call, however it's still lacking in detail, i.e. where should this change should be applied, so I think @FitseTLT's proposal still works for me. cc @blimpich

[DylanDylann]

@jjcoffee

however it's still lacking in detail, i.e. where should this change should be applied,

I said that "when the user re-selects participant by calling".

I think it is enough for the proposal phase. we can implement detail in the PR phase
Note that @FitseTLT's proposal builds on top of my proposal.

cc @blimpich

[FitseTLT]

@jjcoffee Just to bring some attention to my alternative solution: because before the recent refactor of the request flow we used to reset tag and category whenever we navigate to confirmation page. To give u some background we introduced the resetting on this pr (but resetting on mount of the confirmation page) and then we changed the resetting to happen when we navigate to the confirmation page to deal with this issue 👍 So if we don't have a specific reason to change this previous behaviour we might need to stick to it.
cc @blimpich

[blimpich]

@FitseTLT @DylanDylann thank you both for your time and effort spent on this issue. I understand it can be frustrating to not have a proposal chosen, but please remain courteous throughout the proposal and review process. We try our best to be as fair as possible and I'm sure @jjcoffee tried their best to do that in this situation. Practicing inclusivity in our communication is also part of the Code of Conduct, and is a requirement for contributing. Please keep that in mind when commenting.

That being said, because of the similarity in timing and proposals I'm going to go with @FitseTLT's proposal. I agree with @jjcoffee that the level of detail is the deciding factor. @DylanDylann if you would like I can spend time trying to find another issue for you to work on.

[melvin-bot]

📣 @jjcoffee 🎉 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]

📣 @FitseTLT 🎉 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 📖

[jjcoffee]

@FitseTLT I'm not sure I fully understand what you're proposing, but if you're talking about always resetting when navigating to the confirmation page vs. only when the participant is changed, I don't feel too strongly about the latter.

Once you navigate back to the participants page, the selection is reset anyway so it wouldn't be too jarring if tapping the same workspace would reset the tag (it's not quite the "back and forth" style UX I alluded to!). So, I'm fine to go with that if it works better for you!

[melvin-bot]

@alexpensify, @blimpich, @jjcoffee, @FitseTLT Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[jjcoffee]

@FitseTLT Don't forget to let us know when we can expect a PR.

[FitseTLT]

PR ready

[alexpensify]

Awesome, great to see we are making progress here!

[alexpensify]

It looks like we need to wait for automation here.

[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 1.4.15-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Reset category and tag on navigating to confirmation page #33222

If no regressions arise, payment will be issued on 2023-12-29. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @jjcoffee requires payment automatic offer (Reviewer)
• @FitseTLT requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
• [@jjcoffee] 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:
• [@jjcoffee] 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:
• [@jjcoffee] Determine if we should create a regression test for this bug.
• [@jjcoffee] 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.
• [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[alexpensify]

Here is the payment summary:

• External issue reporter - N/A
• Contributor that fixed the issue @FitseTLT $500
• Contributor+ that helped on the issue and/or PR @jjcoffee $500

Upwork Job: https://www.upwork.com/jobs/~0194acf0b0a5cd5e3a

Extra Notes regarding payment: Heads up, I'm OOO until January 4, but I will log in on Friday, December 29 to complete this payment process pending no regression. I've confirmed that @jjcoffee and @FitseTLT are a pending hire, so all set here for the payment date.

@jjcoffee please complete the checklist when you can, thanks!

[alexpensify]

Closing - Thank you everyone for your patience here. I've completed the payment process in Upwork and closed this job.

Happy New Year!

@jjcoffee - please complete the checklist when you get a chance, thanks!

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: Refactor the money request creation flow #28618
• 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: https://github.com/Expensify/App/pull/28618/files#r1439411297
• 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 - the PR was just massive so regressions were expected!
• Determine if we should create a regression test for this bug. Yes - though it may already be covered
• 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.

Regression Test Proposal

1. Click FAB > Request money > Manual (or any type)
2. Select Workspace (that allows categories and tags) as a participant.
3. Now on the confirmation page set the category and tag
4. Navigate back and select any other workspace that allows categories and tags
5. Verify that the tag and categories fields are reset on the confirmation page

Do we agree 👍 or 👎