App
App copied to clipboard
Expensify
[$1000] The transition of opening a new workspace at the creation doesn't match to other pages reported by @Puneet-here
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
- Go to settings > workspaces
- Create a workspace
- Check the transition
Expected Result:
The transition should match with other pages when we navigate from one page to another
Actual Result:
The previous page disappear and new page opens
Workaround:
unknown
Platform:
Where is this issue occurring?
- Web
- iOS
- Android
- Desktop App
- Mobile Web
Version Number: 1.2.21-4 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Expensify/Expensify Issue URL: Issue reported by: @Puneet-here Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666951689786119
https://user-images.githubusercontent.com/43996225/198681345-b835ec79-cada-4aeb-83d5-62e9ac73ca75.mov
https://user-images.githubusercontent.com/43996225/198681377-5963936d-c789-483e-9350-47570d7fff08.mp4
Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@kevinksullivan Whoops! This issue is 2 days overdue. Let's get this updated quick!
@kevinksullivan Huh... This is 4 days overdue. Who can take care of this?
Triggered auto assignment to @youssef-lr (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
Hey @kevinksullivan, just dropping a note as a reminder to keep the pressure on to find a contributor and get this one closed out :)
Is this one external or internal? Let's label it accordingly and decide if it belongs in upwork or with an internal engineer. Either way, gonna add the engineering label so we can move it forward in terms of the process. Let me know if I can help!
Oh sorry @michaelhaxhiu , thanks for the reminder! Hm, this seems ever so slightly off so I could see a case for fixing. Posting to upwork for external, and hired @Puneet-here for reporting.
https://www.upwork.com/jobs/~018725fa3aa6f93d59
Current assignee @kevinksullivan is eligible for the External assigner, not assigning anyone new.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)
Triggered auto assignment to @srikarparsi (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
PROPOSAL:
Cause: The previous page disappear and new page opens because before navigate to new page the modal of previous page has been dismissed by Navigation.dismissModal();
Proposal: Remove the line Navigation.dismissModal(); it look like work normal on model. For the web with current source when we close the new page (new workspace that just created) it go to /settings . When we remove line Navigation.dismissModal(); it go to /settings/workspaces. Which one should be correct path when we close a workspace setting?
Looks like something related to react-navigation may have been mentioned in this issue discussion.
As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.
Feel free to drop a note in #expensify-open-source with any questions.
@tungmt
You are right, this inconsistency is caused by this
Navigation.dismissModal()
However we can't remove it, it was added to fix an issue with transitions to existing workspaces from OldDot
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
So we have to either find a way to fix this without removing Navigation.dismissModal() or to find a way to resolve https://github.com/Expensify/App/pull/10949 without using Navigation.dismissModal()
@eVoloshchak, @youssef-lr, @kevinksullivan, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!
Proposal
Add a new param transitionFromOldDot at createWorkspace function
https://github.com/Expensify/App/blob/ecafa3cf9538b89b21db4ef22290a6c0737b53c2/src/libs/actions/Policy.js#L701
Add an if statement here https://github.com/Expensify/App/blob/ecafa3cf9538b89b21db4ef22290a6c0737b53c2/src/libs/actions/Policy.js#L866-L868
if (transitionFromOldDot) {
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
}
Pass true here as an argument in the end https://github.com/Expensify/App/blob/ecafa3cf9538b89b21db4ef22290a6c0737b53c2/src/libs/actions/App.js#L222
@eVoloshchak, I am sure it will work but I wasn't able to test this locally can you help me test it?
@Puneet-here yeah I think it should work, pass isLoggingInAsNewUser to createWorkspace function
@Puneet-here, cool, I like your solution!
It indeed fixes this issue
And it also works correctly when transitioning from OldDot to NewDot, however, I verified this by calling setUpPoliciesAndNavigate manually with mock data, which is not sufficient.
There is no Get started with a free plan option for me on OldDot, the one that can be seen in the description of https://github.com/Expensify/App/pull/10949
I'll investigate this and will your proposal locally, expect an answer in 24h
Got a response and was able to test, @Puneet-here's proposal looks good!
🎀👀🎀 C+ reviewed! cc: @kevinksullivan
this looks good to me as well. Assigning you to the issue @Puneet-here.
📣 @Puneet-here You have been assigned to this job by @srikarparsi! Please apply to this job in Upwork 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 📖
@eVoloshchak, @kevinksullivan, @srikarparsi, @Puneet-here Whoops! This issue is 2 days overdue. Let's get this updated quick!
Hm looks like this one just missed the notification to label properly. Waiting out the 7 day period still.
Can you add [HOLD awaiting payment [Date]] on this issue? Just came across it trying to clear out bugs that need more help.
