Skip to content
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-03-13] [$500] [Nav][Simplified Collect][Workflows] Delayed Submission/Monthly Frequency - Fix redirection screen on reload #37517

Closed
lakchote opened this issue Feb 29, 2024 · 38 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lakchote
Copy link
Contributor

lakchote commented Feb 29, 2024

Context

  1. Create a Collect policy in NewDot
  2. Go to Workspace's settings > Workflows
  3. Toggle the Delayed Submission and click on Monthly
  4. A RHP should open, refresh the page

Expected result
You should be redirected back to the workflows screen

Actual Result
You are redirected to the #admins room

See video:
https://github.com/Expensify/App/assets/29673073/46449156-88c9-476b-b962-2bd73ede5a26

Task
Your goal is to fix the navigation issue.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0122ad7d632e92e433
  • Upwork Job ID: 1763223792123248640
  • Last Price Increase: 2024-02-29
  • Automatic offers:
    • situchan | Reviewer | 0
    • ikevin127 | Contributor | 0
@lakchote lakchote added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 29, 2024
Copy link

melvin-bot bot commented Feb 29, 2024

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

@lakchote lakchote added Help Wanted Apply this label when an issue is open to proposals by contributors External Added to denote the issue can be worked on by a contributor labels Feb 29, 2024
Copy link

melvin-bot bot commented Feb 29, 2024

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

@melvin-bot melvin-bot bot changed the title [Nav][Simplified Collect][Workflows - Delayed Submission/Monthly Frequency] Fix redirection screen on reload [$500] [Nav][Simplified Collect][Workflows - Delayed Submission/Monthly Frequency] Fix redirection screen on reload Feb 29, 2024
Copy link

melvin-bot bot commented Feb 29, 2024

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

@hereistopdev
Copy link

I am freelancer from Upwork. I would like to work on this issue

Copy link

melvin-bot bot commented Feb 29, 2024

📣 @hereistopdev! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

Copy link

melvin-bot bot commented Feb 29, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@ghost
Copy link

ghost commented Feb 29, 2024

Ready to help

@camusfsx
Copy link

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~0189b8f0baa61b559e?viewMode=1

Copy link

melvin-bot bot commented Feb 29, 2024

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@bernhardoj
Copy link
Contributor

Looks like the workflow RHP page is not available yet, but to solve the issue, we should add the central to RHP mapping for workflow here.

const CENTRAL_PANE_TO_RHP_MAPPING: Partial<Record<CentralPaneName, string[]>> = {
[SCREENS.WORKSPACE.PROFILE]: [SCREENS.WORKSPACE.NAME, SCREENS.WORKSPACE.CURRENCY, SCREENS.WORKSPACE.DESCRIPTION, SCREENS.WORKSPACE.SHARE],
[SCREENS.WORKSPACE.REIMBURSE]: [SCREENS.WORKSPACE.RATE_AND_UNIT, SCREENS.WORKSPACE.RATE_AND_UNIT_RATE, SCREENS.WORKSPACE.RATE_AND_UNIT_UNIT],
[SCREENS.WORKSPACE.MEMBERS]: [SCREENS.WORKSPACE.INVITE, SCREENS.WORKSPACE.INVITE_MESSAGE],
};

So this could work.

// Check for CentralPaneNavigator
for (const [centralPaneName, RHPNames] of Object.entries(CENTRAL_PANE_TO_RHP_MAPPING)) {
if (RHPNames.includes(route.name)) {
return createCentralPaneNavigator({name: centralPaneName as CentralPaneName, params: route.params});
}
}

@situchan
Copy link
Contributor

I think we should hold this until #37213 is merged as not testable yet.
@bernhardoj please submit formal proposal including the case of Categories page (#37505 was closed as same root cause)

@bernhardoj
Copy link
Contributor

Will post a formal proposal once #37213 is merged.

@ikevin127
Copy link
Contributor

ikevin127 commented Feb 29, 2024

Proposal

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

We are not navigated back to the workflows screen.

What is the root cause of that problem?

We're not mapping the workflow / categories RHP routes to the [SCREENS.WORKSPACE.WORKFLOWS] and [SCREENS.WORKSPACE.CATEGORIES] central pane here. This causes the central pane to reset, as noted here -> hence this issue was created.

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

I checked out #37213 and this way I was able to reproduce and apply solution.

const CENTRAL_PANE_TO_RHP_MAPPING: Partial<Record<CentralPaneName, string[]>> = {
[SCREENS.WORKSPACE.PROFILE]: [SCREENS.WORKSPACE.NAME, SCREENS.WORKSPACE.CURRENCY, SCREENS.WORKSPACE.DESCRIPTION, SCREENS.WORKSPACE.SHARE],
[SCREENS.WORKSPACE.REIMBURSE]: [SCREENS.WORKSPACE.RATE_AND_UNIT, SCREENS.WORKSPACE.RATE_AND_UNIT_RATE, SCREENS.WORKSPACE.RATE_AND_UNIT_UNIT],
[SCREENS.WORKSPACE.MEMBERS]: [SCREENS.WORKSPACE.INVITE, SCREENS.WORKSPACE.INVITE_MESSAGE],
};

Within the code block mentioned above we need to map all existing workflow / categories RHP routes to [SCREENS.WORKSPACE.WORKFLOWS] and [SCREENS.WORKSPACE.CATEGORIES] like so:

const CENTRAL_PANE_TO_RHP_MAPPING: Partial<Record<CentralPaneName, string[]>> = {
    /// ...existing mappings
    [SCREENS.WORKSPACE.WORKFLOWS]: [SCREENS.WORKSPACE.WORKFLOWS_AUTO_REPORTING_FREQUENCY, SCREENS.WORKSPACE.WORKFLOWS_AUTO_REPORTING_MONTHLY_OFFSET],
    [SCREENS.WORKSPACE.CATEGORIES]: [SCREENS.WORKSPACE.CATEGORY_SETTINGS, SCREENS.WORKSPACE.CATEGORIES_SETTINGS],
};

Note

So far we only have RHP routes for WORKFLOWS_AUTO_REPORTING_FREQUENCY and WORKFLOWS_AUTO_REPORTING_MONTHLY_OFFSET, but once the PR is merged I'm assuming there might be other RHP routes -> so we need to add all of them there, same for the CATEGORIES map.

Videos

MacOS: Chrome / Safari
Before After
before.mov
after.mov

Copy link

melvin-bot bot commented Feb 29, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@lakchote
Copy link
Contributor Author

lakchote commented Mar 1, 2024

Will post a formal proposal once #37213 is merged.

@bernhardoj please submit your proposal since it has been merged. Thanks

@bernhardoj
Copy link
Contributor

Proposal

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

Refreshing when the submission frequency or workspace category settings show a report screen behind it instead of the workspace-related page.

What is the root cause of that problem?

When we refresh the web, the previous nav stack is lost. To show the correct/related page behind the RHP, we use getAdaptedStateFromPath to customize the nav state based on the current path.

If the state contains an RHP, it will make sure a bottom tab navigator, a central pane, and the RHP itself are included on the nav stack/routes.

if (rhpNavigator) {
// Routes
// - matching bottom tab
// - matching root route for rhp
// - found rhp

The bottom tab navigator will be either an LHN (home), all settings, or a workspace page. We already have the bottom tab to central pane mapping here.

In this issue, the bottom tab navigator is the LHN (home) because the central pane is a report screen.

The central pane is a report screen because the mapping for the workspace workflow central pane with the submission frequency RHP is not available,

const CENTRAL_PANE_TO_RHP_MAPPING: Partial<Record<CentralPaneName, string[]>> = {
[SCREENS.WORKSPACE.PROFILE]: [SCREENS.WORKSPACE.NAME, SCREENS.WORKSPACE.CURRENCY, SCREENS.WORKSPACE.DESCRIPTION, SCREENS.WORKSPACE.SHARE],
[SCREENS.WORKSPACE.REIMBURSE]: [SCREENS.WORKSPACE.RATE_AND_UNIT, SCREENS.WORKSPACE.RATE_AND_UNIT_RATE, SCREENS.WORKSPACE.RATE_AND_UNIT_UNIT],
[SCREENS.WORKSPACE.MEMBERS]: [SCREENS.WORKSPACE.INVITE, SCREENS.WORKSPACE.INVITE_MESSAGE],
};

// Check for CentralPaneNavigator
for (const [centralPaneName, RHPNames] of Object.entries(CENTRAL_PANE_TO_RHP_MAPPING)) {
if (RHPNames.includes(route.name)) {
return createCentralPaneNavigator({name: centralPaneName as CentralPaneName, params: route.params});
}
}

so it defaults to the report screen.

let matchingRootRoute = getMatchingRootRouteForRHPRoute(topmostNestedRHPRoute);
// This may happen if this RHP doens't have a route that should be under the overlay defined.
if (!matchingRootRoute) {
metainfo.isCentralPaneAndBottomTabMandatory = false;
metainfo.isFullScreenNavigatorMandatory = false;
matchingRootRoute = createCentralPaneNavigator({name: SCREENS.REPORT});
}

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

Add the central pane to RHP mapping for workspace workflow and workspace category

[SCREENS.WORKSPACE.WORKFLOWS]: [SCREENS.WORKSPACE.WORKFLOWS_AUTO_REPORTING_FREQUENCY, SCREENS.WORKSPACE.WORKFLOWS_AUTO_REPORTING_MONTHLY_OFFSET],
[SCREENS.WORKSPACE.CATEGORIES]: [SCREENS.WORKSPACE.CATEGORIES_SETTINGS],

@situchan
Copy link
Contributor

situchan commented Mar 1, 2024

@bernhardoj's proposal looks good, though @ikevin127 submitted the formal proposal first.
@lakchote (or autoassigned engineer) will give the final call.
🎀 👀 🎀 C+ reviewed

@melvin-bot melvin-bot bot added the Weekly KSv2 label Mar 1, 2024
@ikevin127
Copy link
Contributor

PR #37620 ready for review! 🚀

@bernhardoj
Copy link
Contributor

Oh wait, no, they are not the same in the OG post, are they?

The solutions are the same

What are we going to do about this though ?

Should I start posting the same kind of comments on issues and expect to be assigned based on whether or not I was first to post a comment (not a formal proposal) ? 👀

The page that we are trying to fix isn't even available yet at that time, so it's not a "formal" issue either. I'm not even trying to speedrun a solution as the issue has been open for an hour, yet no one has given any solution yet. An hour later, you took my solution and formatted it into a formal proposal 😄.

@ikevin127
Copy link
Contributor

@bernhardoj please submit formal proposal including the case of Categories page (#37505 was closed as same root cause)

to which you replied:

Will post a formal proposal once #37213 is merged.

I put in the effort to write a formal proposal with a clear RCA / solution and videos before the PR was merged, as to me that wasn't really a blocker. I simply followed the Contributor guidelines mentioned here at (5).

I don't see a problem here, you had a choice and you chose not to do it 🤔

@bernhardoj
Copy link
Contributor

That's not really an effort if you just copied someone solution. I wouldn't say the RCA is clear as it's just a reverse from the solution.
RCA: we don't map here
Solution: we map here
without even explaining why we should do the mapping and why the report screen is shown.

I must admit it's my fault to not do it earlier as I never expected someone would take the chance to convert my comment into a proposal.

@trjExpensify trjExpensify changed the title [$500] [Nav][Simplified Collect][Workflows - Delayed Submission/Monthly Frequency] Fix redirection screen on reload [$500] [Nav][Simplified Collect][Workflows] Delayed Submission/Monthly Frequency - Fix redirection screen on reload Mar 6, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 6, 2024
@melvin-bot melvin-bot bot changed the title [$500] [Nav][Simplified Collect][Workflows] Delayed Submission/Monthly Frequency - Fix redirection screen on reload [HOLD for payment 2024-03-13] [$500] [Nav][Simplified Collect][Workflows] Delayed Submission/Monthly Frequency - Fix redirection screen on reload Mar 6, 2024
Copy link

melvin-bot bot commented Mar 6, 2024

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 6, 2024
Copy link

melvin-bot bot commented Mar 6, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.47-10 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-03-13. 🎊

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

Copy link

melvin-bot bot commented Mar 6, 2024

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:

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 13, 2024
@mountiny mountiny added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Mar 14, 2024
Copy link

melvin-bot bot commented Mar 14, 2024

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

@ikevin127
Copy link
Contributor

@mallenexpensify 👋 Any blockers from completing the payment here ?

@mallenexpensify
Copy link
Contributor

Contributor: @ikevin127 paid $500 via Upwork
Contributor+: @situchan paid $500 via Upwork.

@situchan can you complete the BZ checklist above plz? Thx

@hereistopdev
Copy link

hereistopdev commented Mar 15, 2024 via email

@mallenexpensify
Copy link
Contributor

@hereistopdev plz review CONTRIBUTING.md. We have a process we follow and all work is completed as part of that process.

@hereistopdev
Copy link

hereistopdev commented Mar 15, 2024 via email

@situchan
Copy link
Contributor

There's no offending PR. This is follow-up of new feature and original PR author/reviewer was already aware of this during PR review.
I think this is still WIP and full regression test will be added after simplified collect / workflows is completely done.
That being said, we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests

10 participants