fix: redirect to proper place after upgrade#46617
Merged
youssef-lr merged 11 commits intoExpensify:mainfrom Aug 12, 2024
allroundexperts:fix-45689-2
Merged
fix: redirect to proper place after upgrade#46617youssef-lr merged 11 commits intoExpensify:mainfrom allroundexperts:fix-45689-2
youssef-lr merged 11 commits intoExpensify:mainfrom
allroundexperts:fix-45689-2
Conversation
fix: redirect to proper place after upgrade
|
@hoangzinh 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] |
allroundexperts
changed the title
Contributor
|
#45730 - this was the previous PR, I'm going to ask @rushatgabhane to review it as it was a revert. |
trjExpensify
requested review from
rushatgabhane
and removed request for
hoangzinh
Member
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-08-02.at.07.27.20.movAndroid: mWeb ChromeiOS: NativeScreen.Recording.2024-08-09.at.06.54.03.moviOS: mWeb SafariScreen.Recording.2024-08-02.at.07.29.36.movMacOS: Chrome / SafariScreen.Recording.2024-08-09.at.06.25.44.movMacOS: DesktopScreen.Recording.2024-08-09.at.06.27.30.mov |
Member
|
Bug: Report fields is not enabled after clicking back from upgrade screen
Expected: report fields is enabled. Screen.Recording.2024-08-02.at.07.32.05.mov |
Contributor
|
Weird. Do you get a different result if you click `Got it, thanks"? |
Member
|
@trjExpensify yep, clicking got it thanks shows the report fields as enabled |
Contributor
|
Okay, nice catch. Strange.. we should upgrade the workspace and enable the feature when they click "Upgrade" without having to click the "Got it" button to close the modal after its done. |
Contributor
Author
Fixed @rushatgabhane |
Member
|
reviewing. @allroundexperts meanwhile could you please fix the conflicts |
Contributor
Author
|
@rushatgabhane Resolved. |
rushatgabhane
approved these changes
Aug 9, 2024
youssef-lr
reviewed
Aug 12, 2024
|
|
||
| const upgradeToCorporate = () => { | ||
| Policy.upgradeToCorporate(policy.id, feature.id); | ||
| if (!policy || !feature) { |
Contributor
There was a problem hiding this comment.
Do we not have a way in the UI to upgrade without having to click on a feature first?
Contributor
Author
There was a problem hiding this comment.
I did this just to keep typescript happy since feature or policy could be null if the user opens the upgrade page via a deep link with incorrect params.
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Contributor
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
Contributor
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Contributor
|
Note: This PR probably hasn't introduced the recession. The regression has been introduced at an earlier point, and the CI system will report any PR that follows this faulty PR as a regression. |
6 tasks
72 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.0.20-0 🚀
|
Closed
6 tasks
Closed
6 tasks
Contributor
|
@youssef-lr can you please take a look into #47399? Looks that it's related to this PR |
Contributor
|
FYI I believe this was deployed to prod yesterday, from this checklist - #47356 |
DylanDylann
reviewed
Aug 21, 2024
| if (!isUpgraded) { | ||
| return; | ||
| } | ||
| confirmUpgrade(); |
Contributor
There was a problem hiding this comment.
@allroundexperts Hi, why do we need to listen blur event here? Could we call confirmUpgrade() right after licking "Got it, thanks" on onConfirmUpgrade callback?
Contributor
Author
There was a problem hiding this comment.
We need to call the confirmation function if the user doesn't event click on the Got it, thanks! button and just closes the RHP by clicking elsewhere on the screen.
50 tasks
paultsimura
reviewed
Sep 7, 2024
Comment on lines
+66
to
+68
| if (!feature || !policy) { | ||
| return <NotFoundPage />; | ||
| } |
Contributor
There was a problem hiding this comment.
Coming from #47904
To handle a case when a regular user with no "upgrade" permissions visits this page, we should have also added a check for the user to be an admin here and in some other places.
6 tasks
| Category.setPolicyCategoryGLCode(route.params.policyID, categoryName, newGLCode); | ||
| } | ||
| Navigation.goBack(); | ||
| Navigation.dismissModal(); |
Contributor
There was a problem hiding this comment.
I suppose this PR related with this issue
We upgraded navigation event for editGLCode
But not for onBackButtonPress
6 tasks
rayane-d
reviewed
Sep 20, 2024
| switch (feature.id) { | ||
| case CONST.UPGRADE_FEATURE_INTRO_MAPPING.reportFields.id: | ||
| Policy.enablePolicyReportFields(policyID, true, true); | ||
| return Navigation.navigate(ROUTES.WORKSPACE_MORE_FEATURES.getRoute(policyID)); |
Contributor
There was a problem hiding this comment.
FYI, This caused this issue: #47683. More info in this proposal: #47683 (comment)
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
This PR fixes a number of issues related to the upgrade flow.
Fixed Issues
$ #45692
$ #45689
$ #45682
$ #45884
PROPOSAL: N/A
Tests
Test 1
Test 2
Test 3
Test 4
Offline tests
N/A
QA Steps
Test 1
Test 2
Test 3
Test 4
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
Screen.Recording.2024-08-01.at.3.07.24.AM.mov
Android: mWeb Chrome
Screen.Recording.2024-08-01.at.3.05.47.AM.mov
iOS: Native
Screen.Recording.2024-08-01.at.3.04.14.AM.mov
iOS: mWeb Safari
Screen.Recording.2024-08-01.at.3.01.41.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-08-01.at.2.53.58.AM.mov
MacOS: Desktop
Screen.Recording.2024-08-01.at.3.00.56.AM.mov