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.

Sign up for GitHub

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 ability to have duplicate account names #3527

Merged
merged 13 commits into from
Oct 11, 2024
+243 −119
Merged

Fix ability to have duplicate account names #3527

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3738bde
add duplicate name validation to sidebar, and both narrow/wide Accoun…
qedi-r Sep 28, 2024
8c87837
unify error messages and styles
qedi-r Sep 28, 2024
a54c1c9
keep state for the current account name on the mobile account modal
qedi-r Sep 28, 2024
c1011f1
add release notes
qedi-r Sep 28, 2024
8a6b13e
remove extra validation function
qedi-r Sep 28, 2024
4d7a0f7
fix typo in AccountSyncSidebar params
qedi-r Sep 28, 2024
7a3529d
don't set current account name to empty string and prevent further ch…
qedi-r Sep 28, 2024
a52d173
lint imports
qedi-r Sep 28, 2024
b6df9c5
update error message to Name x already exists
qedi-r Oct 3, 2024
2642df5
Merge remote-tracking branch 'upstream/master' into issue-316
qedi-r Oct 3, 2024
4ac887a
Merge branch 'master' into issue-316
youngcw Oct 3, 2024
67d1719
use proper translation functions
qedi-r Oct 10, 2024
1103b4c
Merge branch 'master' into issue-316
youngcw Oct 11, 2024
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
20 changes: 15 additions & 5 deletions packages/desktop-client/src/components/accounts/Account.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -296,6 +297,7 @@ type AccountInternalState = {
prevShowCleared?: boolean;
showReconciled: boolean;
editingName: boolean;
nameError: string;
isAdding: boolean;
modalShowing?: boolean;
sort: {
Expand Down Expand Up @@ -343,6 +345,7 @@ class AccountInternal extends PureComponent<
showCleared: props.showCleared,
showReconciled: props.showReconciled,
editingName: false,
nameError: null,
isAdding: false,
sort: null,
filteredAmount: null,
Expand Down Expand Up @@ -703,13 +706,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: null });
}
};

Expand Down Expand Up @@ -1704,6 +1713,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}
Expand Down
236 changes: 139 additions & 97 deletions packages/desktop-client/src/components/accounts/Header.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
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';
import { Trans } from 'react-i18next';

import { t } from 'i18next';

Copy link
Contributor

@coderabbitai coderabbitai bot Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using the useTranslation hook instead of importing t directly.

Importing t directly from i18next might not properly handle re-renders or language changes within React components. Using the useTranslation hook from react-i18next ensures better integration with React's lifecycle and updates when the language changes.

Apply this diff to use the useTranslation hook:

- import { t } from 'i18next';
+ import { useTranslation } from 'react-i18next';

...

+ const { t } = useTranslation();

Committable suggestion was skipped due to low confidence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to ask about this one - it looks like it's in use like this in other places in the product, so I thought this was acceptable. Maybe clarifying statements should be made on https://actualbudget.org/docs/contributing/i18n/?

Copy link
Contributor

Choose a reason for hiding this comment

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

@qedi-r, thank you for pointing that out. Since importing t directly from i18next is used elsewhere in the codebase, it's acceptable in this context. Updating the contributing guidelines to clarify this practice would indeed be helpful.

(_/)
(•_•)
(/|) That's a great observation!

✏️ Learnings added 
Learnt from: qedi-r
PR: actualbudget/actual#3527
File: packages/desktop-client/src/components/accounts/Header.jsx:0-0
Timestamp: 2024-09-28T17:08:10.849Z
Learning: In the Actual Budget project, importing `t` directly from 'i18next' is acceptable and commonly used in the codebase.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest keeping the useTranslation here, there is extra logic to make it well-behaved on language changes and such. We really should use it everywhere but one step at a time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

import { useLocalPref } from '../../hooks/useLocalPref';
import { useSplitsExpanded } from '../../hooks/useSplitsExpanded';
Expand Down Expand Up @@ -67,6 +69,7 @@ export function AccountHeader({
onCreateReconciliationTransaction,
onToggleExtraBalances,
onSaveName,
saveNameError,
onExposeName,
onSync,
onImport,
Expand All @@ -89,8 +92,6 @@ export function AccountHeader({
onMakeAsSplitTransaction,
onMakeAsNonSplitTransactions,
}) {
const { t } = useTranslation();

const [menuOpen, setMenuOpen] = useState(false);
const searchInput = useRef(null);
const triggerRef = useRef(null);
Expand Down Expand Up @@ -176,99 +177,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}
failedAccount={failedAccounts}
accountsSyncing={accountsSyncing}
qedi-r marked this conversation as resolved.
Show resolved Hide resolved
/>
)}
{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>

Expand Down Expand Up @@ -471,6 +394,127 @@ 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,
}) {
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,
},
}}
>
Comment on lines +454 to +465
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Refactor inline styles to support pseudo-classes

The current inline styles for hover effects use pseudo-classes and nested selectors, which are not supported in React's inline style objects. This may cause the hover effects to not work as intended.

Consider refactoring these styles using a CSS-in-JS solution like styled-components or Emotion, or move them to an external CSS file. For example, using styled-components:

import styled from 'styled-components';

const HoverContainer = styled.div`
  display: flex;
  flex-direction: row;
  align-items: center;
  gap: 3px;

  .hover-visible {
    opacity: 0;
    transition: opacity 0.25s;
  }

  &:hover .hover-visible {
    opacity: 1;
  }
`;

// Then in your component:
<HoverContainer>
  {/* ... existing content ... */}
</HoverContainer>

This approach will ensure that the hover effects work as expected while maintaining the component's styling logic.

<View
style={{
fontSize: 25,
fontWeight: 500,
marginRight: 5,
marginBottom: -1,
}}
data-testid="account-name"
>
{account && account.closed
? t('Closed: {{ accountName }}', { accountName })
qedi-r marked this conversation as resolved.
Show resolved Hide resolved
: 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 })
qedi-r marked this conversation as resolved.
Show resolved Hide resolved
: accountName}
</View>
);
}
}
}

function AccountMenu({
account,
canSync,
Expand All @@ -483,8 +527,6 @@ function AccountMenu({
onReconcile,
onMenuSelect,
}) {
const { t } = useTranslation();

const [tooltip, setTooltip] = useState('default');
const syncServerStatus = useSyncServerStatus();

Expand Down
Loading
Loading