Skip to content
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

chore: remove nft gallery and old token details #5016

Merged
merged 25 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0303a66
chore: remove old token balance screen
MuckT Feb 29, 2024
10cccc8
chore: remove nft gallery screen
MuckT Feb 29, 2024
fe747e0
chore: remove unused view_nft_home_assets
MuckT Feb 29, 2024
e2af517
chore: remove show asset details screen statsig feature gate
MuckT Feb 29, 2024
419f1b7
chore: remove show in app nft viewer and show in app nft gallery feat…
MuckT Feb 29, 2024
a13c34e
Merge branch 'main' into tomm/act-1074
MuckT Feb 29, 2024
9a49272
fix: remove unused event, screen and type
MuckT Feb 29, 2024
2a64297
chore: update root state schema
MuckT Feb 29, 2024
ec6dd4b
Merge branch 'main' into tomm/act-1074
MuckT Mar 1, 2024
0838a89
test: update nft feed item tests for feature gate removal
MuckT Mar 1, 2024
0fb5770
test: update nft saga tests for feature gate removal
MuckT Mar 1, 2024
5145410
fix: make fiat exchange and home token balances behavior consistent
MuckT Mar 1, 2024
f42ac27
test: update token balance tests for feature flag and behavior alignment
MuckT Mar 1, 2024
09c020c
Merge branch 'main' into tomm/act-1074
MuckT Mar 2, 2024
4d84eef
Merge branch 'main' into tomm/act-1074
MuckT Mar 4, 2024
eb0e334
Merge branch 'main' into tomm/act-1074
MuckT Mar 4, 2024
4f70905
chore: remove unused screen
MuckT Mar 5, 2024
6fb2c1d
test: adjust mocking of feature gate
MuckT Mar 5, 2024
3a38b24
Merge branch 'main' into tomm/act-1074
MuckT Mar 5, 2024
62c8ea5
chore: restore FiatExchangeTokenBalance behavior
MuckT Mar 5, 2024
99c38dc
test: split out test cases for separate components where needed
MuckT Mar 5, 2024
93a5631
chore: update root state schema
MuckT Mar 5, 2024
047325d
test: remove it.each for single test case
MuckT Mar 5, 2024
fcedcf6
test: remove unused mocked feature gate
MuckT Mar 5, 2024
0459fa9
test(token-balances): clean-up and ensuring feature flags are set
MuckT Mar 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions e2e/src/usecases/Assets.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,6 @@ export default Assets = () => {
await device.installApp()
await launchApp({
newInstance: true,
launchArgs: {
// NFT flag must also be true, otherwise Collectibles tab show an empty screen
statsigGateOverrides: `show_asset_details_screen=true,show_in_app_nft_gallery=true`,
},
permissions: { notifications: 'YES', contacts: 'YES', camera: 'YES' },
})
let mnemonic = SAMPLE_BACKUP_KEY
Expand Down
2 changes: 0 additions & 2 deletions src/analytics/Events.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export enum HomeEvents {
transaction_feed_item_select = 'transaction_feed_item_select',
transaction_feed_address_copy = 'transaction_feed_address_copy',
view_token_balances = 'view_token_balances',
view_nft_home_assets = 'view_nft_home_assets',
home_action_pressed = 'home_action_pressed',
notification_bell_pressed = 'notification_bell_pressed',
notification_center_opened = 'notification_center_opened',
Expand Down Expand Up @@ -614,7 +613,6 @@ export enum AssetsEvents {
export enum NftEvents {
nft_error_screen_open = 'nft_error_screen_open',
nft_media_load = 'nft_media_load',
nft_gallery_screen_open = 'nft_gallery_screen_open',
}

export enum BuilderHooksEvents {
Expand Down
4 changes: 0 additions & 4 deletions src/analytics/Properties.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ interface HomeEventsProperties {
[HomeEvents.transaction_feed_item_select]: undefined
[HomeEvents.transaction_feed_address_copy]: undefined
[HomeEvents.view_token_balances]: { totalBalance?: string }
[HomeEvents.view_nft_home_assets]: undefined
[HomeEvents.home_action_pressed]: { action: HomeActionName }
[HomeEvents.notification_bell_pressed]: { hasNotifications: boolean }
[HomeEvents.notification_center_opened]: { notificationsCount: number }
Expand Down Expand Up @@ -1440,9 +1439,6 @@ interface NftsEventsProperties {
error?: string
mediaType: 'image' | 'video'
}
[NftEvents.nft_gallery_screen_open]: {
numNfts: number
}
}

interface BuilderHooksProperties {
Expand Down
4 changes: 2 additions & 2 deletions src/analytics/docs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ export const eventDocs: Record<AnalyticsEventType, string> = {
[HomeEvents.transaction_feed_item_select]: ``,
[HomeEvents.transaction_feed_address_copy]: ``,
[HomeEvents.view_token_balances]: ``,
[HomeEvents.view_nft_home_assets]: `When "NFTs" is clicked in Home Assets Pages`,
[HomeEvents.home_action_pressed]: ``,
[HomeEvents.notification_bell_pressed]: ``,
[HomeEvents.hide_balances]: `When the eye icon is clicked to hide balances on the home screen`,
Expand Down Expand Up @@ -528,7 +527,6 @@ export const eventDocs: Record<AnalyticsEventType, string> = {
[AssetsEvents.import_token_error]: `When a user enters token address and validation fails in the Import Token screen`,
[NftEvents.nft_error_screen_open]: `When the high level error screen is mounted`,
[NftEvents.nft_media_load]: `When attempting to load NFT media`,
[NftEvents.nft_gallery_screen_open]: `When the gallery screen is mounted`,
[BuilderHooksEvents.hooks_enable_preview_propose]: `When a user scans a QR code or opens a deep link to enable hooks preview`,
[BuilderHooksEvents.hooks_enable_preview_cancel]: `When a user cancels the hooks preview flow`,
[BuilderHooksEvents.hooks_enable_preview_confirm]: `When a user confirms enabling hooks preview`,
Expand All @@ -554,6 +552,7 @@ export const eventDocs: Record<AnalyticsEventType, string> = {
// Legacy event docs
// The below events had docs, but are no longer produced by the latest app version.
// [HomeEvents.home_send]: `when "send" button is pressed from home screen send or request bar (NOT from home screen actions)`,
// [HomeEvents.view_nft_home_assets]: `When "NFTs" is clicked in Home Assets Pages`,
// [DappExplorerEvents.dapp_open_info]: `when a user taps on the help icon`,
// [DappExplorerEvents.dapp_open_more_info]: `when a user taps on the "more" button from inside the help bottom sheet`,
// [DappExplorerEvents.dapp_search]: `when a user searches on the dapp explorer screen`,
Expand All @@ -564,6 +563,7 @@ export const eventDocs: Record<AnalyticsEventType, string> = {
// [CeloExchangeEvents.celo_withdraw_cancel]: `when ’cancel’ is clicked on the review screen`,
// [CeloExchangeEvents.celo_withdraw_confirm]: `when ‘withdraw’ is clicked on the review screen`,
// [CeloExchangeEvents.celo_withdraw_error]: `when there's an error on the withdrawal transaction`,
// [NftEvents.nft_gallery_screen_open]: `When the gallery screen is mounted`,
// [PhoneVerificationEvents.phone_verification_input_help_skip]: `when the user presses skip on the help dialog to skip verification`,
// [PhoneVerificationEvents.phone_verification_skip]: `when skip is pressed in the phone number input screen`,
// [IdentityEvents.contacts_connect]: `when connect button is pressed`,
Expand Down
96 changes: 12 additions & 84 deletions src/components/TokenBalance.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,7 @@ jest.mocked(getDynamicConfigParams).mockReturnValue({
describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
beforeEach(() => {
jest.clearAllMocks()
jest
.mocked(getFeatureGate)
.mockImplementation(
(featureGate) => featureGate !== StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN
)
jest.mocked(getFeatureGate).mockImplementation(() => true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is controlling multiple features flags. Open to other options on how to handle this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the TokenBalance.tsx only references SHOW_HIDE_HOME_BALANCES_TOGGLE gate now that the SHOW_ASSET_DETAILS_SCREEN gate is removed?
Could change this and all below references to this (and just update true / false appropriately)

Suggested change
jest.mocked(getFeatureGate).mockImplementation(() => true)
jest.mocked(getFeatureGate).mockReturnValue(true)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6fb2c1d!

})

it.each([HomeTokenBalance, FiatExchangeTokenBalance])(
Expand Down Expand Up @@ -113,7 +109,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
)

it.each([HomeTokenBalance, FiatExchangeTokenBalance])(
'navigates to TokenBalances screen on View Balances tap if AssetDetails feature gate is false',
'navigates to Assets screen on View Balances tap',
async (TokenBalanceComponent) => {
const store = createMockStore({
...defaultStore,
Expand Down Expand Up @@ -148,49 +144,6 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

fireEvent.press(getByTestId('ViewBalances'))
expect(navigate).toHaveBeenCalledWith(Screens.TokenBalances)
}
)

it.each([HomeTokenBalance, FiatExchangeTokenBalance])(
'navigates to Assets screen on View Balances tap if AssetDetails feature gate is true',
async (TokenBalanceComponent) => {
const store = createMockStore({
...defaultStore,
tokens: {
// FiatExchangeTokenBalance requires 2 balances to display the View Balances button
tokenBalances: {
'celo-alfajores:0x00400FcbF0816bebB94654259de7273f4A05c762': {
priceUsd: '0.1',
tokenId: 'celo-alfajores:0x00400FcbF0816bebB94654259de7273f4A05c762',
address: '0x00400FcbF0816bebB94654259de7273f4A05c762',
networkId: NetworkId['celo-alfajores'],
symbol: 'POOF',
balance: '5',
priceFetchedAt: Date.now(),
},
'celo-alfajores:0x10c892A6EC43a53E45D0B916B4b7D383B1b78C0F': {
priceUsd: '1.16',
address: '0x10c892A6EC43a53E45D0B916B4b7D383B1b78C0F',
tokenId: 'celo-alfajores:0x10c892A6EC43a53E45D0B916B4b7D383B1b78C0F',
networkId: NetworkId['celo-alfajores'],
symbol: 'cEUR',
balance: '7',
priceFetchedAt: Date.now(),
},
},
},
})

jest.mocked(getFeatureGate).mockReturnValue(true)

const { getByTestId } = render(
<Provider store={store}>
<TokenBalanceComponent />
</Provider>
)

fireEvent.press(getByTestId('ViewBalances'))
expect(navigate).toHaveBeenCalledWith(Screens.Assets)
}
Expand All @@ -215,34 +168,11 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

expect(tree.queryByTestId('ViewBalances')).toBeFalsy()
expect(tree.queryByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('$0.00')
}
)

it('HomeTokenBalance shows View Assets link if balance is zero and feature gate is true', async () => {
const store = createMockStore({
...defaultStore,
tokens: {
tokenBalances: {},
},
positions: {
positions: [],
},
})

jest.mocked(getFeatureGate).mockReturnValue(true)

const tree = render(
<Provider store={store}>
<HomeTokenBalance />
</Provider>
)

expect(tree.getByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('$0.00')
})

it.each([HomeTokenBalance, FiatExchangeTokenBalance])(
'renders correctly with zero balance and some positions',
async (TokenBalanceComponent) => {
Expand All @@ -259,7 +189,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

expect(tree.queryByTestId('ViewBalances')).toBeFalsy()
expect(tree.queryByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('$7.91')
}
)
Expand All @@ -280,6 +210,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

expect(tree.queryByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('$0.50')
}
)
Expand All @@ -295,6 +226,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

expect(tree.queryByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('$8.41')
}
)
Expand Down Expand Up @@ -396,7 +328,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

expect(tree.queryByTestId('ViewBalances')).toBeFalsy()
expect(tree.queryByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('₱0.00')
}
)
Expand All @@ -416,7 +348,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

expect(tree.queryByTestId('ViewBalances')).toBeFalsy()
expect(tree.queryByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('₱10.52')
}
)
Expand Down Expand Up @@ -520,7 +452,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

expect(tree.queryByTestId('ViewBalances')).toBeFalsy()
expect(tree.queryByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('₱-')

expect(store.getActions()).toMatchInlineSnapshot(`
Expand Down Expand Up @@ -556,7 +488,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
</Provider>
)

expect(tree.queryByTestId('ViewBalances')).toBeFalsy()
expect(tree.queryByTestId('ViewBalances')).toBeTruthy()
expect(getElementText(tree.getByTestId('TotalTokenBalance'))).toEqual('₱-')

expect(store.getActions()).toMatchInlineSnapshot(`
Expand Down Expand Up @@ -685,9 +617,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
jest
.mocked(getFeatureGate)
.mockImplementation(
(featureGate) =>
featureGate !== StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN &&
featureGate !== StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE
(featureGate) => featureGate !== StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE
)
const store = createMockStore(defaultStore)

Expand All @@ -706,9 +636,7 @@ describe('FiatExchangeTokenBalance and HomeTokenBalance', () => {
jest
.mocked(getFeatureGate)
.mockImplementation(
(featureGate) =>
featureGate !== StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN &&
featureGate !== StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE
(featureGate) => featureGate !== StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE
)
const store = createMockStore({ ...defaultStore, app: { hideHomeBalances: true } })

Expand Down
49 changes: 11 additions & 38 deletions src/components/TokenBalance.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -211,14 +211,7 @@ export function AssetsTokenBalance({ showInfo }: { showInfo: boolean }) {
</TouchableOpacity>
)}
</View>
<TokenBalance
style={
getFeatureGate(StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN)
? styles.totalBalance
: styles.balance
}
singleTokenViewEnabled={false}
/>
<TokenBalance style={styles.totalBalance} singleTokenViewEnabled={false} />

{shouldRenderInfoComponent && (
<Animated.View style={[styles.totalAssetsInfoContainer, animatedStyles]}>
Expand All @@ -242,11 +235,7 @@ export function HomeTokenBalance() {
ValoraAnalytics.track(HomeEvents.view_token_balances, {
totalBalance: totalBalance?.toString(),
})
navigate(
getFeatureGate(StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN)
? Screens.Assets
: Screens.TokenBalances
)
navigate(Screens.Assets)
}

const onCloseDialog = () => {
Expand Down Expand Up @@ -276,20 +265,13 @@ export function HomeTokenBalance() {
>
{t('whatTotalValue.body')}
</Dialog>
{(getFeatureGate(StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN) ||
tokenBalances.length >= 1) && (
<TouchableOpacity style={styles.row} onPress={onViewBalances} testID="ViewBalances">
<Text style={styles.viewBalances}>{t('viewBalances')}</Text>
<ProgressArrow style={styles.arrow} color={Colors.primary} />
</TouchableOpacity>
)}
<TouchableOpacity style={styles.row} onPress={onViewBalances} testID="ViewBalances">
<Text style={styles.viewBalances}>{t('viewBalances')}</Text>
<ProgressArrow style={styles.arrow} color={Colors.primary} />
</TouchableOpacity>
</View>
<TokenBalance
style={
getFeatureGate(StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN)
? styles.totalBalance
: styles.balance
}
style={styles.totalBalance}
showHideHomeBalancesToggle={getFeatureGate(
StatsigFeatureGates.SHOW_HIDE_HOME_BALANCES_TOGGLE
)}
Expand All @@ -301,31 +283,22 @@ export function HomeTokenBalance() {
export function FiatExchangeTokenBalance() {
const { t } = useTranslation()
const totalBalance = useTotalTokenBalance()
const tokenBalances = useTokensWithTokenBalance()

const onViewBalances = () => {
ValoraAnalytics.track(FiatExchangeEvents.cico_landing_token_balance, {
totalBalance: totalBalance?.toString(),
})
navigate(
getFeatureGate(StatsigFeatureGates.SHOW_ASSET_DETAILS_SCREEN)
? Screens.Assets
: Screens.TokenBalances
)
navigate(Screens.Assets)
}

return (
<View style={styles.container} testID="FiatExchangeTokenBalance">
<View style={styles.titleExchange}>
<View style={styles.row}>
{tokenBalances.length > 1 ? (
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior was updated because the tests were coupled together. Now the behavior of the home token balances and the fiat exchange token balances matches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be updated? Seems like the original intent is to not make it a touchable because there's only one token and there's no need for the user to go to the assets screen to view the breakdown
image
If we want to change this behavior, I'd do this in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior is different when show_asset_details_screen was set to true which is 100%. If we want to keep the behavior of the existing FiatExchangeTokenBalance some tests will need to be updated. Due to the coupled testing it was unclear if we ever expected these two components to diverge in behavior like they have.

Copy link
Contributor

@satish-ravi satish-ravi Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the FiatExchangeTokenBalance behavior has always been different from the HomeTokenBalance behavior.
FiatExchangeTokenBalance shows the ViewBalances touchable if num tokens > 1 both before and after the asset details change. HomeTokenBalance before asset details only included the ViewBalances touchable if num tokens >= 1 and after asset details, it is always included (since we also show tokens with zero balance in the assets screen).

I think we had coupled tests prior just because the case for 0 tokens overlapped. Can't we just drop the it.each and just keep the test for FiatExchangeTokenBalance and keep this test for HomeTokenBalance and just remove the feature gate in the name and mocks? No new tests need to be added as part of this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 62c8ea5 and tests updated in 99c38dc!

<TouchableOpacity style={styles.row} onPress={onViewBalances} testID="ViewBalances">
<Text style={styles.exchangeTotalValue}>{t('totalValue')}</Text>
<ProgressArrow style={styles.exchangeArrow} height={9.62} color={Colors.gray4} />
</TouchableOpacity>
) : (
<TouchableOpacity style={styles.row} onPress={onViewBalances} testID="ViewBalances">
<Text style={styles.exchangeTotalValue}>{t('totalValue')}</Text>
)}
<ProgressArrow style={styles.exchangeArrow} height={9.62} color={Colors.gray4} />
</TouchableOpacity>
</View>
</View>
<TokenBalance style={styles.exchangeBalance} />
Expand Down
15 changes: 0 additions & 15 deletions src/icons/navigator/NFT.tsx

This file was deleted.

Loading
Loading