Move free trial banner to #admins room for users selecting MANAGE_TEAM onboarding intent by c3024 · Pull Request #53895 · Expensify/App

c3024 · 2024-12-11T06:46:17Z

Explanation of Change

This PR adds the logic to display the "Free trial" banner and button in header on #admins room for users selecting "Manage my team's expenses" intent during onboarding.

Fixed Issues

$ #53526
PROPOSAL: NA

Tests

Test 1:

Logon to ND with a new email without a '+' in it.
Select the "Manage my team's expenses" onboarding intent > Next.
Select any option in the number of employees step.
Verify that no "Free trial" banner shows up on Concierge chat on LHN nor in the header of the Concierge chat.

Test 2:

Logon to ND with a new email with a '+' in it.
Select the "Manage my team's expenses" onboarding intent > Next.
Select any option in the number of employees step.
Verify that the "Free trial" banner shows up on Concierge chat on LHN and in the header of the Concierge chat.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Same as Tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

freeAndroid.mov

Android: mWeb Chrome

freeAndroidmWeb.mp4

iOS: Native

freeiOS.mov

iOS: mWeb Safari

freeiOSmWeb.MP4

MacOS: Chrome / Safari

freeChrome1.mp4

freeChrome2.mp4

MacOS: Desktop

freeDesktop.mov

melvin-bot · 2024-12-11T08:21:01Z

@thesahindia 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]

thesahindia · 2024-12-13T16:34:07Z

@c3024, please resolve the conflicts.

…nage-teams-onboarding-intent

c3024 · 2024-12-16T08:11:31Z

Done.

The Jest Unit Tests workflow is failing for all PRs. But, the failing PolicyUtilsTests succeeds when ran locally.

…nage-teams-onboarding-intent

thesahindia · 2024-12-18T12:53:05Z

@c3024, could you please fix the lint issue in this?

…nage-teams-onboarding-intent

thesahindia · 2024-12-19T07:30:10Z

Reviewer Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-12-19.at.12.58.50.PM.mov

Screen.Recording.2024-12-19.at.12.58.07.PM.mov

Android: mWeb Chrome

Screen.Recording.2024-12-19.at.12.57.35.PM.mov

Screen.Recording.2024-12-19.at.12.55.24.PM.mov

iOS: Native

Screen.Recording.2024-12-19.at.12.45.00.PM.mov

Screen.Recording.2024-12-19.at.12.44.07.PM.mov

iOS: mWeb Safari

Screen.Recording.2024-12-19.at.12.46.45.PM.mov

Screen.Recording.2024-12-19.at.12.46.30.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-12-19.at.12.41.53.PM.mov

Screen.Recording.2024-12-19.at.1.21.10.PM.mov

MacOS: Desktop

Screen.Recording.2024-12-19.at.1.20.37.PM.mov

Screen.Recording.2024-12-19.at.12.41.08.PM.mov

…nage-teams-onboarding-intent

c3024 · 2024-12-19T08:01:41Z

@thesahindia , fixed!

thesahindia

~~Works well!~~

There is an issue

src/pages/home/HeaderView.tsx

thesahindia

@c3024, could you please verify it?

…nage-teams-onboarding-intent

thesahindia

Thanks for the changes!

…nage-teams-onboarding-intent

iwiznia · 2025-01-09T17:41:09Z

src/libs/ReportUtils.ts

+    if (isEmptyObject(onboarding)) {
+        return (optionOrReport as OptionData)?.isConciergeChat ?? isConciergeChatReport(optionOrReport);
+    }
+    // For users with emails that do not contain a ‘+’, and who select the MANAGE_TEAM intent, the onboarding tasks are in the #admins room.


Best comments say why, not what. So why is this?

That was an already existing design choice because onboarding guide is added to the #admins room. The 'why' of that is unrelated to this PR. The code seems to be self-explanatory here. I will remove that comment.

What do you mean existing, the logic seems new to me, especially the team and + thing.
I rather leave the comment, but explain why we do what we do

This was the logic. Backend does not assign a guide for signups with emails containing '+' in them and rejects posting tasks in the #admins room. So, tasks are posted in Concierge for these signups.

iwiznia · 2025-01-09T17:49:09Z

src/pages/home/HeaderView.tsx

+    );
    // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
-    const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID || report?.reportID || '-1'}`);
+    const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID || report?.reportID || undefined}`);


Why is this undefined and not CONST.DEFAULT_NUMBER_ID?

CONST.DEFAULT_NUMBER_ID is integer. This is a string. Guide suggests to remove the fallback string. But, removing it crashed as mentioned here if there are some reports saved in Onyx previously in the browser. So, added the undefined fallback to avoid that crash.

Oh right, I always forget reportID is string in JS, then why did we use the default number here?

These changes were lint failures unrelated to the changes required for the PR. So, I just replaced the fallbacks for number and strings as per the style guide. One used -1 and another '-1'. But, I found now that CONST.DEFAULT_NUMBER_ID is used at most places as fallback for reportID in the repo. CONST.DEFAULT_NUMBER_ID can be used here as well.

…nage-teams-onboarding-intent

c3024 · 2025-01-16T02:59:49Z

@iwiznia , updated the comment to explain the 'why'. Please have a look!

iwiznia · 2025-01-16T19:04:24Z

src/components/LHNOptionsList/OptionRowLHN.tsx


    // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
-    const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${optionItem?.reportID || -1}`);
+    const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${optionItem?.reportID || CONST.DEFAULT_NUMBER_ID}`);


Isn't reportID a string?

It is. I didn't change the fallback value from string to Number. The existing fallback was the Number -1. I replaced it with CONST. This Number fallback is used as fallback for reportID at other places too.

I don't know why Number was used there as fallback instead of string.

I think we should use the string fallback for strings, no?

Yes. I think we should. Let me change and hope nothing breaks.🤞

We shouldn't use any fallback for strings as per the guide.

…nage-teams-onboarding-intent

OSBotify · 2025-01-16T22:37:42Z

✋ 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 · 2025-01-17T13:54:32Z

🚀 Deployed to staging by https://github.com/iwiznia in version: 9.0.87-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅
🤖🔄 android HybridApp 🤖🔄	success ✅
🍎🔄 iOS HybridApp 🍎🔄	success ✅

IuliiaHerets · 2025-01-17T15:04:47Z

This PR is failing because of issue #55408

github-actions · 2025-01-21T08:07:36Z

🚀 Deployed to production by https://github.com/Beamanator in version: 9.0.87-3 🚀

platform	result
🤖 android 🤖	true ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅
🤖🔄 android HybridApp 🤖🔄	failure ❌
🍎🔄 iOS HybridApp 🍎🔄	success ✅

c3024 added 2 commits December 11, 2024 12:14

move free trial banner to admins room

254bd0e

free trial logic for header for manage team intent

640d83b

c3024 marked this pull request as ready for review December 11, 2024 08:20

c3024 requested a review from a team as a code owner December 11, 2024 08:20

melvin-bot bot requested review from thesahindia and removed request for a team December 11, 2024 08:20

c3024 added 2 commits December 16, 2024 13:13

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

56ee397

…nage-teams-onboarding-intent

prettier

37c60be

c3024 mentioned this pull request Dec 16, 2024

[HOLD for payment 2025-01-28] [$250] Onboarding - "Start a free trial" banner and button appear in Concierge then disappear #53526

Closed

8 tasks

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

472f435

…nage-teams-onboarding-intent

c3024 added 2 commits December 18, 2024 19:26

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

b187a0e

…nage-teams-onboarding-intent

prettier

51c1483

c3024 added 2 commits December 19, 2024 13:10

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

9134cda

…nage-teams-onboarding-intent

fix lint

8156350

thesahindia approved these changes Dec 19, 2024

View reviewed changes

melvin-bot bot requested a review from iwiznia December 19, 2024 12:10

thesahindia reviewed Dec 19, 2024

View reviewed changes

src/pages/home/HeaderView.tsx Outdated Show resolved Hide resolved

thesahindia suggested changes Dec 19, 2024

View reviewed changes

c3024 added 2 commits December 19, 2024 20:13

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

b546542

…nage-teams-onboarding-intent

fix potential errors due to lint changes

cf4dc60

thesahindia approved these changes Dec 20, 2024

View reviewed changes

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

4d323fc

…nage-teams-onboarding-intent

iwiznia reviewed Jan 9, 2025

View reviewed changes

remove redundant comment

b437baf

c3024 added 7 commits January 13, 2025 18:28

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

60c67c9

…nage-teams-onboarding-intent

make the comment to explain the why

8afaca3

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

eafed51

…nage-teams-onboarding-intent

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

e0f61fc

…nage-teams-onboarding-intent

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

6800e0e

…nage-teams-onboarding-intent

use named reports to fix lint errors

41716d5

use named imports to fix lint errors

116cda6

iwiznia reviewed Jan 16, 2025

View reviewed changes

c3024 added 2 commits January 17, 2025 03:30

Merge branch 'main' into move-free-trial-banner-to-admins-room-for-ma…

4c9d7cf

…nage-teams-onboarding-intent

remove fallback for string reportID

31bd5c9

iwiznia approved these changes Jan 16, 2025

View reviewed changes

iwiznia merged commit 63354a8 into Expensify:main Jan 16, 2025

github-actions bot mentioned this pull request Jan 17, 2025

Deploy Checklist: New Expensify 2025-01-17 #55396

Closed

60 tasks

IuliiaHerets mentioned this pull request Jan 17, 2025

[$250] Onboarding - Free trial banner is displayed on archived admin room #55408

Closed

8 tasks

mvtglobally mentioned this pull request Feb 11, 2025

Deploy Checklist: New Expensify 2025-02-11 #56625

Closed

69 tasks

FitseTLT mentioned this pull request Aug 8, 2025

[Due for payment 2025-09-17] [$250] Onboarding - "Trial: 30 days left" banner shown on #admins and Concierge chat for "Track" user #68235

Closed

8 tasks

Conversation

c3024 commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Dec 11, 2024

Uh oh!

thesahindia commented Dec 13, 2024

Uh oh!

c3024 commented Dec 16, 2024

Uh oh!

thesahindia commented Dec 18, 2024

Uh oh!

thesahindia commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

c3024 commented Dec 19, 2024

Uh oh!

thesahindia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thesahindia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesahindia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c3024 commented Jan 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c3024 Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c3024 Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Jan 16, 2025

Uh oh!

github-actions bot commented Jan 17, 2025

Uh oh!

IuliiaHerets commented Jan 17, 2025

Uh oh!

github-actions bot commented Jan 21, 2025

Uh oh!

c3024 commented Dec 11, 2024 •

edited

Loading

thesahindia commented Dec 19, 2024 •

edited

Loading

thesahindia left a comment •

edited

Loading

thesahindia left a comment •

edited

Loading

c3024 Jan 16, 2025 •

edited

Loading

c3024 Jan 16, 2025 •

edited

Loading