Default to null balance instead of 0 (show '-' for unknown balance) #916

lukaw3d · 2022-07-08T00:14:48Z

Fixes #816

Before	After

github-actions · 2022-07-08T00:16:47Z

MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Fixed	Errors	Elapsed time
✅ EDITORCONFIG	editorconfig-checker	17		0	0.04s
✅ GIT	git_diff	yes		no	0.02s
✅ TSX	eslint	6	0	0	7.34s
✅ TYPESCRIPT	eslint	8	0	0	7.49s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

codecov · 2022-07-08T00:21:32Z

Codecov Report

Merging #916 (a533303) into master (fe1790a) will increase coverage by 0.03%.
The diff coverage is 84.61%.

❗ Current head a533303 differs from pull request most recent head 1e764db. Consider uploading reports for the commit 1e764db to get more accurate results

@@            Coverage Diff             @@
##           master     #916      +/-   ##
==========================================
+ Coverage   87.74%   87.77%   +0.03%     
==========================================
  Files          98       97       -1     
  Lines        1640     1628      -12     
  Branches      358      360       +2     
==========================================
- Hits         1439     1429      -10     
+ Misses        201      199       -2

Flag	Coverage Δ
cypress	`57.75% <53.84%> (-0.32%)`	⬇️
jest	`79.07% <84.61%> (+4.55%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/app/components/AmountFormatter/index.tsx	`100.00% <ø> (ø)`
...ages/AccountPage/Features/AccountSummary/index.tsx	`100.00% <ø> (ø)`
src/app/state/account/index.ts	`73.68% <0.00%> (-4.10%)`	⬇️
src/app/state/account/saga.ts	`63.04% <0.00%> (ø)`
src/app/pages/AccountPage/index.tsx	`97.77% <100.00%> (+2.53%)`	⬆️
...e/Features/DelegationList/ActiveDelegationList.tsx	`100.00% <100.00%> (ø)`
...eatures/DelegationList/DebondingDelegationList.tsx	`100.00% <100.00%> (ø)`
src/app/state/staking/index.ts	`89.28% <100.00%> (+8.51%)`	⬆️
src/app/components/Footer/index.tsx	`100.00% <0.00%> (ø)`
...rc/app/components/MobileFooterNavigation/index.tsx
... and 2 more

pro-wh · 2022-07-08T16:31:32Z

gonna have to ask about this on the issue

src/app/pages/AccountPage/__tests__/index.test.tsx

buberdds · 2022-07-12T14:47:54Z

src/app/pages/AccountPage/__tests__/index.test.tsx

-    const history = createMemoryHistory()
-    history.push('/account/account_address')
-    const page = renderPage(store, history)
+  it('with missing delegations', async () => {


Can we not create a snapshot for with missing delegations test? Both snapshots creates +2k lines and in most cases they are useless as one snapshot already covers page layout markup and styles.

Maybe. I would like it capture:

"Couldn't load delegations."

"API appears to be down"

Total balance - Available 100.0 Staked - Debonding -

"Active delegations ()" (so we notice if this breaks. i18n uses "count" to signify plurality, but we give it null)

without sprinkling data-testid everywhere

Changed to:

oasis-wallet-web/src/app/pages/AccountPage/__tests__/index.test.tsx

Lines 84 to 90 in 93184ec

expect(page.container).toHaveTextContent('Oasis Scan API appears to be down')

const balance = await screen.findByTestId('account-balance-total')

expect(balance).toHaveTextContent('-')

const balanceSummary = await screen.findByTestId('account-balance-summary')

expect(balanceSummary.textContent).toMatchSnapshot()

const tabs = await screen.findByRole('navigation')

expect(tabs.textContent).toMatchSnapshot()

oasis-wallet-web/src/app/pages/AccountPage/__tests__/__snapshots__/index.test.tsx.snap

Lines 1225 to 1227 in 93184ec

exports[`<AccountPage /> with missing delegations 1`] = `"Total balance-Available100.0 Staked-Debonding-"`;

exports[`<AccountPage /> with missing delegations 2`] = `"TransactionsActive delegations ()Debonding delegations ()"`;

cool, thanks

src/app/state/staking/index.ts

buberdds · 2022-07-12T14:57:35Z

src/app/pages/AccountPage/index.tsx

@@ -147,13 +151,13 @@ export function AccountPage(props: Props) {
            <NavItem
              route={`/account/${address}/active-delegations`}
              label={t('account.subnavigation.activeDelegations', 'Active delegations ({{count}})', {
-                count: stake.delegations.length,
+                count: stake.delegations?.length ?? null!,


Is this going to show a label with an empty bracket when nullish coalescing operator returns its right-hand side operand?

yes, it displays Active delegations ()

More effort solutions:

add another translation string stake.delegations ? t(account.subnavigation.activeDelegations) : t(account.subnavigation.activeDelegationsNull)

use a different field: count is used for plurality and needs to be a number, and null workaround could break. Make that other field a string, so it can be e.g. - too, and update translations

tjanez

I actually prefer - to ?, I think it is prettier and looks more professional.

lukaw3d requested review from buberdds, tjanez and pro-wh July 8, 2022 00:14

lukaw3d force-pushed the lw/unknown-null branch from 1a59e70 to 903b31e Compare July 8, 2022 02:31

lukaw3d mentioned this pull request Jul 8, 2022

When backend doesn't give a response, prefer showing ? (for unknown) #816

Closed

lukaw3d force-pushed the lw/unknown-null branch from fc04e3c to 18f8c74 Compare July 11, 2022 17:59

buberdds reviewed Jul 12, 2022

View reviewed changes

lukaw3d force-pushed the lw/unknown-null branch 3 times, most recently from 93184ec to a533303 Compare July 13, 2022 21:55

tjanez approved these changes Jul 15, 2022

View reviewed changes

lukaw3d added 8 commits July 18, 2022 18:26

Clear transactions and delegations on error

c570dac

Rename "liquid_balance" to "available"

a2d8512

Default to null balance instead of 0

d2f38ba

Default to null delegations instead of []

ab3e4f4

Fix undefined commission rates in monitor

57f2c92

Test AccountPage

9a2b6cd

Rename data-testid="account-balance-total"

41e5549

Reduce snapshot in AccountPage with missing delegations

1e764db

lukaw3d force-pushed the lw/unknown-null branch from a533303 to 1e764db Compare July 18, 2022 16:27

lukaw3d merged commit 13289f4 into master Jul 18, 2022

lukaw3d deleted the lw/unknown-null branch July 18, 2022 16:33

lukaw3d mentioned this pull request Sep 15, 2022

ParaTimes: handle MAX value in transaction amount step #988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to null balance instead of 0 (show '-' for unknown balance) #916

Default to null balance instead of 0 (show '-' for unknown balance) #916

lukaw3d commented Jul 8, 2022

github-actions bot commented Jul 8, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading

pro-wh commented Jul 8, 2022

buberdds Jul 12, 2022

lukaw3d Jul 12, 2022 •

edited

Loading

lukaw3d Jul 12, 2022

buberdds Jul 13, 2022

buberdds Jul 12, 2022

lukaw3d Jul 12, 2022

tjanez left a comment

	expect(page.container).toHaveTextContent('Oasis Scan API appears to be down')
	const balance = await screen.findByTestId('account-balance-total')
	expect(balance).toHaveTextContent('-')
	const balanceSummary = await screen.findByTestId('account-balance-summary')
	expect(balanceSummary.textContent).toMatchSnapshot()
	const tabs = await screen.findByRole('navigation')
	expect(tabs.textContent).toMatchSnapshot()

	exports[`<AccountPage /> with missing delegations 1`] = `"Total balance-Available100.0 Staked-Debonding-"`;

	exports[`<AccountPage /> with missing delegations 2`] = `"TransactionsActive delegations ()Debonding delegations ()"`;

Default to null balance instead of 0 (show '-' for unknown balance) #916

Default to null balance instead of 0 (show '-' for unknown balance) #916

Conversation

lukaw3d commented Jul 8, 2022

github-actions bot commented Jul 8, 2022 • edited Loading

MegaLinter status: ✅ SUCCESS

codecov bot commented Jul 8, 2022 • edited Loading

Codecov Report

pro-wh commented Jul 8, 2022

buberdds Jul 12, 2022

Choose a reason for hiding this comment

lukaw3d Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

lukaw3d Jul 12, 2022

Choose a reason for hiding this comment

buberdds Jul 13, 2022

Choose a reason for hiding this comment

buberdds Jul 12, 2022

Choose a reason for hiding this comment

lukaw3d Jul 12, 2022

Choose a reason for hiding this comment

tjanez left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 8, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading

lukaw3d Jul 12, 2022 •

edited

Loading