feat: reward bottom sheet #5024

bakoushin · 2024-03-04T11:10:15Z

Description

Show a bottom sheet with a description of a reward associated with a recently celebrated NFT (on NFT celebration see #4886).

The bottom sheet must show up:

only once within the app session
if the reward has not expired
after the NFT celebration was displayed or if the reward is about to expire

Simulator.Screen.Recording.-.iPhone.15.-.2024-03-04.at.12.38.48.mp4

The whole NFT celebration / reward / reminder journey is facilitated through reward status in a redux state:

Status	Expected behavior
`celebrationReadyToDisplay`	display NFT celebration
`celebrationDisplayed`	check if we can display a reward
`rewardReadyToDisplay`	display reward bottom sheet
`rewardDisplayed`	check if we can display a reminder
`reminderReadyToDisplay`	display reminder
`reminderDisplayed`	end of journey

Notes:

The expiration pill copy when the reward is about to expire (brown one) deliberately doesn't follow the designs because of localization concerns.
The former nftCelebration state is discarded during migration, because it was never used yet.

Test plan

Tested manually
Updated unit tests

Related issues

Fixes RET-1002

Backwards compatibility

Y

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

codecov · 2024-03-04T11:52:41Z

Codecov Report

Attention: Patch coverage is 93.56436% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 85.50%. Comparing base (5bab44d) to head (ae347ad).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5024      +/-   ##
==========================================
+ Coverage   85.46%   85.50%   +0.03%     
==========================================
  Files         719      721       +2     
  Lines       29199    29368     +169     
  Branches     5038     5079      +41     
==========================================
+ Hits        24955    25111     +156     
- Misses       4014     4024      +10     
- Partials      230      233       +3

Files	Coverage Δ
src/analytics/Events.tsx	`100.00% <100.00%> (ø)`
src/analytics/Properties.tsx	`100.00% <ø> (ø)`
src/home/WalletHome.tsx	`97.54% <100.00%> (+0.08%)`	⬆️
src/home/actions.ts	`97.05% <100.00%> (+1.05%)`	⬆️
src/home/celebration/NftCelebration.tsx	`76.81% <100.00%> (-1.57%)`	⬇️
src/home/celebration/utils.ts	`100.00% <100.00%> (ø)`
src/home/selectors.ts	`100.00% <100.00%> (ø)`
src/redux/migrations.ts	`97.28% <100.00%> (+<0.01%)`	⬆️
src/redux/store.ts	`78.68% <ø> (ø)`
src/statsig/constants.ts	`100.00% <ø> (ø)`
... and 6 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bab44d...ae347ad. Read the comment docs.

emerge-tools · 2024-03-04T14:09:26Z

1 build increased size

Name	Version	Download	Change	Install	Change	Approval
Celo (test) org.celo.mobile.test	1.79.0 (144)	24.2 MB	⬆️ 5.9 kB (0.02%)	60.3 MB	⬆️ 12.3 kB (0.02%)	N/A

Celo (test) 1.79.0 (144)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 12.3 kB (0.02%)
Total download size change: ⬆️ 5.9 kB (0.02%)

Largest size changes

Item	Install Size Change
main.jsbundle	⬆️ 12.3 kB

🛸 Powered by Emerge Tools

kathaypacific

i haven't reviewed the tests because i had some questions about the implementation, but looks good!!

kathaypacific · 2024-03-05T08:44:14Z

src/statsig/constants.ts

@@ -111,6 +111,9 @@ export const DynamicConfigs = {
    configName: StatsigDynamicConfigs.NFT_CELEBRATION_CONFIG,
    defaultValues: {
      celebratedNft: {} as { networkId?: NetworkId; contractAddress?: string },
+      deepLink: '',
+      expirationDate: new Date(0).toISOString(),
+      reminderDate: new Date(0).toISOString(),


Suggested change

reminderDate: new Date(0).toISOString(),

rewardReminderDate: new Date(0).toISOString(),

kathaypacific · 2024-03-05T08:44:27Z

src/statsig/constants.ts

@@ -111,6 +111,9 @@ export const DynamicConfigs = {
    configName: StatsigDynamicConfigs.NFT_CELEBRATION_CONFIG,
    defaultValues: {
      celebratedNft: {} as { networkId?: NetworkId; contractAddress?: string },
+      deepLink: '',
+      expirationDate: new Date(0).toISOString(),


Suggested change

expirationDate: new Date(0).toISOString(),

rewardExpirationDate: new Date(0).toISOString(),

aside: any reason why you chose string instead of number for the dates? using number would remove the need to parse it to be a number in the saga later?

because the strings are human readable. let machine do the conversion, not us 🤖

kathaypacific · 2024-03-05T08:57:38Z

test/RootStateSchema.json

@@ -2,6 +2,19 @@
    "$schema": "http://json-schema.org/draft-07/schema#",
    "additionalProperties": false,
    "definitions": {
+        "Action<any>": {


this is a strange / seemingly unrelated change?

kathaypacific · 2024-03-05T09:20:44Z

src/nfts/saga.ts

+
+  const expired = isPast(expirationDate)
+  if (expired) {
+    return


i was wondering if we should thinking about the celebration separately to the reward - it kind of made sense in my head to celebrate the nft even if the user does not open the app in time to use their reward? i'll ask the question in the design channel, perhaps i've not got the correct mental model of this feature.

if we want to also hide the nft celebration if the expiry date is past though, i'd suggest resetting the state here rather than returning early. that way you can remove the date checks in the selectors?

kathaypacific · 2024-03-05T09:27:48Z

src/nfts/saga.ts

+  ) {
+    return
+  }
+


instead of having the early returns above, could we gate the below logic we want to execute with the NftCelebrationStatus values that we care about? i found this logic a little hard to understand, but it seems we'd only want to execute logic if the NftCelebrationStatus === celebrationDisplayed, right?

kathaypacific · 2024-03-05T10:45:25Z

src/home/celebration/NftReward.tsx

+
+  const pillLabel =
+    expirationStatus === ExpirationStatus.expired
+      ? t('nftCelebration.rewardBottomSheet.expired')


if the reward is expired, should we hide the sheet rather than show a user that they've got expired rewards?

yes, hiding the sheet could be an option. with it, we have one string less to translate.

kathaypacific · 2024-03-05T10:51:58Z

src/home/celebration/NftReward.tsx

+  expired = 'expired',
+}
+
+// recalculates expiration status every second


if i understand correctly, this is necessary only towards the end of the reward period? i think that we can simplify this by setting an expiry date in statsig that is like 30min-1 hour before the actual expiry date - the worst case is that a user who is seeing this really close to the end of the program does not see the reward bottom sheet, however i think this tradeoff is worth the reduced code complexity? (we can also think of it like, we allow a grace period in the backend from the expiry time)

yes, it's mostly for handling the situation when the reward expires while the bottom sheet is open.

I agree that we could expect that user wouldn't spend too much time looking at this bottom sheet and it's acceptable to not update expiration time and not auto dismiss the bottom sheet once it expires.

I like the idea of having a grace period, giving the late comers the chance to exercise their rewards.

kathaypacific · 2024-03-05T10:52:52Z

src/home/celebration/NftReward.tsx

+          ? ExpirationStatus.aboutToExpire
+          : ExpirationStatus.active,
+    })
+    timeoutID.current = setTimeout(updateExpirationStatus, UPDATE_INTERVAL)


this recursion is kind of blowing my mind, why not do a setInterval instead?

kathaypacific · 2024-03-05T10:55:17Z

src/home/celebration/NftReward.tsx

+type ExpirationPillProps = { status: ExpirationStatus; label: string }
+
+const ExpirationPill = ({ status, label }: ExpirationPillProps) => (
+  <View style={styles.pillContainer}>


nit: i don't think you need this div?

kathaypacific

LGTM!!

kathaypacific · 2024-03-06T13:48:56Z

src/home/reducers.ts

  loading: boolean
  notifications: IdToNotification
  cleverTapInboxMessages: CleverTapInboxMessage[]
  hasVisitedHome: boolean
  nftCelebration: {
    networkId: NetworkId
    contractAddress: string
-    displayed: boolean
+    status: NftCelebrationStatus
+    expirationDate: string


Suggested change

expirationDate: string

rewardExpirationDate: string

kathaypacific · 2024-03-06T14:06:41Z

src/home/actions.ts

@@ -61,6 +85,9 @@ export type ActionTypes =
  | VisitHomeAction
  | CelebratedNftFoundAction
  | NftCelebrationDisplayedAction
+  | NftRewardReadyToDisplayAction
+  | NftRewardDisplayedAction
+  | NftRewardReminderReadyToDisplayAction


nit: i wonder if NftRewardReadyToDisplayAction and NftRewardReminderReadyToDisplayAction could be combined, the action + reducer + their usage seem very similar, perhaps it'd be more simple to have a single action that takes a boolean showReminder param?

sure, thanks!

kathaypacific · 2024-03-06T14:10:33Z

src/home/celebration/NftReward.tsx

+  const { t } = useTranslation()
+  const dispatch = useDispatch()
+
+  const canShowNftReward = useSelector(showNftRewardSelector)


nit: maybe you don't need to check this again because it is checked in the WalletHome already?

kathaypacific · 2024-03-06T14:12:14Z

src/home/celebration/NftReward.tsx

+  const expirationDate = new Date(nftCelebration?.expirationDate ?? 0)
+  const rewardReminderDate = new Date(nftCelebration?.rewardReminderDate ?? 0)
+
+  const aboutToExpire = isToday(rewardReminderDate) || isPast(rewardReminderDate)


can we rely on the value of NftCelebrationStatus here? the date is already used to determine the status in the findNftReward saga.

if not, can we remove the check for isPast here? it is unlikely to be true but it feels weird to evaluate a past date as "about to expire". can we use isPast to determine the value of isVisible instead?

I'm sorry, but I'm not getting the idea against isPast.

we want to show this bottom sheet in a "reminder state" if the current date falls within the reminder period.
it is not necessary that the user will open the app on the first day of this period, that's why we want to check if the reminder date is in the past.

but I agree that instead of looking at the reminder date we can rely on the NftCelebrationStatus instead. thanks for the suggestion!

kathaypacific · 2024-03-06T14:14:52Z

src/home/celebration/NftReward.tsx

+    }
+
+    if (index === -1) {
+      ValoraAnalytics.track(HomeEvents.nft_reward_dismiss, {


i think this is fine, but i think this dismiss event will always be fired (even if the accept event is also fired) - perhaps something we should note in the docs so that it's clear how to use this event. currently the doc says that this event is only triggered when the user dismisses the sheet (which i interpreted as dismiss without accepting the reward)

that's a great catch! thanks!
the docs are correct: this event must dispatch only if the reward wasn't accepted. fixed it by adding a state variable.

sadly I can't figure out yet how to simulate bottom sheet dismiss event in tests to cover it with a test.

kathaypacific · 2024-03-06T14:17:39Z

src/home/celebration/NftReward.tsx

+    ...typeScale.labelSemiBoldXSmall,
+    paddingHorizontal: Spacing.Small12,
+    paddingVertical: Spacing.Smallest8,
+    borderRadius: 1000,


this big radius made me lol 😂

kathaypacific · 2024-03-06T14:27:11Z

src/nfts/saga.ts

+    deepLink,
+  }
+
+  const showReminder = isToday(rewardReminderDate) || isPast(rewardReminderDate)


is isPast supposed to be here?

I believe, yes.

here we are checking if we are within the reminder period.

if user opens the app within the reminder period, but not exactly on the reminder start date, rewardReminderDate would be in the past.

for example:

user opened app: March, 2

rewardReminderDate: March, 1 (isPast === true)

OH of course, you're right. i was confusing this with the expiry date

kathaypacific · 2024-03-06T14:37:19Z

src/nfts/saga.ts

+    return
+  }
+
+  if (!isSameNftContract(lastNftCelebration, celebratedNft)) {


i found this function still quite hard to understand because there are so many early return conditions. i think there's room to simplify this, e.g. the checks for whether network id / contract address are already done in isSameNftContract, so we can remove some of the early return conditions relating to that? the bottom sheet itself also checks if the user owns the nft, so perhaps that condition can also be removed? it'd be ideal to have a condition that is the negative of all these early returns but idk if that's possible

yes, thank for spotting this.
removed some redundant early returns.

kathaypacific · 2024-03-06T14:40:22Z

src/home/celebration/NftReward.test.tsx

+    expect(queryByText('nftCelebration.rewardBottomSheet.cta')).toBeNull()
+  })
+
+  it('hanldes the cta correctly', () => {


Suggested change

it('hanldes the cta correctly', () => {

it('handles the cta correctly', () => {

kathaypacific · 2024-03-06T14:42:34Z

src/home/celebration/NftReward.test.tsx

+  })
+
+  it('hanldes the cta correctly', () => {
+    jest.useFakeTimers().setSystemTime(new Date('3000-11-01T00:00:00.000Z').getTime())


i think we need to reset this mock in the beforeEach's probably? otherwise this will affect the next tests, not a big deal now but could potentially cost some head scratching time in the future (like #4953 (review))

### Description Show a bottom sheet with a description of a reward associated with a recently celebrated NFT (on NFT celebration see valora-inc#4886). The bottom sheet must show up: * only once within the app session * if the reward has not expired * after the NFT celebration was displayed or if the reward is about to expire https://github.com/valora-inc/wallet/assets/2737872/7ffb0a71-7f53-4e9f-be8e-1a9690bc61f0 The whole NFT celebration / reward / reminder journey is facilitated through reward status in a redux state: | Status | Expected behavior | |--------|--------| | `celebrationReadyToDisplay` | display NFT celebration | | `celebrationDisplayed` | check if we can display a reward | | `rewardReadyToDisplay` | display reward bottom sheet | | `rewardDisplayed` | check if we can display a reminder | | `reminderReadyToDisplay` | display reminder | | `reminderDisplayed` | end of journey | Notes: 1. The expiration pill copy when the reward is about to expire (brown one) deliberately doesn't follow the designs because of localization concerns. 2. The former `nftCelebration` state is discarded during migration, because it was never used yet. ### Test plan * Tested manually * Updated unit tests ### Related issues - Fixes RET-1002 ### Backwards compatibility Y ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

bakoushin added 11 commits March 4, 2024 11:30

first draft

497aac0

fix logic and storage

32c23ac

add tests

b4798b5

add analytics

ab25637

more tests

1113730

more tests

8acf049

tidy up

fa11b30

Merge branch 'main' into alex/reward-bottom-sheet

05ef86f

fix migrations

ca48a2d

fix TS issues

26bb054

update RootStateSchema.json

fe91f82

bakoushin added 5 commits March 4, 2024 14:05

tidy up

40317ca

more tests

23cfe05

tidy up

1a80dc3

more tests

2aef28c

tidy up

b41e821

bakoushin added 2 commits March 4, 2024 15:25

Merge branch 'main' into alex/reward-bottom-sheet

9b928cc

tidy up

8a5a304

bakoushin marked this pull request as ready for review March 4, 2024 14:36

bakoushin added 3 commits March 4, 2024 15:38

update RootStateSchema.json

06c7f9b

fix RootStateSchema.json issue

eae69cb

RootStateSchema.json

798677a

kathaypacific reviewed Mar 5, 2024

View reviewed changes

bakoushin added 5 commits March 5, 2024 20:08

Merge branch 'main' into alex/reward-bottom-sheet

38f7cb7

reminderDate -> rewardReminderDate

f910e5b

auto dismiss bottom sheet when reward is expired

23fc736

tidy up styling

cb91f68

simplify expiration date calculation

bc0a347

bakoushin added 8 commits March 6, 2024 09:15

Merge branch 'main' into alex/reward-bottom-sheet

b11633e

Merge branch 'main' into alex/reward-bottom-sheet

6186c56

tidy up

56adf28

add separate copy for reward reminder sheet

caea6e2

util function isSameNftContract

a36dda5

switch over explicit celebration states in saga

8679bf6

update reward dates and deeplink when setting status

dcf683a

tidy up

9cfd429

kathaypacific approved these changes Mar 6, 2024

View reviewed changes

bakoushin added 10 commits March 6, 2024 16:29

expirationDate -> rewardExpirationDate

ceb7e9f

fix typo

47ccfa8

reset date faking after test

ecd3278

handle dismiss analytics event properly

0588e83

rely on NftCelebrationStatus to detect the reminder mode

1f11987

combine reward and reminder ready into a single action

dfd96a9

Merge branch 'main' into alex/reward-bottom-sheet

552644c

move visibility tests to WalletHome

3ebc9e5

remove redundant early returns from saga

f048fc7

update RootStateSchema.json

8a0d4aa

bakoushin enabled auto-merge March 7, 2024 11:29

bakoushin added this pull request to the merge queue Mar 7, 2024

bakoushin removed this pull request from the merge queue due to a manual request Mar 7, 2024

bakoushin added 2 commits March 7, 2024 12:59

tidy up

3aa661a

add more tests

ae347ad

bakoushin enabled auto-merge March 7, 2024 12:02

bakoushin added this pull request to the merge queue Mar 7, 2024

Merged via the queue into main with commit 09e6ff9 Mar 7, 2024
16 checks passed

bakoushin deleted the alex/reward-bottom-sheet branch March 7, 2024 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reward bottom sheet #5024

feat: reward bottom sheet #5024

bakoushin commented Mar 4, 2024 •

edited

codecov bot commented Mar 4, 2024 •

edited

emerge-tools bot commented Mar 4, 2024 •

edited

kathaypacific left a comment

kathaypacific Mar 5, 2024

kathaypacific Mar 5, 2024

kathaypacific Mar 5, 2024

bakoushin Mar 5, 2024

kathaypacific Mar 5, 2024

kathaypacific Mar 5, 2024

kathaypacific Mar 5, 2024

kathaypacific Mar 5, 2024

bakoushin Mar 5, 2024

kathaypacific Mar 5, 2024

bakoushin Mar 5, 2024

kathaypacific Mar 5, 2024

kathaypacific Mar 5, 2024

kathaypacific left a comment

kathaypacific Mar 6, 2024

kathaypacific Mar 6, 2024

bakoushin Mar 7, 2024

kathaypacific Mar 6, 2024

kathaypacific Mar 6, 2024

bakoushin Mar 7, 2024

kathaypacific Mar 6, 2024

bakoushin Mar 7, 2024

kathaypacific Mar 6, 2024

kathaypacific Mar 6, 2024

bakoushin Mar 7, 2024

kathaypacific Mar 7, 2024

kathaypacific Mar 6, 2024

bakoushin Mar 7, 2024

kathaypacific Mar 6, 2024

kathaypacific Mar 6, 2024

	reminderDate: new Date(0).toISOString(),
	rewardReminderDate: new Date(0).toISOString(),

	expirationDate: new Date(0).toISOString(),
	rewardExpirationDate: new Date(0).toISOString(),

	it('hanldes the cta correctly', () => {
	it('handles the cta correctly', () => {

feat: reward bottom sheet #5024

feat: reward bottom sheet #5024

Conversation

bakoushin commented Mar 4, 2024 • edited

Description

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Mar 4, 2024 • edited

Codecov Report

emerge-tools bot commented Mar 4, 2024 • edited

1 build increased size

Celo (test) 1.79.0 (144)org.celo.mobile.test

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin commented Mar 4, 2024 •

edited

codecov bot commented Mar 4, 2024 •

edited

emerge-tools bot commented Mar 4, 2024 •

edited

Celo (test) 1.79.0 (144)
org.celo.mobile.test