-
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
[Free trial] Implement and show Trial Ended banner, Expensify DM GBR and custom Expensify DM chat item in the App after Free Trial ends #44483
[Free trial] Implement and show Trial Ended banner, Expensify DM GBR and custom Expensify DM chat item in the App after Free Trial ends #44483
Conversation
…al-banner-and-badges-after-free-trial-starts' into pac-guerreiro/feature/43671-trial-ended-banner-expensify-dm-gbr-and-custom-chat-item
…al-banner-and-badges-after-free-trial-starts' into pac-guerreiro/feature/43671-trial-ended-banner-expensify-dm-gbr-and-custom-chat-item
…al-banner-and-badges-after-free-trial-starts' into pac-guerreiro/feature/43671-trial-ended-banner-expensify-dm-gbr-and-custom-chat-item
…al-banner-and-badges-after-free-trial-starts' into pac-guerreiro/feature/43671-trial-ended-banner-expensify-dm-gbr-and-custom-chat-item # Conflicts: # src/libs/ReportActionsUtils.ts
…al-banner-and-badges-after-free-trial-starts' into pac-guerreiro/feature/43671-trial-ended-banner-expensify-dm-gbr-and-custom-chat-item # Conflicts: # src/pages/settings/Subscription/CardSection/CardSection.tsx
…er-expensify-dm-gbr-and-custom-chat-item
Looking good to me. Just wanted to confirm with @Expensify/design and @MitchExpensify - we decided not to put any kind of GBR on the bottom Settings tab, right? |
From the doc, it doesn't look like it. When the free trial ends a message will be sent in the Conci/onboarding DM with a CTA green button to add a payment card, so a GBR is added to the chat. I think the Inbox bottom tab will get GBR'd "for free" because of that with the existing logic for GBR'd chats. The issue for adding that message hasn't be completed yet: #43694 |
Yeah like Tom said, I think we decided to only use the one from Inbox. |
Okay cool, I wasn't sure where we landed with that. I don't think we need to change anything here but it does seem like a strange detour to GBR the Inbox when the action that needs to be taken happens in Settings... but I'll rest my case there for now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pac-guerreiro Thanks for the PR.
I have left one comment otherwise the code looks good. Additionally, I also have following observations:
- The subtitle text for
Concierge
Chat Report in LHN does not reflect the text from actionable add payment. The same problem occurs forExpensify
Chat Report. - The GBR shows up for
Expensify
Chat Report but does not show forConcierge
Chat Report in LHN fortrial ended
case.
Please have a look on these as well.
* Checks if a given report action corresponds to a add payment card action. | ||
* @param reportAction | ||
*/ | ||
function isActionableAddPaymentCard(reportAction: OnyxEntry<ReportAction>): reportAction is ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.ACTIONABLE_ADD_PAYMENT_CARD> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the return type ReportAction
here? Shouldn't it be boolean
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're applying a type predicate - https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
Not only does this function return a boolean, it also tells the compiler that the argument we passed to it is a specific subtype of ReportAction
, which allows us developers to easily access it's properties without the need to do manual assertions.
I hope this answers your requestion 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. That's cool. Thanks.
@shawnborton Is the gap more than required between the lines |
…er-expensify-dm-gbr-and-custom-chat-item
@pac-guerreiro Also, please add test steps in the PR description. The prerequisites for the tests are very detailed though. Thanks. |
There was an additional |
…er-expensify-dm-gbr-and-custom-chat-item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, just these comments seem to be pending.
…er-expensify-dm-gbr-and-custom-chat-item # Conflicts: # src/components/Icon/Illustrations.ts
Points 1&2 are moot, we don't need to do anything there now as discussed in the thread. @pac-guerreiro did you address point 3?
|
Point 3 has been addressed. There needs to be some improvements though. I'll post on that shortly. For now there are some merge conflicts to address. |
Once you address the conflict feel free to ask on slack for someone else to merge so that it can happen sooner and hence lesser chances of another merge conflict. Additionally, please make the following changes to your QA steps:
|
…er-expensify-dm-gbr-and-custom-chat-item # Conflicts: # src/libs/OptionsListUtils.ts
@chiragsalian thank you for the help with the QA steps! I applied your suggestions, but let me know if I applied them correctly! Also, I resolved conflicts 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with one comment @pac-guerreiro
…er-expensify-dm-gbr-and-custom-chat-item
…er-expensify-dm-gbr-and-custom-chat-item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/chiragsalian in version: 9.0.7-3 🚀
|
🚀 Cherry-picked to staging by https://github.com/thienlnam in version: 9.0.7-4 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀
|
function isChatUsedForOnboarding(optionOrReport: OnyxEntry<Report> | OptionData): boolean { | ||
return AccountUtils.isAccountIDOddNumber(currentUserAccountID ?? -1) | ||
? isSystemChat(optionOrReport) | ||
: (optionOrReport as OptionData).isConciergeChat ?? isConciergeChatReport(optionOrReport); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should had used optional chaining here, this caused the app to crash and caused the issue #45492
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.8-6 🚀
|
Details
Fixed Issues
$#43671
PROPOSAL: N/A
Tests
accountID
:accountID
is a odd number, then the chat used to display the free trial ended will be the Expensify DM, otherwise it's the Concierge DM.Expensify.tsx
in order to display the Free Trial ended message in the chat:text
property from the report action you added in the previous stepOffline tests
Same as above
QA Steps
Only test on web and desktop for now. It'll be tricky for QA to test other platforms just yet. Once the feature is completely done we'll put the full steps for QA to test (or self test them).
WEB/DESKTOP
accountID
:accountID
is a odd number, then the chat used to display the free trial ended will be the Expensify DM, otherwise it's the Concierge DM.reportID
from the urltext
property from the report action you added in the previous step.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android.-.Native.mp4
Android: mWeb Chrome
Android.-.Chrome.mp4
iOS: Native
iOS.-.Native.mp4
iOS: mWeb Safari
iOS.-.Safari.mp4
MacOS: Chrome / Safari
MacOS.-.Chrome.mp4
MacOS: Desktop
MacOS.-.Native.mp4