Skip to content

Commit

Permalink
refactor: streamline SmartTransactionsBannerAlert logic
Browse files Browse the repository at this point in the history
- Replace inline Redux selector with `getSmartTransactionsOptInStatusInternal`
- Add `getSmartTransactionsMigrationAppliedInternal` for consistent state handling
- Centralize banner visibility logic in `alertConditions` for clarity
- Consolidate alert dismissal logic in `dismissAlert` and remove redundant `dispatch`
- Simplify state typing and remove unused types
- Merge duplicate dismissal tests for shared functionality

Alert state is now managed via AlertController's `setAlertEnabledness`. Selectors are reusable, logic is more maintainable, and tests better reflect component behavior.
  • Loading branch information
httpJunkie committed Dec 19, 2024
1 parent ab5c1d7 commit 105d90d
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 57 deletions.
20 changes: 20 additions & 0 deletions shared/modules/selectors/smart-transactions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type SmartTransactionsMetaMaskState = {
metamask: {
preferences: {
smartTransactionsOptInStatus?: boolean;
smartTransactionsMigrationApplied?: boolean;
};
internalAccounts: {
selectedAccount: string;
Expand Down Expand Up @@ -72,6 +73,25 @@ export const getSmartTransactionsOptInStatusInternal = createSelector(
},
);

/**
* Returns whether the smart transactions migration has been applied to the user's settings.
* This specifically tracks if Migration 135 has been run, which enables Smart Transactions
* by default for users who have never interacted with the feature or who previously opted out
* with no STX activity.
*
* This should only be used for internal checks of the migration status, and not
* for determining overall Smart Transactions availability.
*
* @param state - The state object.
* @returns true if the migration has been applied to the user's settings, false if not or if unset.
*/
export const getSmartTransactionsMigrationAppliedInternal = createSelector(
getPreferences,
(preferences: { smartTransactionsMigrationApplied?: boolean }): boolean => {
return preferences?.smartTransactionsMigrationApplied ?? false;
},
);

/**
* Returns the user's explicit opt-in status for the smart transactions feature.
* This should only be used for metrics collection, and not for determining if the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,24 +117,24 @@ describe('SmartTransactionsBannerAlert', () => {
).not.toBeInTheDocument();
});

it('calls setAlertEnabledness when close button clicked', () => {
it('dismisses banner when close button or link is clicked', () => {
const store = configureStore(mockState);
renderWithProvider(<SmartTransactionsBannerAlert />, store);

// Test close button
const { unmount } = renderWithProvider(<SmartTransactionsBannerAlert />, store);
screen.getByRole('button', { name: /close/iu }).click();

expect(setAlertEnabledness).toHaveBeenCalledWith(
AlertTypes.smartTransactionsMigration,
false,
);
});

it('calls setAlertEnabledness when (Higher success rates) link clicked', () => {
const store = configureStore(mockState);
renderWithProvider(<SmartTransactionsBannerAlert />, store);
// Cleanup
unmount();
jest.clearAllMocks();

// Test link
renderWithProvider(<SmartTransactionsBannerAlert />, store);
screen.getByText('smartTransactionsEnabledLink').click();

expect(setAlertEnabledness).toHaveBeenCalledWith(
AlertTypes.smartTransactionsMigration,
false,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { useCallback } from 'react';
import { useSelector, useDispatch } from 'react-redux';
import { useSelector } from 'react-redux';
import { TransactionType } from '@metamask/transaction-controller';
import { useI18nContext } from '../../../../hooks/useI18nContext';
import {
Expand All @@ -14,30 +14,19 @@ import { SMART_TRANSACTIONS_LEARN_MORE_URL } from '../../../../../shared/constan
import { FontWeight } from '../../../../helpers/constants/design-system';
import { useConfirmContext } from '../../context/confirm';
import { isCorrectDeveloperTransactionType } from '../../../../../shared/lib/confirmation.utils';
import {
getSmartTransactionsOptInStatusInternal,
getSmartTransactionsMigrationAppliedInternal,
} from '../../../../../shared/modules/selectors/smart-transactions';

type MarginType = 'default' | 'none' | 'noTop' | 'onlyTop';

type SmartTransactionsBannerAlertProps = {
marginType?: MarginType;
};

type MetaMaskState = {
alertEnabledness: {
[key: string]: boolean;
};
preferences: {
smartTransactionsOptInStatus: boolean;
smartTransactionsMigrationApplied: boolean;
};
};

type RootState = {
metamask: MetaMaskState;
};

export const SmartTransactionsBannerAlert: React.FC<SmartTransactionsBannerAlertProps> =
React.memo(({ marginType = 'default' }) => {
const dispatch = useDispatch();
const t = useI18nContext();

let currentConfirmation;
Expand All @@ -49,54 +38,41 @@ export const SmartTransactionsBannerAlert: React.FC<SmartTransactionsBannerAlert
}

const alertEnabled = useSelector(
(state: RootState) =>
(state: {
metamask: { alertEnabledness?: { [key: string]: boolean } };
}) =>
state.metamask.alertEnabledness?.[
AlertTypes.smartTransactionsMigration
] !== false,
);

const smartTransactionsOptIn = useSelector(
(state: RootState) =>
state.metamask.preferences?.smartTransactionsOptInStatus === true,
getSmartTransactionsOptInStatusInternal,
);

const smartTransactionsMigrationApplied = useSelector(
(state: RootState) =>
state.metamask.preferences?.smartTransactionsMigrationApplied === true,
getSmartTransactionsMigrationAppliedInternal,
);

const dismissAlert = useCallback(() => {
setAlertEnabledness(AlertTypes.smartTransactionsMigration, false);
}, []);

React.useEffect(() => {
if (alertEnabled && !smartTransactionsOptIn) {
dispatch({
type: 'alert/dismiss',
payload: {
alertId: AlertTypes.smartTransactionsMigration,
enabled: false,
},
});
setAlertEnabledness(AlertTypes.smartTransactionsMigration, false);
dismissAlert();
}
}, [alertEnabled, smartTransactionsOptIn, dispatch]);

const handleDismiss = useCallback(() => {
dispatch({
type: 'alert/dismiss',
payload: {
alertId: AlertTypes.smartTransactionsMigration,
enabled: false,
},
});
setAlertEnabledness(AlertTypes.smartTransactionsMigration, false);
}, [dispatch]);
}, [alertEnabled, smartTransactionsOptIn, dismissAlert]);

const alertConditions =
alertEnabled &&
smartTransactionsOptIn &&
smartTransactionsMigrationApplied;

const shouldRender =
currentConfirmation === null
? alertEnabled &&
smartTransactionsOptIn &&
smartTransactionsMigrationApplied
: alertEnabled &&
smartTransactionsOptIn &&
smartTransactionsMigrationApplied &&
? alertConditions
: alertConditions &&
isCorrectDeveloperTransactionType(
currentConfirmation?.type as TransactionType,
);
Expand All @@ -121,7 +97,7 @@ export const SmartTransactionsBannerAlert: React.FC<SmartTransactionsBannerAlert
return (
<BannerAlert
severity={BannerAlertSeverity.Info}
onClose={handleDismiss}
onClose={dismissAlert}
data-testid="smart-transactions-banner-alert"
style={getMarginStyle()}
>
Expand All @@ -131,7 +107,7 @@ export const SmartTransactionsBannerAlert: React.FC<SmartTransactionsBannerAlert
<Text as="p">
<ButtonLink
href={SMART_TRANSACTIONS_LEARN_MORE_URL}
onClick={handleDismiss}
onClick={dismissAlert}
externalLink
style={{ height: 'unset', verticalAlign: 'unset' }}
>
Expand Down

0 comments on commit 105d90d

Please sign in to comment.