-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Comments
Triggered auto assignment to @miljakljajic ( |
Job added to Upwork: https://www.upwork.com/jobs/~0122ad7d632e92e433 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan ( |
I am freelancer from Upwork. I would like to work on this issue |
📣 @hereistopdev! 📣
|
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. |
Ready to help |
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
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.
So this could work. App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 121 to 126 in 0cc5817
|
I think we should hold this until #37213 is merged as not testable yet. |
Will post a formal proposal once #37213 is merged. |
ProposalPlease re-state the problem that we are trying to solve in this issueWe 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 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.
Within the code block mentioned above we need to map all existing workflow / categories RHP routes to 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 VideosMacOS: Chrome / Safari
|
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. |
@bernhardoj please submit your proposal since it has been merged. Thanks |
ProposalPlease 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 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. App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 156 to 160 in a2d0cf2
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,
App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 121 to 126 in a2d0cf2
so it defaults to the report screen. App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 167 to 174 in a2d0cf2
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
|
@bernhardoj's proposal looks good, though @ikevin127 submitted the formal proposal first. |
PR #37620 ready for review! 🚀 |
The solutions are the same
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 😄. |
to which you replied:
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 🤔 |
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. 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. |
|
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:
|
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:
|
Triggered auto assignment to @mallenexpensify ( |
@mallenexpensify 👋 Any blockers from completing the payment here ? |
Contributor: @ikevin127 paid $500 via Upwork @situchan can you complete the BZ checklist above plz? Thx |
Hi Expensify/App,
If you have any sub-project to fix, Plz let me know.
I am always ready for you.
thanks.
…On Fri, Mar 15, 2024 at 6:02 PM Matt Allen ***@***.***> wrote:
Contributor: @ikevin127 <https://github.com/ikevin127> paid $500 via
Upwork
Contributor+: @situchan <https://github.com/situchan> paid $500 via
Upwork.
@situchan <https://github.com/situchan> can you complete the BZ checklist
above
<#37517 (comment)>
plz? Thx
—
Reply to this email directly, view it on GitHub
<#37517 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A7XO7XUVRROX3TV2BZE4FUTYYMSRDAVCNFSM6AAAAABEAEPYX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBQGA3TSNZWGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@hereistopdev plz review CONTRIBUTING.md. We have a process we follow and all work is completed as part of that process. |
Before, I applied with Expensify account, but you told me the project is
taken by other one.
What I want is just let me know when you have new issues to fix.
Thx
…On Fri, Mar 15, 2024 at 6:04 PM Matt Allen ***@***.***> wrote:
@hereistopdev <https://github.com/hereistopdev> plz review CONTRIBUTING.md
<https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md>.
We have a process we follow and all work is completed as part of that
process.
—
Reply to this email directly, view it on GitHub
<#37517 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A7XO7XRNZXAHPARXQR5H7ZLYYMS3BAVCNFSM6AAAAABEAEPYX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBQGA4DIMJSGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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. |
Context
Delayed Submission
and click onMonthly
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
The text was updated successfully, but these errors were encountered: