Prevent account name collisions (#16752)

* dealt with most of the problems in the Create Account dialog * Fixed "newAccountNumberName" localizations * In another language, don't allow accounts named, for instance, Cuenta 3 * Editing an account name later now follows the same rules * Fixing lint errors * Responding to the review by @adonesky1 * Worked with @montelaidev to alter the RegExp, in order to catch spaces before and after the account name * Fixed line breaks for eslint
2024-12-23 01:39:44 +01:00 · 2022-12-22 09:27:31 -08:00 · 2022-12-22 09:27:31 -08:00 · b9d9112b97
commit b9d9112b97
parent 3cf5ef642f
13 changed files with 143 additions and 121 deletions
--- a/app/_locales/am/messages.json
+++ b/app/_locales/am/messages.json
@ -544,7 +544,7 @@
    "message": "አዲስ መለያ"
  },
  "newAccountNumberName": {
-    "message": "መለያ$1",
+    "message": "መለያ $1",
    "description": "Default name of next account to be created on create account screen"
  },
  "newContact": {
--- a/app/_locales/en/messages.json
+++ b/app/_locales/en/messages.json
@ -144,6 +144,10 @@
    "message": "This account name already exists",
    "description": "This is an error message shown when the user enters a new account name that matches an existing account name"
  },
+  "accountNameReserved": {
+    "message": "This account name is reserved",
+    "description": "This is an error message shown when the user enters a new account name that is reserved for future use"
+  },
  "accountOptions": {
    "message": "Account options"
  },
--- a/app/_locales/fa/messages.json
+++ b/app/_locales/fa/messages.json
@ -559,7 +559,7 @@
    "message": "حساب جدید"
  },
  "newAccountNumberName": {
-    "message": "حساب 1$1",
+    "message": "حساب $1",
    "description": "Default name of next account to be created on create account screen"
  },
  "newContact": {
--- a/app/_locales/no/messages.json
+++ b/app/_locales/no/messages.json
@ -546,7 +546,7 @@
    "message": "Ny konto "
  },
  "newAccountNumberName": {
-    "message": "Konto $1 ",
+    "message": "Konto $1",
    "description": "Default name of next account to be created on create account screen"
  },
  "newContact": {
--- a/app/_locales/ta/messages.json
+++ b/app/_locales/ta/messages.json
@ -282,7 +282,7 @@
    "message": "புதிய கணக்கு"
  },
  "newAccountNumberName": {
-    "message": "கணக்கு $ 1",
+    "message": "கணக்கு $1",
    "description": "Default name of next account to be created on create account screen"
  },
  "newContract": {
--- a/package.json
+++ b/package.json
@ -41,8 +41,8 @@
    "sentry:publish": "node ./development/sentry-publish.js",
    "lint": "yarn lint:prettier && yarn lint:eslint && yarn lint:tsc && yarn lint:styles",
    "lint:fix": "yarn lint:prettier:fix && yarn lint:eslint:fix && yarn lint:styles:fix",
-    "lint:prettier": "prettier '**/*.json' --check",
-    "lint:prettier:fix": "prettier '**/*.json' --write",
+    "lint:prettier": "prettier --check -- **/*.json",
+    "lint:prettier:fix": "prettier --write -- **/*.json",
    "lint:changed": "{ git ls-files --others --exclude-standard ; git diff-index --name-only --diff-filter=d HEAD ; } | grep --regexp='[.]js$' | tr '\\n' '\\0' | xargs -0 eslint",
    "lint:changed:fix": "{ git ls-files --others --exclude-standard ; git diff-index --name-only --diff-filter=d HEAD ; } | grep --regexp='[.]js$' | tr '\\n' '\\0' | xargs -0 eslint --fix",
    "lint:changelog": "auto-changelog validate",
@ -53,7 +53,7 @@
    "lint:lockfile:dedupe:fix": "yarn dedupe",
    "lint:lockfile": "lockfile-lint --path yarn.lock --allowed-hosts npm yarn github.com codeload.github.com --empty-hostname true --allowed-schemes \"https:\" \"git+https:\" \"npm:\" \"patch:\" \"workspace:\"",
    "lint:shellcheck": "./development/shellcheck.sh",
-    "lint:styles": "stylelint '*/**/*.scss'",
+    "lint:styles": "stylelint -- */**/*.scss",
    "lint:styles:fix": "yarn lint:styles --fix",
    "lint:tsc": "tsc --project tsconfig.json --noEmit",
    "validate-source-maps": "node ./development/sourcemap-validator.js",
--- a/ui/components/app/modals/account-details-modal/account-details-modal.component.js
+++ b/ui/components/app/modals/account-details-modal/account-details-modal.component.js
@ -41,7 +41,6 @@ export default class AccountDetailsModal extends Component {
      setAccountLabel,
      keyrings,
      rpcPrefs,
-      accounts,
      history,
      hideModal,
      blockExplorerLinkText,
@ -52,12 +51,6 @@ export default class AccountDetailsModal extends Component {
      return kr.accounts.includes(address);
    });

-    const getAccountsNames = (allAccounts, currentName) => {
-      return Object.values(allAccounts)
-        .map((item) => item.name)
-        .filter((itemName) => itemName !== currentName);
-    };
-
    let exportPrivateKeyFeatureEnabled = true;
    // This feature is disabled for hardware wallets
    if (isHardwareKeyring(keyring?.type)) {
@ -91,7 +84,7 @@ export default class AccountDetailsModal extends Component {
          className="account-details-modal__name"
          defaultValue={name}
          onSubmit={(label) => setAccountLabel(address, label)}
-          accountsNames={getAccountsNames(accounts, name)}
+          accounts={this.props.accounts}
        />

        <QrView
@ -111,15 +104,12 @@ export default class AccountDetailsModal extends Component {
              : openBlockExplorer
          }
        >
-          {this.context.t(
-            blockExplorerLinkText.firstPart,
-            blockExplorerLinkText.secondPart === ''
-              ? null
-              : [blockExplorerLinkText.secondPart],
-          )}
+          {this.context.t(blockExplorerLinkText.firstPart, [
+            blockExplorerLinkText.secondPart,
+          ])}
        </Button>

-        {exportPrivateKeyFeatureEnabled ? (
+        {exportPrivateKeyFeatureEnabled && (
          <Button
            type="secondary"
            className="account-details-modal__button"
@ -137,7 +127,7 @@ export default class AccountDetailsModal extends Component {
          >
            {this.context.t('exportPrivateKey')}
          </Button>
-        ) : null}
+        )}
      </AccountModalContainer>
    );
  }
--- a/ui/components/ui/editable-label/editable-label.js
+++ b/ui/components/ui/editable-label/editable-label.js
@ -1,13 +1,14 @@
 import classnames from 'classnames';
 import PropTypes from 'prop-types';
 import React, { Component } from 'react';
+import { getAccountNameErrorMessage } from '../../../helpers/utils/accounts';

-class EditableLabel extends Component {
+export default class EditableLabel extends Component {
  static propTypes = {
    onSubmit: PropTypes.func.isRequired,
    defaultValue: PropTypes.string,
    className: PropTypes.string,
-    accountsNames: PropTypes.array,
+    accounts: PropTypes.array,
  };

  static contextTypes = {
@ -19,91 +20,71 @@ class EditableLabel extends Component {
    value: this.props.defaultValue || '',
  };

-  handleSubmit() {
-    const { value } = this.state;
-    const { accountsNames } = this.props;
-
-    if (value === '' || accountsNames.includes(value)) {
+  async handleSubmit(isValidAccountName) {
+    if (!isValidAccountName) {
      return;
    }

-    Promise.resolve(this.props.onSubmit(value)).then(() =>
-      this.setState({ isEditing: false }),
-    );
+    await this.props.onSubmit(this.state.value);
+    this.setState({ isEditing: false });
  }

  renderEditing() {
-    const { value } = this.state;
-    const { accountsNames } = this.props;
+    const { isValidAccountName, errorMessage } = getAccountNameErrorMessage(
+      this.props.accounts,
+      this.context,
+      this.state.value,
+      this.props.defaultValue,
+    );

-    return [
-      <input
-        key={1}
-        type="text"
-        required
-        dir="auto"
-        value={this.state.value}
-        onKeyPress={(event) => {
-          if (event.key === 'Enter') {
-            this.handleSubmit();
-          }
-        }}
-        onChange={(event) => this.setState({ value: event.target.value })}
-        data-testid="editable-input"
-        className={classnames('large-input', 'editable-label__input', {
-          'editable-label__input--error':
-            value === '' || accountsNames.includes(value),
-        })}
-        autoFocus
-      />,
-      <button
-        className="editable-label__icon-button"
-        key={2}
-        onClick={() => this.handleSubmit()}
-      >
-        <i className="fa fa-check editable-label__icon" />
-      </button>,
-    ];
+    return (
+      <div className={classnames('editable-label', this.props.className)}>
+        <input
+          type="text"
+          required
+          dir="auto"
+          value={this.state.value}
+          onKeyPress={(event) => {
+            if (event.key === 'Enter') {
+              this.handleSubmit(isValidAccountName);
+            }
+          }}
+          onChange={(event) => this.setState({ value: event.target.value })}
+          data-testid="editable-input"
+          className={classnames('large-input', 'editable-label__input', {
+            'editable-label__input--error': !isValidAccountName,
+          })}
+          autoFocus
+        />
+        <button
+          className="editable-label__icon-button"
+          onClick={() => this.handleSubmit(isValidAccountName)}
+        >
+          <i className="fa fa-check editable-label__icon" />
+        </button>
+        <div className="editable-label__error editable-label__error-amount">
+          {errorMessage}
+        </div>
+      </div>
+    );
  }

  renderReadonly() {
-    return [
-      <div key={1} className="editable-label__value">
-        {this.state.value}
-      </div>,
-      <button
-        key={2}
-        className="editable-label__icon-button"
-        data-testid="editable-label-button"
-        onClick={() => this.setState({ isEditing: true })}
-      >
-        <i className="fas fa-pencil-alt editable-label__icon" />
-      </button>,
-    ];
+    return (
+      <div className={classnames('editable-label', this.props.className)}>
+        <div className="editable-label__value">{this.state.value}</div>
+        <button
+          className="editable-label__icon-button"
+          data-testid="editable-label-button"
+          onClick={() => this.setState({ isEditing: true })}
+        >
+          <i className="fas fa-pencil-alt editable-label__icon" />
+        </button>
+      </div>
+    );
  }

  render() {
-    const { isEditing, value } = this.state;
-    const { className, accountsNames } = this.props;
-
-    return (
-      <>
-        <div className={classnames('editable-label', className)}>
-          {isEditing ? this.renderEditing() : this.renderReadonly()}
-        </div>
-        {accountsNames.includes(value) ? (
-          <div
-            className={classnames(
-              'editable-label__error',
-              'editable-label__error-amount',
-            )}
-          >
-            {this.context.t('accountNameDuplicate')}
-          </div>
-        ) : null}
-      </>
-    );
+    return this.state.isEditing ? this.renderEditing() : this.renderReadonly();
  }
 }
-
-export default EditableLabel;
--- a/ui/components/ui/editable-label/editable-label.stories.js
+++ b/ui/components/ui/editable-label/editable-label.stories.js
@ -14,13 +14,20 @@ export default {
    className: {
      control: 'text',
    },
-    accountsNames: {
+    accounts: {
      control: 'array',
    },
  },
  args: {
    defaultValue: 'Account 3',
-    accountsNames: ['Account 1', 'Account 2'],
+    accounts: [
+      {
+        name: 'Account 1',
+      },
+      {
+        name: 'Account 2',
+      },
+    ],
  },
 };

--- a/ui/components/ui/editable-label/index.scss
+++ b/ui/components/ui/editable-label/index.scss
@ -3,6 +3,7 @@
  align-items: center;
  justify-content: center;
  position: relative;
+  flex-flow: wrap;

  &__value {
    max-width: 250px;
@ -26,7 +27,6 @@
  }

  &__icon-button {
-    position: absolute;
    margin-left: 10px;
    left: 100%;
    background: unset;
@ -42,6 +42,8 @@

    left: 8px;
    color: var(--color-error-default);
+    width: 100%;
+    text-align: center;
  }

  &__error-amount {
--- a/ui/helpers/utils/accounts.js
+++ b/ui/helpers/utils/accounts.js
@ -0,0 +1,37 @@
+export function getAccountNameErrorMessage(
+  accounts,
+  context,
+  newAccountName,
+  defaultAccountName,
+) {
+  const isDuplicateAccountName = accounts.some(
+    (item) => item.name === newAccountName,
+  );
+
+  const localizedWordForAccount = context
+    .t('newAccountNumberName')
+    .replace(' $1', '');
+
+  // Match strings starting with ${localizedWordForAccount} and then any numeral, case insensitive
+  // Trim spaces before and after
+  const reservedRegEx = new RegExp(
+    `^\\s*${localizedWordForAccount} \\d+\\s*$`,
+    'iu',
+  );
+  const isReservedAccountName = reservedRegEx.test(newAccountName);
+
+  const isValidAccountName =
+    newAccountName === defaultAccountName || // What is written in the text field is the same as the placeholder
+    (!isDuplicateAccountName && !isReservedAccountName);
+
+  let errorMessage;
+  if (isValidAccountName) {
+    errorMessage = '\u200d'; // This is Unicode for an invisible character, so the spacing stays constant
+  } else if (isDuplicateAccountName) {
+    errorMessage = context.t('accountNameDuplicate');
+  } else if (isReservedAccountName) {
+    errorMessage = context.t('accountNameReserved');
+  }
+
+  return { isValidAccountName, errorMessage };
+}
--- a/ui/pages/create-account/index.scss
+++ b/ui/pages/create-account/index.scss
@ -84,13 +84,18 @@
    border: 1px solid var(--color-border-muted);
    border-radius: 4px;
    background-color: var(--color-background-default);
-    color: var(--color-text-muted);
+    color: var(--color-text-default);
    margin-top: 15px;
    padding: 0 20px;

    &__error {
      border: 1px solid var(--color-error-alternative);
    }
+
+    &::placeholder {
+      color: var(--color-text-muted);
+      opacity: 1;
+    }
  }

  &__error {
@ -105,7 +110,7 @@
  }

  &__buttons {
-    margin-top: 39px;
+    margin-top: 22px;
    display: flex;
    width: 100%;
    justify-content: space-between;
--- a/ui/pages/create-account/new-account.component.js
+++ b/ui/pages/create-account/new-account.component.js
@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import classnames from 'classnames';
 import Button from '../../components/ui/button';
 import { EVENT, EVENT_NAMES } from '../../../shared/constants/metametrics';
+import { getAccountNameErrorMessage } from '../../helpers/utils/accounts';

 export default class NewAccountCreateForm extends Component {
  static defaultProps = {
@ -46,11 +47,12 @@ export default class NewAccountCreateForm extends Component {
        });
    };

-    const accountNameExists = (allAccounts, accountName) => {
-      return Boolean(allAccounts.find((item) => item.name === accountName));
-    };
-
-    const existingAccountName = accountNameExists(accounts, newAccountName);
+    const { isValidAccountName, errorMessage } = getAccountNameErrorMessage(
+      accounts,
+      this.context,
+      newAccountName,
+      defaultAccountName,
+    );

    return (
      <div className="new-account-create-form">
@ -59,8 +61,9 @@ export default class NewAccountCreateForm extends Component {
        </div>
        <div>
          <input
-            className={classnames('new-account-create-form__input', {
-              'new-account-create-form__input__error': existingAccountName,
+            className={classnames({
+              'new-account-create-form__input': true,
+              'new-account-create-form__input__error': !isValidAccountName,
            })}
            value={newAccountName}
            placeholder={defaultAccountName}
@ -69,16 +72,9 @@ export default class NewAccountCreateForm extends Component {
            }
            autoFocus
          />
-          {existingAccountName ? (
-            <div
-              className={classnames(
-                ' new-account-create-form__error',
-                ' new-account-create-form__error-amount',
-              )}
-            >
-              {this.context.t('accountNameDuplicate')}
-            </div>
-          ) : null}
+          <div className="new-account-create-form__error new-account-create-form__error-amount">
+            {errorMessage}
+          </div>
          <div className="new-account-create-form__buttons">
            <Button
              type="secondary"
@ -93,7 +89,7 @@ export default class NewAccountCreateForm extends Component {
              large
              className="new-account-create-form__button"
              onClick={createClick}
-              disabled={existingAccountName}
+              disabled={!isValidAccountName}
            >
              {this.context.t('create')}
            </Button>