-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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-09-19][$250] Workspace upgrade - View your subscription links in upgrade success RHP opens Workspaces page #47683
Comments
Triggered auto assignment to @kevinksullivan ( |
We think that this bug might be related to #wave-control |
@kevinksullivan FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
ProposalPlease re-state the problem that we are trying to solve in this issue.Workspace upgrade - View your subscription links in upgrade success RHP opens Workspaces page What is the root cause of that problem?When the page is being removed the useEffect below is called and the app is navigate to a different route according to the logic below. App/src/pages/workspace/upgrade/WorkspaceUpgradePage.tsx Lines 42 to 64 in 0e4f02e
What changes do you think we should make in order to solve the problem?I believe we should only call onConfirmUpgrade={() => {
Navigation.dismissModal();
confirmUpgrade();
}} What alternative solutions did you explore? (Optional)Instead of
Result |
ProposalPlease re-state the problem that we are trying to solve in this issue.Access your subscription links after successful upgrade by opening the Workspaces page in RHP. What is the root cause of that problem?When blur, the confirm process executed with navigate. App/src/pages/workspace/upgrade/WorkspaceUpgradePage.tsx Lines 56 to 60 in 0e4f02e
What changes do you think we should make in order to solve the problem?Add const confirmUpgrade = useCallback((shouldNavigate : boolean = false) => {
console.log('[wildebug] confirmUpgrade called with shouldNavigate:', shouldNavigate);
if (!feature) {
console.log('[wildebug] feature is not defined');
return;
}
switch (feature.id) {
case CONST.UPGRADE_FEATURE_INTRO_MAPPING.reportFields.id:
Policy.enablePolicyReportFields(policyID, true, true);
if (shouldNavigate) {
console.log('[wildebug] Navigating to:', ROUTES.WORKSPACE_MORE_FEATURES.getRoute(policyID));
return Navigation.navigate(ROUTES.WORKSPACE_MORE_FEATURES.getRoute(policyID));
}
break;
default:
if (shouldNavigate) {
if (route.params.backTo) {
console.log('[wildebug] Navigating to backTo route:', route.params.backTo);
return Navigation.navigate(route.params.backTo);
} else {
console.log('[wildebug] Navigating back');
return Navigation.goBack();
}
}
break;
}
}, [feature, policyID, route.params.backTo]);
...
{isUpgraded && (
<UpgradeConfirmation
onConfirmUpgrade={() => {
confirmUpgrade(true);
Navigation.dismissModal()}}
policyName={policy.name}
/>
)} What alternative solutions did you explore? (Optional)N/A |
Proposal Updated
|
ProposalPlease re-state the problem that we are trying to solve in this issue.App opens subcription page, then quickly opens Workspaces. What is the root cause of that problem?When users click view your subscription, the current page will be dismiss then we have the logic to navigate to WS
What changes do you think we should make in order to solve the problem?We should not trigger navigation in confirmUpgrade
For backTo, we should handle it in What alternative solutions did you explore? (Optional)NA |
@kevinksullivan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Job added to Upwork: https://www.upwork.com/jobs/~01dbc6c489313735ea |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah ( |
@rayane-djouah What do you think about my proposal |
Will review the proposals today |
@kevinksullivan, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Sorry for the delay! Reviewing 👀 |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Thank you, everyone, for your proposals. I agree that the root cause is that when the "upgrade" page is closed, we navigate to the Regarding the proposed solutions:
We need to call the
This will navigate to subscription page without calling
This approach will navigate to the subscription page without calling the I agree with @dominictb that we should not trigger navigation within the @dominictb's proposal looks good to me. |
@rayane-djouah can you add 🎀 👀 🎀 C+ reviewed to assign another engineer, then ping them in a comment to ask for 👀 on the PR? Thx. (also.. we need to reassign Bondy too after the new engineer's been added) |
🎀 👀 🎀 |
Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
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. |
We've fixed this ^ in a follow-up PR. Slack thread link, PR: #49076 |
Waiting on the regression PR to be deployed for 7 days. Looping in another BZ for payment as I'm going OOO |
Triggered auto assignment to @johncschuster ( |
Note The production deploy automation failed: This should be on [HOLD for Payment 2024-09-19] according to #49025 production deploy checklist, confirmed in #48673 (comment) |
I'll issue payment as soon as the regression window has passed 👍 |
Payment has been issued! 🎉 @rayane-djouah / @dominictb can you please provide a regression test step list if one is required? |
@johncschuster can you plz post payment summaries in the format in this SO? For auditing purposes we need to know who was paid, how much, and via Upwork/NewDot. @rayane-djouah is the C+ who needs to provide the regression test steps. Thx |
Sure thing! |
Payment Summary: Contributor: @dominictb paid $250 via Upwork Upwork job here! |
@bondydaa, @johncschuster, @kevinksullivan, @thienlnam, @rayane-djouah, @dominictb Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Checklist
Regression Test Proposal
Do we agree 👍 or 👎 |
Thanks, @rayane-djouah! I've created a QA issue and have paid this one out. Closing! |
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.22-5
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh05081@applause.expensifail.com
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
App will open subscription page.
Actual Result:
App opens subcription page, then quickly opens Workspaces.
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6576765_1724132976353.bandicam_2024-08-20_13-47-37-976.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @dominictbThe text was updated successfully, but these errors were encountered: