From f9c08a995d90e76e873d0aea17c6387d5b897abd Mon Sep 17 00:00:00 2001
From: Ryan Bianchi <1435081+qedi-r@users.noreply.github.com>
Date: Thu, 10 Oct 2024 23:53:17 -0400
Subject: [PATCH] Fix ability to have duplicate account names (#3527)

* add duplicate name validation to sidebar, and both narrow/wide Account pages

* unify error messages and styles

* keep state for the current account name on the mobile account modal

* add release notes

* remove extra validation function

* fix typo in AccountSyncSidebar params

* don't set current account name to empty string and prevent further changes on AccountMenuModal

* lint imports

* update error message to Name x already exists

* use proper translation functions

---------

Co-authored-by: youngcw <calebyoung94@gmail.com>
---
 .../src/components/accounts/Account.tsx       |  20 +-
 .../src/components/accounts/Header.jsx        | 232 +++++++++++-------
 .../components/modals/AccountMenuModal.tsx    |  52 +++-
 .../modals/CreateLocalAccountModal.tsx        |  29 ++-
 .../src/components/util/accountValidation.ts  |  23 ++
 upcoming-release-notes/3527.md                |   6 +
 6 files changed, 243 insertions(+), 119 deletions(-)
 create mode 100644 packages/desktop-client/src/components/util/accountValidation.ts
 create mode 100644 upcoming-release-notes/3527.md

diff --git a/packages/desktop-client/src/components/accounts/Account.tsx b/packages/desktop-client/src/components/accounts/Account.tsx
index 92ab8a2f3e4..db2b910ddd9 100644
--- a/packages/desktop-client/src/components/accounts/Account.tsx
+++ b/packages/desktop-client/src/components/accounts/Account.tsx
@@ -69,6 +69,7 @@ import { Button } from '../common/Button2';
 import { Text } from '../common/Text';
 import { View } from '../common/View';
 import { TransactionList } from '../transactions/TransactionList';
+import { validateAccountName } from '../util/accountValidation';
 
 import { AccountHeader } from './Header';
 
@@ -296,6 +297,7 @@ type AccountInternalState = {
   prevShowCleared?: boolean;
   showReconciled: boolean;
   editingName: boolean;
+  nameError: string;
   isAdding: boolean;
   modalShowing?: boolean;
   sort: {
@@ -343,6 +345,7 @@ class AccountInternal extends PureComponent<
       showCleared: props.showCleared,
       showReconciled: props.showReconciled,
       editingName: false,
+      nameError: '',
       isAdding: false,
       sort: null,
       filteredAmount: null,
@@ -707,13 +710,19 @@ class AccountInternal extends PureComponent<
   };
 
   onSaveName = (name: string) => {
-    if (name.trim().length) {
-      const accountId = this.props.accountId;
+    const accountNameError = validateAccountName(
+      name,
+      this.props.accountId,
+      this.props.accounts,
+    );
+    if (accountNameError) {
+      this.setState({ nameError: accountNameError });
+    } else {
       const account = this.props.accounts.find(
-        account => account.id === accountId,
-      )!;
+        account => account.id === this.props.accountId,
+      );
       this.props.updateAccount({ ...account, name });
-      this.setState({ editingName: false });
+      this.setState({ editingName: false, nameError: '' });
     }
   };
 
@@ -1708,6 +1717,7 @@ class AccountInternal extends PureComponent<
                 onAddTransaction={this.onAddTransaction}
                 onToggleExtraBalances={this.onToggleExtraBalances}
                 onSaveName={this.onSaveName}
+                saveNameError={this.state.nameError}
                 onExposeName={this.onExposeName}
                 onReconcile={this.onReconcile}
                 onDoneReconciling={this.onDoneReconciling}
diff --git a/packages/desktop-client/src/components/accounts/Header.jsx b/packages/desktop-client/src/components/accounts/Header.jsx
index ac386433cf6..319d8251f2d 100644
--- a/packages/desktop-client/src/components/accounts/Header.jsx
+++ b/packages/desktop-client/src/components/accounts/Header.jsx
@@ -1,4 +1,4 @@
-import React, { useState, useRef } from 'react';
+import React, { useState, useRef, Fragment } from 'react';
 import { useHotkeys } from 'react-hotkeys-hook';
 import { Trans, useTranslation } from 'react-i18next';
 
@@ -67,6 +67,7 @@ export function AccountHeader({
   onCreateReconciliationTransaction,
   onToggleExtraBalances,
   onSaveName,
+  saveNameError,
   onExposeName,
   onSync,
   onImport,
@@ -90,7 +91,6 @@ export function AccountHeader({
   onMakeAsNonSplitTransactions,
 }) {
   const { t } = useTranslation();
-
   const [menuOpen, setMenuOpen] = useState(false);
   const searchInput = useRef(null);
   const triggerRef = useRef(null);
@@ -176,99 +176,21 @@ export function AccountHeader({
             }}
           >
             {!!account?.bank && (
-              <View
-                style={{
-                  backgroundColor: accountsSyncing.includes(account.id)
-                    ? theme.sidebarItemBackgroundPending
-                    : failedAccounts.has(account.id)
-                      ? theme.sidebarItemBackgroundFailed
-                      : theme.sidebarItemBackgroundPositive,
-                  marginRight: '4px',
-                  width: 8,
-                  height: 8,
-                  borderRadius: 8,
-                }}
+              <AccountSyncSidebar
+                account={account}
+                failedAccounts={failedAccounts}
+                accountsSyncing={accountsSyncing}
               />
             )}
-            {editingName ? (
-              <InitialFocus>
-                <Input
-                  defaultValue={accountName}
-                  onEnter={e => onSaveName(e.target.value)}
-                  onBlur={e => onSaveName(e.target.value)}
-                  onEscape={() => onExposeName(false)}
-                  style={{
-                    fontSize: 25,
-                    fontWeight: 500,
-                    marginTop: -3,
-                    marginBottom: -4,
-                    marginLeft: -6,
-                    paddingTop: 2,
-                    paddingBottom: 2,
-                    width: Math.max(20, accountName.length) + 'ch',
-                  }}
-                />
-              </InitialFocus>
-            ) : isNameEditable ? (
-              <View
-                style={{
-                  flexDirection: 'row',
-                  alignItems: 'center',
-                  gap: 3,
-                  '& .hover-visible': {
-                    opacity: 0,
-                    transition: 'opacity .25s',
-                  },
-                  '&:hover .hover-visible': {
-                    opacity: 1,
-                  },
-                }}
-              >
-                <View
-                  style={{
-                    fontSize: 25,
-                    fontWeight: 500,
-                    marginRight: 5,
-                    marginBottom: -1,
-                  }}
-                  data-testid="account-name"
-                >
-                  {account && account.closed
-                    ? t('Closed: {{ accountName }}', { accountName })
-                    : accountName}
-                </View>
-
-                {account && (
-                  <NotesButton
-                    id={`account-${account.id}`}
-                    defaultColor={theme.pageTextSubdued}
-                  />
-                )}
-                <Button
-                  variant="bare"
-                  aria-label={t('Edit account name')}
-                  className="hover-visible"
-                  onPress={() => onExposeName(true)}
-                >
-                  <SvgPencil1
-                    style={{
-                      width: 11,
-                      height: 11,
-                      color: theme.pageTextSubdued,
-                    }}
-                  />
-                </Button>
-              </View>
-            ) : (
-              <View
-                style={{ fontSize: 25, fontWeight: 500, marginBottom: -1 }}
-                data-testid="account-name"
-              >
-                {account && account.closed
-                  ? t('Closed: {{ accountName }}', { accountName })
-                  : accountName}
-              </View>
-            )}
+            <AccountNameField
+              account={account}
+              accountName={accountName}
+              isNameEditable={isNameEditable}
+              editingName={editingName}
+              saveNameError={saveNameError}
+              onSaveName={onSaveName}
+              onExposeName={onExposeName}
+            />
           </View>
         </View>
 
@@ -470,6 +392,129 @@ export function AccountHeader({
   );
 }
 
+function AccountSyncSidebar({ account, failedAccounts, accountsSyncing }) {
+  return (
+    <View
+      style={{
+        backgroundColor: accountsSyncing.includes(account.id)
+          ? theme.sidebarItemBackgroundPending
+          : failedAccounts.has(account.id)
+            ? theme.sidebarItemBackgroundFailed
+            : theme.sidebarItemBackgroundPositive,
+        marginRight: '4px',
+        width: 8,
+        height: 8,
+        borderRadius: 8,
+      }}
+    />
+  );
+}
+
+function AccountNameField({
+  account,
+  accountName,
+  isNameEditable,
+  editingName,
+  saveNameError,
+  onSaveName,
+  onExposeName,
+}) {
+  const { t } = useTranslation();
+
+  if (editingName) {
+    return (
+      <Fragment>
+        <InitialFocus>
+          <Input
+            defaultValue={accountName}
+            onEnter={e => onSaveName(e.target.value)}
+            onBlur={e => onSaveName(e.target.value)}
+            onEscape={() => onExposeName(false)}
+            style={{
+              fontSize: 25,
+              fontWeight: 500,
+              marginTop: -3,
+              marginBottom: -4,
+              marginLeft: -6,
+              paddingTop: 2,
+              paddingBottom: 2,
+              width: Math.max(20, accountName.length) + 'ch',
+            }}
+          />
+        </InitialFocus>
+        {saveNameError && (
+          <View style={{ color: theme.warningText }}>{saveNameError}</View>
+        )}
+      </Fragment>
+    );
+  } else {
+    if (isNameEditable) {
+      return (
+        <View
+          style={{
+            flexDirection: 'row',
+            alignItems: 'center',
+            gap: 3,
+            '& .hover-visible': {
+              opacity: 0,
+              transition: 'opacity .25s',
+            },
+            '&:hover .hover-visible': {
+              opacity: 1,
+            },
+          }}
+        >
+          <View
+            style={{
+              fontSize: 25,
+              fontWeight: 500,
+              marginRight: 5,
+              marginBottom: -1,
+            }}
+            data-testid="account-name"
+          >
+            {account && account.closed
+              ? t('Closed: {{ accountName }}', { accountName })
+              : accountName}
+          </View>
+
+          {account && (
+            <NotesButton
+              id={`account-${account.id}`}
+              defaultColor={theme.pageTextSubdued}
+            />
+          )}
+          <Button
+            variant="bare"
+            aria-label={t('Edit account name')}
+            className="hover-visible"
+            onPress={() => onExposeName(true)}
+          >
+            <SvgPencil1
+              style={{
+                width: 11,
+                height: 11,
+                color: theme.pageTextSubdued,
+              }}
+            />
+          </Button>
+        </View>
+      );
+    } else {
+      return (
+        <View
+          style={{ fontSize: 25, fontWeight: 500, marginBottom: -1 }}
+          data-testid="account-name"
+        >
+          {account && account.closed
+            ? t('Closed: {{ accountName }}', { accountName })
+            : accountName}
+        </View>
+      );
+    }
+  }
+}
+
 function AccountMenu({
   account,
   canSync,
@@ -483,7 +528,6 @@ function AccountMenu({
   onMenuSelect,
 }) {
   const { t } = useTranslation();
-
   const [tooltip, setTooltip] = useState('default');
   const syncServerStatus = useSyncServerStatus();
 
diff --git a/packages/desktop-client/src/components/modals/AccountMenuModal.tsx b/packages/desktop-client/src/components/modals/AccountMenuModal.tsx
index f07a6915c44..c65fea01f3b 100644
--- a/packages/desktop-client/src/components/modals/AccountMenuModal.tsx
+++ b/packages/desktop-client/src/components/modals/AccountMenuModal.tsx
@@ -1,8 +1,10 @@
-import React, { type ComponentProps, useRef, useState } from 'react';
+import { type ComponentProps, Fragment, useRef, useState } from 'react';
+import { useTranslation } from 'react-i18next';
 
 import { type AccountEntity } from 'loot-core/types/models';
 
 import { useAccount } from '../../hooks/useAccount';
+import { useAccounts } from '../../hooks/useAccounts';
 import { useNotes } from '../../hooks/useNotes';
 import { SvgClose, SvgDotsHorizontalTriple, SvgLockOpen } from '../../icons/v1';
 import { SvgNotesPaper } from '../../icons/v2';
@@ -18,6 +20,7 @@ import {
 import { Popover } from '../common/Popover';
 import { View } from '../common/View';
 import { Notes } from '../Notes';
+import { validateAccountName } from '../util/accountValidation';
 
 type AccountMenuModalProps = {
   accountId: string;
@@ -36,19 +39,41 @@ export function AccountMenuModal({
   onEditNotes,
   onClose,
 }: AccountMenuModalProps) {
+  const { t } = useTranslation();
   const account = useAccount(accountId);
+  const accounts = useAccounts();
   const originalNotes = useNotes(`account-${accountId}`);
+  const [accountNameError, setAccountNameError] = useState('');
+  const [currentAccountName, setCurrentAccountName] = useState(
+    account?.name || t('New Account'),
+  );
 
   const onRename = (newName: string) => {
+    newName = newName.trim();
     if (!account) {
       return;
     }
+    if (!newName) {
+      setCurrentAccountName(t('Account'));
+    } else {
+      setCurrentAccountName(newName);
+    }
 
     if (newName !== account.name) {
-      onSave?.({
-        ...account,
-        name: newName,
-      });
+      const renameAccountError = validateAccountName(
+        newName,
+        accountId,
+        accounts,
+      );
+      if (renameAccountError) {
+        setAccountNameError(renameAccountError);
+      } else {
+        setAccountNameError('');
+        onSave?.({
+          ...account,
+          name: newName,
+        });
+      }
     }
   };
 
@@ -93,11 +118,18 @@ export function AccountMenuModal({
               />
             }
             title={
-              <ModalTitle
-                isEditable
-                title={account.name}
-                onTitleUpdate={onRename}
-              />
+              <Fragment>
+                <ModalTitle
+                  isEditable
+                  title={currentAccountName}
+                  onTitleUpdate={onRename}
+                />
+                {accountNameError && (
+                  <View style={{ color: theme.warningText }}>
+                    {accountNameError}
+                  </View>
+                )}
+              </Fragment>
             }
             rightContent={<ModalCloseButton onPress={close} />}
           />
diff --git a/packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx b/packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
index 7c737d30615..9b93c54a37c 100644
--- a/packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
+++ b/packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
@@ -1,11 +1,12 @@
 // @ts-strict-ignore
-import React, { type FormEvent, useState } from 'react';
+import { type FormEvent, useState } from 'react';
 import { Form } from 'react-aria-components';
 import { useDispatch } from 'react-redux';
 
 import { closeModal, createAccount } from 'loot-core/client/actions';
 import { toRelaxedNumber } from 'loot-core/src/shared/util';
 
+import * as useAccounts from '../../hooks/useAccounts';
 import { useNavigate } from '../../hooks/useNavigate';
 import { theme } from '../../style';
 import { Button } from '../common/Button2';
@@ -24,24 +25,35 @@ import {
 import { Text } from '../common/Text';
 import { View } from '../common/View';
 import { Checkbox } from '../forms';
+import { validateAccountName } from '../util/accountValidation';
 
 export function CreateLocalAccountModal() {
   const navigate = useNavigate();
   const dispatch = useDispatch();
+  const accounts = useAccounts.useAccounts();
   const [name, setName] = useState('');
   const [offbudget, setOffbudget] = useState(false);
   const [balance, setBalance] = useState('0');
 
-  const [nameError, setNameError] = useState(false);
+  const [nameError, setNameError] = useState(null);
   const [balanceError, setBalanceError] = useState(false);
 
   const validateBalance = balance => !isNaN(parseFloat(balance));
 
+  const validateAndSetName = (name: string) => {
+    const nameError = validateAccountName(name, '', accounts);
+    if (nameError) {
+      setNameError(nameError);
+    } else {
+      setName(name);
+      setNameError(null);
+    }
+  };
+
   const onSubmit = async (event: FormEvent<HTMLFormElement>) => {
     event.preventDefault();
 
-    const nameError = !name;
-    setNameError(nameError);
+    const nameError = validateAccountName(name, '', accounts);
 
     const balanceError = !validateBalance(balance);
     setBalanceError(balanceError);
@@ -72,18 +84,15 @@ export function CreateLocalAccountModal() {
                     onChange={event => setName(event.target.value)}
                     onBlur={event => {
                       const name = event.target.value.trim();
-                      setName(name);
-                      if (name && nameError) {
-                        setNameError(false);
-                      }
+                      validateAndSetName(name);
                     }}
                     style={{ flex: 1 }}
                   />
                 </InitialFocus>
               </InlineField>
               {nameError && (
-                <FormError style={{ marginLeft: 75 }}>
-                  Name is required
+                <FormError style={{ marginLeft: 75, color: theme.warningText }}>
+                  {nameError}
                 </FormError>
               )}
 
diff --git a/packages/desktop-client/src/components/util/accountValidation.ts b/packages/desktop-client/src/components/util/accountValidation.ts
new file mode 100644
index 00000000000..86ec7fe94b6
--- /dev/null
+++ b/packages/desktop-client/src/components/util/accountValidation.ts
@@ -0,0 +1,23 @@
+import { t } from 'i18next';
+
+import { type AccountEntity } from 'loot-core/types/models';
+
+export function validateAccountName(
+  newAccountName: string,
+  accountId: string,
+  accounts: AccountEntity[],
+): string {
+  newAccountName = newAccountName.trim();
+  if (newAccountName.length) {
+    const duplicateNamedAccounts = accounts.filter(
+      account => account.name === newAccountName && account.id !== accountId,
+    );
+    if (duplicateNamedAccounts.length) {
+      return t('Name {{ newAccountName }} already exists.', { newAccountName });
+    } else {
+      return '';
+    }
+  } else {
+    return t('Name cannot be blank.');
+  }
+}
diff --git a/upcoming-release-notes/3527.md b/upcoming-release-notes/3527.md
new file mode 100644
index 00000000000..6e476c8276d
--- /dev/null
+++ b/upcoming-release-notes/3527.md
@@ -0,0 +1,6 @@
+---
+category: Bugfix
+authors: [qedi-r]
+---
+
+Updates UI to disallow non-unique account names.