Repository navigation
[No QA] fix: simplify busy onboarding view - #102276
Conversation
|
@mkhutornyi 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] |
| * @description 'rhpHomePage' - Navigate to Home page with Concierge Anywhere accessible in #admins room | ||
| * @description 'trackExpensesWithConcierge' - Track workspace admins land on Home page with Concierge side panel (desktop) or Concierge chat (mobile) | ||
| * @description 'inboxAdminsBespoke' - Bespoke welcome message delivered in #admins room instead of onboarding tasks | ||
| * @description 'homePageNoRHP' - Land on Home with the side panel closed, at every company size; opening Concierge shows the Concierge DM |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
The new @description line joins two independent clauses with a semicolon (...at every company size; opening Concierge shows the Concierge DM). CONSISTENCY-15 asks that comments read as plain, natural sentences and avoid semicolons in place of two separate sentences.
Split it into two sentences:
* @description 'homePageNoRHP' - Land on Home with the side panel closed, at every company size. Opening Concierge shows the Concierge DMReviewed at: 95262ef | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
🚧 trjExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
| // The homePageNoRHP arm of the onboarding experiment lands on Home with the side panel closed, at every company size, | ||
| // so it is handled before the variants that are limited to micro companies or that open the side panel. | ||
| if (variant === CONST.ONBOARDING_RHP_VARIANT.HOME_PAGE_NO_RHP) { | ||
| SidePanelActions.dismissSidePanel(); |
There was a problem hiding this comment.
SidePanelActions.dismissSidePanel();
What's this line for?
There was a problem hiding this comment.
Good catch, it isn't needed. For a new account nvp_sidePanel isn't set when onboarding finishes and nothing in the onboarding flow opens it, so the panel is already closed. I removed the line and changed the test to check that the side panel isn't opened.
|
@neil-marcellini how can this be QA'd or tested properly without the BE? 🤔
|
|
I think this is not testable in staging. App PR should be deployed first |
@annaweber830 this step seems wrong. Does it open full screen? |
Code reviewThe change looks correct and matches the issue. I found no blocking problems. CI tests, lint, and typecheck all pass. The new I have three non-blocking notes:
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb Safariios.mp4MacOS: Chrome / Safariweb.mov |
Regression checkI found no regressions for accounts that exist today. The new code only runs when the variant is One future behavior change is worth confirming. It only matters once the backend assigns the new value:
I also ran onboarding on web ("Manage my team's expenses", 10-50 employees), and the results are in the next comment. That run can't tell the new branch from the old one. The test email contains |
Mhm, why App first? Also, if it's not testable, then we need to mark it as |
Yes, I think the app does need to go out first because when we change the backend, we are going to remove the task list from the admins room and send the new variants, including this homepage no RHP variant. If we send that from the backend before the frontend is ready, then it will fall through to the control variant default with no task list. It would also change what the variant experiment is tracking and the homepageNoRHP variant would be measuring the control variant for some time. I suppose it could technically happen, even if the frontend goes out first, if a user is on an old app version, but that's very unlikely, considering that this is an onboarding flow. I'll mark it No QA. |
neil-marcellini
left a comment
There was a problem hiding this comment.
Looks great, thank you!
|
The PR author checklist was failing, but they clearly tried to fill it out, so I went ahead and merged. |
|
🚧 neil-marcellini has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.5.0-0 🚀
|
|
No help site update is needed: this PR adds a new onboarding experiment variant ( |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.0-5 🚀
Bundle Size Analysis (Sentry): |


Explanation of Change
Adds the
homePageNoRHPonboarding variant toCONST.ONBOARDING_RHP_VARIANTand its type, which is also what letsextractRHPVariantFromResponseaccept it from the server. When an account has it,navigateAfterOnboardingcloses the side panel and sends the user to Home at every company size, before the micro-company RHP checks run. If a report is already open it stays, same as therhpHomePagepath. Opening Concierge from Home already shows the Concierge DM for this value, becauseSidePanelContextProvideronly picks #admins forrhpAdminsRoomandrhpHomePage, so I added a test for it instead of changing code. The other variants and the Submit workspace path are untouched, and unit tests cover the Home landing for micro, small and large companies. The backend doesn't assignhomePageNoRHPyet (https://github.com/Expensify/Expensify/issues/679256), so nothing changes in production until it does.Fixed Issues
$ #100238
PROPOSAL: #100238 (comment)
Tests
Offline tests
Same as tests
QA Steps
No QA. We should QA it with the backend change that will come later.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
issue-100238-android-native.mp4
Android: mWeb Chrome
issue-100238-android-mweb-chrome.mp4
iOS: Native
issue-100238-ios-native-manual.mp4
iOS: mWeb Safari
issue-100238-ios-mweb-safari-manual.mp4
MacOS: Chrome / Safari
issue-100238-macos-chrome.mp4