Plan changes to subscriptions #54771
Conversation
… the downgrade and upgrade pages
…eb desktop flex row layout
| getUrlWithBackToParam(`settings/workspaces/${policyID}/upgrade/${encodeURIComponent(featureName ?? '')}` as const, backTo), | ||
| route: 'settings/workspaces/:policyID?/upgrade/:featureName?', | ||
| getRoute: (policyID?: string, featureName?: string, backTo?: string) => | ||
| policyID ? getUrlWithBackToParam(`settings/workspaces/${policyID}/upgrade/${encodeURIComponent(featureName ?? '')}` as const, backTo) : (`settings/workspaces/upgrade` as const), |
There was a problem hiding this comment.
@DylanDylann, I have made the policyID optional for the WORKSPACE_UPGRADE and WORKSPACE_DOWNGRADE routes.
The reasoning behind this is that when navigating to Subscriptions -> and if the user has multiple workspaces as an owner and then presses the 'Upgrade'/'Downgrade' plan type, we navigate them to the Upgrade/Downgrade page and On this page, instead of the 'Upgrade'/'Downgrade' CTA, we show a 'Go to Workspaces' button at the bottom so we are not navigating them to any specific ws page.
If the user navigates to the upgrade/downgrade pages while owning multiple workspaces, the new route URLs will be settings/workspaces/upgrade and settings/workspaces/downgrade.
On both of these pages, I have updated the condition to display the 'Not Found' page, if the route params don't include a policyID, then checks whether the user has more than one workspace as an owner. If so, it shows the page with the 'Go to Workspaces' button; otherwise, it shows the 'Not Found' page.
Please let me know what you think about the outlined plan. Feel free to share any suggestions."
There was a problem hiding this comment.
@jayeshmangwani It looks good to me
|
@DylanDylann When we navigate to "Workspaces" from the "Upgrade" page and upgrade the workspace(s) to the Control Plan, then if we go back twice, we can still see the Upgrade page, even though the workspace(s) are already upgraded. How do you think should we handle this navigation? we can add an navigation-bug.mov |
|
@DylanDylann could you take a look at this comment? Thanks! |
|
Sure, I will do it now |
|
Option 1: If the navigation stack is: Subscription Page --> Upgrade/Downgrade Page Then If we click on "Go to workspace" we should update the navigation stack Subscription Page --> Workspace Page Then if the user clicks the back button, they will be navigated to the subscription page Option 2: Your suggestion here Option 3: Subscription Page --> Upgrade/Downgrade Page --> Workspace List --> Workspace Profile --> Select Plan Type Then If we click on a new type we should update the navigation stack Subscription Page --> |
|
I think we should ask the internal team to make the final decision. We have 3 options #54771 (comment) and I prefer the third one |
|
@jayeshmangwani Updated to add new option |
|
@hungvu193 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@hungvu193, please ignore the auto-assignment for this; @DylanDylann will review this PR |
| const theme = useTheme(); | ||
|
|
||
| return ( | ||
| <View style={[styles.borderedContentCard, styles.flex1, styles.mt5, styles.p5, index === 0 && styles.mr3]}> |
There was a problem hiding this comment.
| <View style={[styles.borderedContentCard, styles.flex1, styles.mt5, styles.p5, index === 0 && styles.mr3]}> | |
| <View style={[styles.borderedContentCard, styles.flex1, styles.mt5, styles.p5, index === 0 && styles.mr3, plan.isSelected && styles.borderColorFocus]}> |
Let's add green border on the selected option
| @@ -51,7 +62,9 @@ function GenericFeaturesView({onUpgrade, buttonDisabled, loading}: GenericFeatur | |||
| </View> | |||
| ))} | |||
| <Text style={[styles.textNormal, styles.textSupporting, styles.mt4]}> | |||
| {translate('workspace.upgrade.commonFeatures.benefits.note')}{' '} | |||
| {translate('workspace.upgrade.commonFeatures.benefits.startsAt')} | |||
| <Text style={[styles.textSupporting, styles.textBold]}>{formattedPrice}</Text> | |||
There was a problem hiding this comment.
Please add a space before Learn more link
| // In this case, check if the user is an admin on more than one policy. | ||
| // If the user is not an admin on multiple policies, they cannot perform the action, | ||
| // so display the NotFoundPage. | ||
| const canPerformUpgrade = (!!policy && PolicyUtils.isPolicyAdmin(policy)) || (!policyID && ownerPolicies.length > 1); |
There was a problem hiding this comment.
I think we should move this to an util function
| @@ -187,18 +197,19 @@ function WorkspaceUpgradePage({route}: WorkspaceUpgradePageProps) { | |||
| } | |||
| }} | |||
| /> | |||
| {isUpgraded && ( | |||
| {!!policyID && !!policy && isUpgraded && ( | |||
There was a problem hiding this comment.
Let's remove one of them
|
@jayeshmangwani Just want to confirm that, we are using option 1 for this problem |
| Navigation.goBack(); | ||
| Navigation.navigate(ROUTES.SETTINGS_WORKSPACES); |
There was a problem hiding this comment.
| Navigation.goBack(); | |
| Navigation.navigate(ROUTES.SETTINGS_WORKSPACES); | |
| Navigation.goBack(); | |
| Navigation.navigate(ROUTES.SETTINGS_WORKSPACES); |
Let's use
Navigation.navigate(ROUTES.SETTINGS_WORKSPACES, CONST.NAVIGATION.TYPE.UP);
to replace the current screen
Yes, I added the Option 1 approach for that problem, and it makes sense too. However, I was using the goBack and navigation methods. I'll push the changes you suggested. Also, this approach still needs to be confirmed with Carlos. |
|
Hi @carlosmiceli, We need your consensus on one navigation expectation. Precondition: The user has more than one workspace as an admin.
Reasoning: This behavior is based on the following comments: #54771 (comment) and #54771 (comment) Please let us know if we need to open a discussion regarding this behavior or if we can proceed with the current navigation flow, where pressing back from the Workspaces screen redirects the user to the Subscription page. |
|
The rest looks fine to me |
|
There is only one problem that needs to be confirmed here. It is so minor so I think it should not be blocked this PR, then we can handle it in a follow-up PR |
agree |
|
@carlosmiceli Kindly bump for the final reviewing. It seems this is the last PR on the Downgrade feature, do we have any other tasks |
|
Sorry, been OOO for the past 7 days, now getting back into everything! 🙏 Looking into it now. |
|
Ok, we can merge this PR, but let's make sure to address this in a Slack thread, can you get that conversation started so we can decide what flow we'd prefer? I'll leave a note about this in the main issue so we don't forget about it, but great work on everything so far! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
I think this flow is fine. The user will have had to select Collect before to trigger the downgrade flow so going back to the Subscriptions page without launching the downgrade flow automatically will work well. |
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.0.84-0 🚀
|
|
@carlosmiceli existing flow does the same thing, so we are good here and don't need to make any additional changes. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.84-7 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.84-7 🚀
|
Explanation of Change
Fixed Issues
$ #52588
PROPOSAL:
Tests
Single Workspace Upgrade
Single Workspace Downgrade (follow Single Workspace Upgrade steps first)
Multi Workspace Upgrade (follow Single Workspace Upgrade + Downgrade steps first)
Multi Workspace Upgrade (follow Multi Workspace Upgrade steps first)
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android.mov
Android: mWeb Chrome
mweb-chrome.mov
iOS: Native
iOS.mov
iOS: mWeb Safari
mweb-safari.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov