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

Fix/accounts tab search does not work correctly with special chars #14600

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
64 changes: 9 additions & 55 deletions packages/suite/src/components/suite/AccountLabel.tsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
import styled from 'styled-components';
import { getTitleForNetwork, getTitleForCoinjoinAccount } from '@suite-common/wallet-utils';
import { BadgeSize, Row, TOOLTIP_DELAY_LONG, TruncateWithTooltip } from '@trezor/components';
import { useCallback } from 'react';
import { useTranslation } from '../../hooks/suite';
import { spacings } from '@trezor/theme';
import { AccountType, NetworkSymbol } from '@suite-common/wallet-config';
import { AccountTypeBadge } from './AccountTypeBadge';
import { Bip43Path, NetworkType } from '@suite-common/wallet-config';
import { useDefaultAccountLabel } from 'src/hooks/suite';

const TabularNums = styled.span`
font-variant-numeric: tabular-nums;
text-overflow: ellipsis;
overflow: hidden;
`;

export interface AccountLabelProps {
interface AccountLabelProps {
accountLabel?: string;
accountType: AccountType;
symbol: NetworkSymbol;
Expand All @@ -25,36 +23,6 @@ export interface AccountLabelProps {
networkType?: NetworkType;
}

export const useAccountLabel = () => {
const { translationString } = useTranslation();

const defaultAccountLabelString = useCallback(
({
accountType,
symbol,
index = 0,
}: {
accountType: AccountType;
symbol: NetworkSymbol;
index?: number;
}) => {
if (accountType === 'coinjoin') {
return translationString(getTitleForCoinjoinAccount(symbol));
}

return translationString('LABELING_ACCOUNT', {
networkName: translationString(getTitleForNetwork(symbol)), // Bitcoin, Ethereum, ...
index: index + 1, // This is the number which shows after hash, e.g. Ethereum #3
});
},
[translationString],
);

return {
defaultAccountLabelString,
};
};

export const AccountLabel = ({
accountLabel,
accountType = 'normal',
Expand All @@ -63,32 +31,18 @@ export const AccountLabel = ({
showAccountTypeBadge,
accountTypeBadgeSize = 'medium',
symbol,
index = 0,
index,
}: AccountLabelProps) => {
const { defaultAccountLabelString } = useAccountLabel();

if (accountLabel) {
return (
<TruncateWithTooltip delayShow={TOOLTIP_DELAY_LONG}>
<Row gap={spacings.sm}>
<TabularNums>{accountLabel}</TabularNums>
{showAccountTypeBadge && (
<AccountTypeBadge
accountType={accountType}
size={accountTypeBadgeSize}
path={path}
networkType={networkType}
/>
)}
</Row>
</TruncateWithTooltip>
);
}
const { getDefaultAccountLabel } = useDefaultAccountLabel();

return (
<TruncateWithTooltip delayShow={TOOLTIP_DELAY_LONG}>
<Row gap={spacings.sm}>
{defaultAccountLabelString({ accountType, symbol, index })}
{accountLabel ? (
<TabularNums>{accountLabel}</TabularNums>
) : (
getDefaultAccountLabel({ accountType, symbol, index })
)}
{showAccountTypeBadge && (
<AccountTypeBadge
accountType={accountType}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import {
FiatValue,
AmountUnitSwitchWrapper,
} from 'src/components/suite';
import { useAccountLabel } from 'src/components/suite/AccountLabel';
import { useSelector } from 'src/hooks/suite';
import { useDefaultAccountLabel, useSelector } from 'src/hooks/suite';
import { selectLabelingDataForSelectedAccount } from 'src/reducers/suite/metadataReducer';
import { CoinLogo } from '@trezor/product-components';

Expand Down Expand Up @@ -83,7 +82,7 @@ export const AccountDetails = ({ selectedAccount, isBalanceShown }: AccountDetai
const [shouldAnimate, setShouldAnimate] = useState(false);
const [hasMounted, setHasMounted] = useState(false);
const selectedAccountLabels = useSelector(selectLabelingDataForSelectedAccount);
const { defaultAccountLabelString } = useAccountLabel();
const { getDefaultAccountLabel } = useDefaultAccountLabel();
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for renaming the returned callback, the old name didn't make much sense 👍

const { symbol, key, path, index, accountType, formattedBalance } = selectedAccount;

useEffect(() => {
Expand Down Expand Up @@ -121,7 +120,7 @@ export const AccountDetails = ({ selectedAccount, isBalanceShown }: AccountDetai
defaultValue: path,
value: selectedAccountLabels.accountLabel,
}}
defaultEditableValue={defaultAccountLabelString({ accountType, symbol, index })}
defaultEditableValue={getDefaultAccountLabel({ accountType, symbol, index })}
updateFlag={isBalanceShown}
/>
</AccountHeading>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { sortByCoin, getFailedAccounts, accountSearchFn } from '@suite-common/wallet-utils';
import { Account } from '@suite-common/wallet-types';
import { useAccountSearch, useDiscovery, useSelector } from 'src/hooks/suite';
import { selectDevice } from '@suite-common/wallet-core';
import { selectAccounts, selectDevice } from '@suite-common/wallet-core';
import { selectAccountLabels } from 'src/reducers/suite/metadataReducer';
import { Translation } from 'src/components/suite';
import { AccountItemSkeleton } from './AccountItemSkeleton';
Expand All @@ -10,17 +10,19 @@ import { AccountsMenuNotice } from './AccountsMenuNotice';
import { spacings } from '@trezor/theme';
import { Column } from '@trezor/components';
import { AccountSection } from './AcccountSection';
import { useDefaultAccountLabel } from 'src/hooks/suite';

interface AccountListProps {
onItemClick?: () => void;
}

export const AccountsList = ({ onItemClick }: AccountListProps) => {
const device = useSelector(selectDevice);
const accounts = useSelector(state => state.wallet.accounts);
const accounts = useSelector(selectAccounts);
const selectedAccount = useSelector(state => state.wallet.selectedAccount);
const coinjoinIsPreloading = useSelector(state => state.wallet.coinjoin.isPreloading);
const accountLabels = useSelector(selectAccountLabels);
const { getDefaultAccountLabel } = useDefaultAccountLabel();

const { discovery, getDiscoveryStatus } = useDiscovery();
const { coinFilter, searchString } = useAccountSearch();
Expand All @@ -37,7 +39,14 @@ export const AccountsList = ({ onItemClick }: AccountListProps) => {
const list = sortByCoin(accounts.filter(a => a.deviceState === device.state).concat(failed));
const filteredAccounts =
searchString || coinFilter
? list.filter(a => accountSearchFn(a, searchString, coinFilter, accountLabels[a.key]))
? list.filter(account => {
const { key, accountType, symbol, index } = account;
const accountLabel = accountLabels.hasOwnProperty(key)
? accountLabels[key]
: getDefaultAccountLabel({ accountType, symbol, index });

return accountSearchFn(account, searchString, coinFilter, accountLabel);
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 I'd add a test case for this at accountUtils.test.ts#L217.
Although the accountSearchFn function is itself unchanged, its usage has slightly changed. There is now no test case to match substring with supplied label, only whole string.
This will test it, and at the same time it'll help describe what is the fn typically used for 🙂

expect(accountSearchFn(btcAcc, '#1', undefined, 'Bitcoin #1')).toBe(true);

})
: list;

const filterAccountsByType = (type: Account['accountType']) =>
Expand Down
1 change: 1 addition & 0 deletions packages/suite/src/hooks/suite/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export { useDispatch } from './useDispatch';
export { usePasswords } from './usePasswords';
export { useDebugLanguageShortcut } from './useDebugLanguageShortcut';
export { useDisplayMode } from './useDisplayMode';
export { useDefaultAccountLabel } from './useDefaultAccountLabel';

// replaced in suite-native
export { useLocales } from 'src/hooks/suite/useLocales';
34 changes: 34 additions & 0 deletions packages/suite/src/hooks/suite/useDefaultAccountLabel.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { getTitleForNetwork, getTitleForCoinjoinAccount } from '@suite-common/wallet-utils';
import { useCallback } from 'react';
import { useTranslation } from './useTranslation';
import { AccountType, NetworkSymbol } from '@suite-common/wallet-config';

export interface GetDefaultAccountLabelParams {
accountType: AccountType;
symbol: NetworkSymbol;
index?: number;
}

export const useDefaultAccountLabel = () => {
const { translationString } = useTranslation();

const getDefaultAccountLabel = useCallback(
({ accountType, symbol, index = 0 }: GetDefaultAccountLabelParams): string => {
if (accountType === 'coinjoin') {
return translationString(getTitleForCoinjoinAccount(symbol));
}

const displayedAccountNumber = index + 1;

return translationString('LABELING_ACCOUNT', {
networkName: translationString(getTitleForNetwork(symbol)), // Bitcoin, Ethereum, ...
index: displayedAccountNumber,
});
},
[translationString],
);

return {
getDefaultAccountLabel,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@ import {
import { CoinmarketSellFormDefaultValuesProps } from 'src/types/coinmarket/coinmarketForm';
import { FormState, Output } from '@suite-common/wallet-types';
import { selectAccountLabels } from 'src/reducers/suite/metadataReducer';
import { useSelector } from 'src/hooks/suite';
import { useSelector, useDefaultAccountLabel } from 'src/hooks/suite';
import { selectAccounts, selectDevice } from '@suite-common/wallet-core';
import { useAccountLabel } from 'src/components/suite/AccountLabel';
import {
FORM_DEFAULT_FIAT_CURRENCY,
FORM_DEFAULT_PAYMENT_METHOD,
Expand All @@ -30,7 +29,7 @@ export const useCoinmarketBuildAccountGroups = (
const accounts = useSelector(selectAccounts);
const accountLabels = useSelector(selectAccountLabels);
const device = useSelector(selectDevice);
const { defaultAccountLabelString } = useAccountLabel();
const { getDefaultAccountLabel } = useDefaultAccountLabel();
const { tokenDefinitions } = useSelector(state => state);
const supportedSymbols = useSelector(state =>
type === 'sell'
Expand All @@ -46,15 +45,15 @@ export const useCoinmarketBuildAccountGroups = (
accountLabels,
tokenDefinitions,
supportedCryptoIds: supportedSymbols,
defaultAccountLabelString,
getDefaultAccountLabel,
}),
[
accounts,
supportedSymbols,
accountLabels,
device,
tokenDefinitions,
defaultAccountLabelString,
getDefaultAccountLabel,
],
);

Expand Down
9 changes: 3 additions & 6 deletions packages/suite/src/types/coinmarket/coinmarket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
SelectAssetOptionGroupProps,
SelectAssetOptionProps,
} from '@trezor/product-components/src/components/SelectAssetModal/SelectAssetModal';
import { GetDefaultAccountLabelParams } from 'src/hooks/suite/useDefaultAccountLabel';

export type UseCoinmarketProps = WithSelectedAccountLoadedProps;
export type UseCoinmarketCommonProps = UseCoinmarketProps & {
Expand Down Expand Up @@ -190,15 +191,11 @@ export interface CoinmarketGetSortedAccountsProps {

export interface CoinmarketBuildAccountOptionsProps extends CoinmarketGetSortedAccountsProps {
accountLabels: Record<string, string | undefined>;
defaultAccountLabelString: ({
getDefaultAccountLabel: ({
accountType,
symbol,
index,
}: {
accountType: Account['accountType'];
symbol: Account['symbol'];
index?: number;
}) => string;
}: GetDefaultAccountLabelParams) => string;
supportedCryptoIds: Set<CryptoId> | undefined;
tokenDefinitions: Partial<TokenDefinitionsState>;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ import * as BUY_FIXTURE from 'src/utils/wallet/coinmarket/__fixtures__/buyUtils'
import * as SELL_FIXTURE from 'src/utils/wallet/coinmarket/__fixtures__/sellUtils';
import * as EXCHANGE_FIXTURE from 'src/utils/wallet/coinmarket/__fixtures__/exchangeUtils';
import { CryptoId } from 'invity-api';
import { useAccountLabel } from 'src/components/suite/AccountLabel';
import { useDefaultAccountLabel } from 'src/hooks/suite/useDefaultAccountLabel';

jest.mock('src/components/suite/AccountLabel', () => ({
...jest.requireActual('src/components/suite/AccountLabel'),
useAccountLabel: jest.fn(),
jest.mock('src/hooks/suite/useDefaultAccountLabel', () => ({
...jest.requireActual('src/hooks/suite/useDefaultAccountLabel'),
useDefaultAccountLabel: jest.fn(),
}));

describe('coinmarket utils', () => {
Expand Down Expand Up @@ -158,15 +158,15 @@ describe('coinmarket utils', () => {

it('coinmarketBuildAccountOptions', () => {
const label = 'mocked label';
const defaultAccountLabelString = (useAccountLabel as jest.Mock).mockImplementation(
const getDefaultAccountLabel = (useDefaultAccountLabel as jest.Mock).mockImplementation(
() => label,
);

const sortedAccounts = coinmarketBuildAccountOptions({
accounts: FIXTURE_ACCOUNTS as Account[],
deviceState: '1stTestnetAddress@device_id:0',
accountLabels: {},
defaultAccountLabelString,
getDefaultAccountLabel,
tokenDefinitions: { eth: { coin: coinDefinitions } },
supportedCryptoIds: new Set([
'bitcoin',
Expand Down
4 changes: 2 additions & 2 deletions packages/suite/src/utils/wallet/coinmarket/coinmarketUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ export const coinmarketBuildAccountOptions = ({
accountLabels,
tokenDefinitions,
supportedCryptoIds,
defaultAccountLabelString,
getDefaultAccountLabel,
}: CoinmarketBuildAccountOptionsProps): CoinmarketAccountsOptionsGroupProps[] => {
const accountsSorted = coinmarketGetSortedAccounts({
accounts,
Expand All @@ -334,7 +334,7 @@ export const coinmarketBuildAccountOptions = ({

const groupLabel =
accountLabels[account.key] ??
defaultAccountLabelString({
getDefaultAccountLabel({
accountType,
symbol: accountSymbol,
index,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ describe('account utils', () => {
'zpub6rszzdAK6RuafeRwyN8z1cgWcXCuKbLmjjfnrW4fWKtcoXQ8787214pNJjnBG5UATyghuNzjn6Lfp5k5xymrLFJnCy46bMYJPyZsbpFGagT',
),
).toBe(true);
expect(accountSearchFn(btcAcc, '#1', undefined, 'Bitcoin #1')).toBe(true);
});

it('getNetworkAccountFeatures', () => {
Expand Down
Loading