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

Hide 'Add snap account' button behind an experimental setting #20974

Merged
merged 20 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
38d1da5
feat: hide 'Add Snap Account' button behind an experimental setting
danroc Sep 19, 2023
1c81f39
test: update tests to support the "add snap account button" toggle
danroc Sep 20, 2023
a2a04c5
Merge branch 'develop' into feature/snap-keyring-switch
danroc Sep 20, 2023
576f465
ui: fix buttons margins in accounts list
danroc Sep 20, 2023
ba8ee07
Merge branch 'develop' into feature/snap-keyring-switch
danroc Sep 21, 2023
484a0d3
test: add e2e test to hide the "Add snap account" button in settings
danroc Sep 21, 2023
966a62c
ui: fix margin in the MMI account button
danroc Sep 21, 2023
7385401
test: update experimental tab snapshot
danroc Sep 21, 2023
a48cb74
test: don't run e2e test in stable for now
danroc Sep 21, 2023
ffafdd6
Merge branch 'develop' into feature/snap-keyring-switch
danroc Sep 21, 2023
fff5e3f
test: move test to snaps folder
danroc Sep 21, 2023
ea90aca
Merge branch 'develop' into feature/snap-keyring-switch
danroc Sep 21, 2023
640691b
Revert "test: move test to snaps folder"
danroc Sep 21, 2023
06873fd
test: also filter tests from other folders
danroc Sep 21, 2023
7573e5c
chore: revert capitalization change
danroc Sep 21, 2023
b899129
Merge branch 'develop' into feature/snap-keyring-switch
danroc Sep 21, 2023
e607c03
chore: change "Accounts" -> "accounts"
danroc Sep 21, 2023
03ac55e
test: update test snapshot
danroc Sep 21, 2023
e474aab
fixup! test: also filter tests from other folders
danroc Sep 21, 2023
373d7f0
Merge branch 'develop' into feature/snap-keyring-switch
danroc Sep 21, 2023
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
12 changes: 12 additions & 0 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions app/scripts/controllers/preferences.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ export default class PreferencesController {
///: BEGIN:ONLY_INCLUDE_IN(blockaid)
securityAlertsEnabled: false,
///: END:ONLY_INCLUDE_IN
///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
addSnapAccountEnabled: false,
///: END:ONLY_INCLUDE_IN
advancedGasFee: {},

// WARNING: Do not use feature flags for security-sensitive things.
Expand Down Expand Up @@ -240,6 +243,20 @@ export default class PreferencesController {
}
///: END:ONLY_INCLUDE_IN

///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
/**
* Setter for the `addSnapAccountEnabled` property.
*
* @param {boolean} addSnapAccountEnabled - Whether or not the user wants to
* enable the "Add Snap accounts" button.
*/
setAddSnapAccountEnabled(addSnapAccountEnabled) {
this.store.updateState({
addSnapAccountEnabled,
});
}
///: END:ONLY_INCLUDE_IN

/**
* Setter for the `advancedGasFee` property
*
Expand Down
6 changes: 6 additions & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -2372,6 +2372,12 @@ export default class MetamaskController extends EventEmitter {
preferencesController,
),
///: END:ONLY_INCLUDE_IN
///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
setAddSnapAccountEnabled:
preferencesController.setAddSnapAccountEnabled.bind(
preferencesController,
),
///: END:ONLY_INCLUDE_IN
setIpfsGateway: preferencesController.setIpfsGateway.bind(
preferencesController,
),
Expand Down
3 changes: 3 additions & 0 deletions shared/constants/metametrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,9 @@ export enum MetaMetricsEventName {
SnapUpdated = 'Snap Updated',
SnapExportUsed = 'Snap Export Used',
///: END:ONLY_INCLUDE_IN
///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
AddSnapAccountEnabled = 'Add Snap Account Enabled',
///: END:ONLY_INCLUDE_IN
}

export enum MetaMetricsEventAccountType {
Expand Down
1 change: 1 addition & 0 deletions test/data/mock-state.json
Original file line number Diff line number Diff line change
Expand Up @@ -1380,6 +1380,7 @@
}
],
"desktopEnabled": false,
"addSnapAccountEnabled": false,
"pendingApprovals": {
"testApprovalId": {
"id": "testApprovalId",
Expand Down
29 changes: 15 additions & 14 deletions test/e2e/run-all.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,20 +101,6 @@ async function main() {
if (snaps) {
const testDir = path.join(__dirname, 'snaps');
testPaths = await getTestPathsForTestDir(testDir);

if (buildType && buildType !== 'flask') {
// These tests should only be ran on Flask for now
const filteredTests = [
'test-snap-manageAccount.spec.js',
'test-snap-rpc.spec.js',
'test-snap-lifecycle.spec.js',
'ppom-toggle-settings.spec.js',
'petnames.spec.js',
];
testPaths = testPaths.filter((p) =>
filteredTests.every((filteredTest) => !p.endsWith(filteredTest)),
);
}
} else if (rpc) {
const testDir = path.join(__dirname, 'json-rpc');
testPaths = await getTestPathsForTestDir(testDir);
Expand All @@ -135,6 +121,21 @@ async function main() {
}
}

// These tests should only be ran on Flask for now.
if (buildType !== 'flask') {
const filteredTests = [
'settings-add-snap-account-toggle.spec.js',
'test-snap-manageAccount.spec.js',
'test-snap-rpc.spec.js',
'test-snap-lifecycle.spec.js',
'ppom-toggle-settings.spec.js',
'petnames.spec.js',
];
testPaths = testPaths.filter((p) =>
filteredTests.every((filteredTest) => !p.endsWith(filteredTest)),
);
}

const runE2eTestPath = path.join(__dirname, 'run-e2e-test.js');

const args = [runE2eTestPath];
Expand Down
52 changes: 52 additions & 0 deletions test/e2e/tests/settings-add-snap-account-toggle.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
const { strict: assert } = require('assert');
const { withFixtures } = require('../helpers');
const FixtureBuilder = require('../fixture-builder');

describe('Add snap account experimental settings', function () {
it('switch "Enable Add snap account" to on', async function () {
await withFixtures(
{
fixtures: new FixtureBuilder().build(),
title: this.test.title,
failOnConsoleError: false,
},
async ({ driver }) => {
await driver.navigate();
await driver.fill('#password', 'correct horse battery staple');
Copy link
Contributor

Choose a reason for hiding this comment

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

This line and the next can be replaced by await unlockWallet(driver); going forward

await driver.press('#password', driver.Key.ENTER);

// Make sure the "Add snap account" button is not visible.
await driver.clickElement('[data-testid="account-menu-icon"]');
await driver.assertElementNotPresent({
text: 'Add snap account',
tag: 'button',
});
await driver.clickElement('.mm-box button[aria-label="Close"]');

Copy link
Contributor

Choose a reason for hiding this comment

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

For the ease of troubleshooting potential test failures going forward, will recommend that we consider separating this single block into two.

it ('add snap account disabled by default`...
and
it ('add snap account after experimental setting enabled'....

// Navigate to experimental settings.
await driver.clickElement(
'[data-testid="account-options-menu-button"]',
);
await driver.clickElement({ text: 'Settings', tag: 'div' });
await driver.clickElement({ text: 'Experimental', tag: 'div' });

// Switch "Enable Add snap account" to on.
const toggle = await driver.findElement(
'[data-testid="add-snap-account-toggle"]',
);
await driver.scrollToElement(toggle);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably turn this into a fixure and save test steps here.

await driver.clickElement('[data-testid="add-snap-account-toggle"]');

// Make sure the "Add snap account" button is visible.
await driver.clickElement('[data-testid="account-menu-icon"]');
assert.equal(
await driver.isElementPresentAndVisible({
text: 'Add snap account',
tag: 'button',
}),
true,
);
},
);
});
});
18 changes: 12 additions & 6 deletions ui/components/multichain/account-list-menu/account-list-menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ import {
getMetaMaskAccountsOrdered,
getConnectedSubjectsForAllAddresses,
getOriginOfCurrentTab,
///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
getIsAddSnapAccountEnabled,
///: END:ONLY_INCLUDE_IN
} from '../../../selectors';
import { toggleAccountMenu, setSelectedAccount } from '../../../store/actions';
import {
Expand Down Expand Up @@ -57,6 +60,9 @@ export const AccountListMenu = ({ onClose }) => {
const currentTabOrigin = useSelector(getOriginOfCurrentTab);
const history = useHistory();
const dispatch = useDispatch();
///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
const addSnapAccountEnabled = useSelector(getIsAddSnapAccountEnabled);
///: END:ONLY_INCLUDE_IN

const [searchQuery, setSearchQuery] = useState('');
const [actionMode, setActionMode] = useState('');
Expand Down Expand Up @@ -198,7 +204,7 @@ export const AccountListMenu = ({ onClose }) => {
</Box>
{/* Add / Import / Hardware */}
<Box padding={4}>
<Box marginBottom={4}>
<Box>
<ButtonLink
size={Size.SM}
startIconName={IconName.Add}
Expand All @@ -218,7 +224,7 @@ export const AccountListMenu = ({ onClose }) => {
{t('addAccount')}
</ButtonLink>
</Box>
<Box marginBottom={4}>
<Box marginTop={4}>
<ButtonLink
size={Size.SM}
startIconName={IconName.Import}
Expand All @@ -237,7 +243,7 @@ export const AccountListMenu = ({ onClose }) => {
{t('importAccount')}
</ButtonLink>
</Box>
<Box marginBottom={4}>
<Box marginTop={4}>
<ButtonLink
size={Size.SM}
startIconName={IconName.Hardware}
Expand Down Expand Up @@ -265,7 +271,7 @@ export const AccountListMenu = ({ onClose }) => {
</Box>
{
///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
<>
addSnapAccountEnabled && (
<Box marginTop={4}>
<ButtonLink
size={Size.SM}
Expand All @@ -284,12 +290,12 @@ export const AccountListMenu = ({ onClose }) => {
{t('settingAddSnapAccount')}
</ButtonLink>
</Box>
</>
)
///: END:ONLY_INCLUDE_IN
}
{
///: BEGIN:ONLY_INCLUDE_IN(build-mmi)
<Box>
<Box marginTop={4}>
<ButtonLink
size={Size.SM}
startIconName={IconName.Custody}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,29 +158,58 @@ describe('AccountListMenu', () => {
});

///: BEGIN:ONLY_INCLUDE_IN(keyring-snaps)
it('renders the add snap account button', async () => {
const { getByText } = render();
const addSnapAccountButton = getByText(
messages.settingAddSnapAccount.message,
);
expect(addSnapAccountButton).toBeInTheDocument();

fireEvent.click(addSnapAccountButton);
describe('addSnapAccountButton', () => {
const renderWithState = (state, props = { onClose: () => jest.fn() }) => {
const store = configureStore({
...mockState,
...{
metamask: {
...mockState.metamask,
...state,
},
},
activeTab: {
id: 113,
title: 'E2E Test Dapp',
origin: 'https://metamask.github.io',
protocol: 'https:',
url: 'https://metamask.github.io/test-dapp/',
},
});
return renderWithProvider(<AccountListMenu {...props} />, store);
};

it("doesn't render the add snap account button if it's disabled", async () => {
const { getByText } = renderWithState({ addSnapAccountEnabled: false });
expect(() => getByText(messages.settingAddSnapAccount.message)).toThrow(
`Unable to find an element with the text: ${messages.settingAddSnapAccount.message}`,
);
});

await waitFor(() => {
expect(mockToggleAccountMenu).toHaveBeenCalled();
it("renders the add snap account button if it's enabled", async () => {
const { getByText } = renderWithState({ addSnapAccountEnabled: true });
const addSnapAccountButton = getByText(
messages.settingAddSnapAccount.message,
);
expect(addSnapAccountButton).toBeInTheDocument();

fireEvent.click(addSnapAccountButton);
await waitFor(() => {
expect(mockToggleAccountMenu).toHaveBeenCalled();
});
});
});

it('pushes history when clicking add snap account from extended view', async () => {
const { getByText } = render();
mockGetEnvironmentType.mockReturnValueOnce('fullscreen');
const addSnapAccountButton = getByText(
messages.settingAddSnapAccount.message,
);
fireEvent.click(addSnapAccountButton);
await waitFor(() => {
expect(historyPushMock).toHaveBeenCalledWith(ADD_SNAP_ACCOUNT_ROUTE);
it('pushes history when clicking add snap account from extended view', async () => {
const { getByText } = renderWithState({ addSnapAccountEnabled: true });
mockGetEnvironmentType.mockReturnValueOnce('fullscreen');
const addSnapAccountButton = getByText(
messages.settingAddSnapAccount.message,
);

fireEvent.click(addSnapAccountButton);
await waitFor(() => {
expect(historyPushMock).toHaveBeenCalledWith(ADD_SNAP_ACCOUNT_ROUTE);
});
});
});
///: END:ONLY_INCLUDE_IN
Expand Down
Loading
Loading