fix: removes ability to have duplicate account names on front-end #2776

Tigatok · 2024-05-18T00:05:49Z

netlify · 2024-05-18T00:06:05Z

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	`6831a0c`
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/66493486a014600008f85c66
😎 Deploy Preview	https://deploy-preview-2776.demo.actualbudget.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

github-actions · 2024-05-18T00:07:02Z

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
9	4.72 MB → 4.72 MB (+1.45 kB)	+0.03%

Changeset

File	Δ	Size
`src/components/modals/CreateLocalAccountModal.tsx`	📈 +694 B (+11.34%)	5.97 kB → 6.65 kB
`src/components/accounts/Header.jsx`	📈 +512 B (+3.34%)	14.98 kB → 15.48 kB
`src/components/accounts/Account.jsx`	📈 +276 B (+0.60%)	44.58 kB → 44.85 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
static/js/wide.js	261.78 kB → 262.55 kB (+788 B)	+0.29%
static/js/index.js	3 MB → 3 MB (+694 B)	+0.02%

Smaller

No assets were smaller

Unchanged

Asset	File Size	% Changed
static/js/indexeddb-main-thread-worker-e59fee74.js	13.5 kB	0%
static/js/resize-observer.js	18.37 kB	0%
static/js/BackgroundImage.js	122.29 kB	0%
static/js/narrow.js	59.97 kB	0%
static/js/usePreviewTransactions.js	790 B	0%
static/js/AppliedFilters.js	20.54 kB	0%
static/js/ReportRouter.js	1.23 MB	0%

github-actions · 2024-05-18T00:07:03Z

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
1	1.2 MB	0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset	File Size	% Changed
kcab.worker.js	1.2 MB	0%

MatissJanis

Thanks for working on this! Overall looks very good and solves 50% of the problem. The other 50% is: we should also catch this issue when an account is renamed.

MatissJanis · 2024-05-18T21:01:29Z

packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx

@@ -49,7 +55,12 @@ export function CreateLocalAccountModal({
              event.preventDefault();

              const nameError = !name;
-              setNameError(nameError);
+              setNameError({ error: nameError, message: 'Name is required' });


💬 suggestion: ‏we can put this in a conditional and return early if the error is encountered. Saves us a tiny amount of compute.

Suggested change

setNameError({ error: nameError, message: 'Name is required' });

if (!name) {

setNameError({ error: true, message: 'Name is required' });

return;

}

MatissJanis · 2024-05-18T21:02:01Z

packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx

-  const [nameError, setNameError] = useState(false);
+  const [nameError, setNameError] = useState({
+    error: false,
+    message: 'Name is required',


💬 suggestion: ‏initially there should be no error

Suggested change

message: 'Name is required',

message: '',

MatissJanis · 2024-05-18T21:04:57Z

packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx

@@ -75,15 +86,17 @@ export function CreateLocalAccountModal({
                    const name = event.target.value.trim();
                    setName(name);
                    if (name && nameError) {


💬 suggestion: ‏since we now have multiple types of errors: I think we can reset to the "no error" state if there previously was an error instead of using the name var

Suggested change

if (name && nameError) {

if (nameError.error) {

MatissJanis · 2024-05-18T21:05:57Z

packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx

-              setNameError(nameError);
+              setNameError({ error: nameError, message: 'Name is required' });
+
+              if (accounts.some(account => account.name === name)) {


💭 thought: ‏I wonder if we should make this case insensitive. WDYT?

I'd leave that up to the user. I'd say more control is better than less control, but that is just my opinion. LMK if we should change. Can always be done later.

Tigatok · 2024-05-18T23:10:25Z

I changed a couple things, added the editing name error functionality as well to check for duplicated and add an error if found.
I also found/fixed a bug with the error state when using setState by using the local error value on the create account form. LMK if you want me to change anything else.

Screen.Recording.2024-05-18.at.4.07.59.PM.mov

MatissJanis

Overall looks really good! Just two small comments. Let me know your thoughts.

MatissJanis · 2024-05-20T18:36:17Z

packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx

+              if (nameError.error) {
+                setNameError(nameError);
+              } else {
+                setNameError({ error: false, message: '' });
+              }


💬 suggestion: ‏could simplify this further

Suggested change

if (nameError.error) {

setNameError(nameError);

} else {

setNameError({ error: false, message: '' });

}

setNameError(nameError);

I can take a look at that, I think when I tried that maybe it needed to get un-set, if not, I'm not sure why I didn't do that in the first place haha

MatissJanis · 2024-05-20T18:40:11Z

packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx

+    return { error: true, message: `Name: “${name}” already exists.` };
+  }
+
+  return { error: false, message: '' };


🥜 nitpick: ‏what if we always returned a string in this util? Then we could re-use it in both this modal and the account page. Slightly less duplication of logic.

joel-jeremy · 2024-05-21T14:32:20Z

What do you guys think about putting the error message on the right of the input to avoid shifting the position of the transactions table?

Tigatok · 2024-05-21T15:23:09Z

@joel-jeremy I did think of the position to the right, but I'm not too sure how that would work mobile-y. Maybe could look into a popup of some sort?

MatissJanis · 2024-05-22T16:40:39Z

Good idea. A tooltip at the bottom with the error would fit nicely there IMO.

(if you use the tooltip component, then please use common/Tooltip)

Here's what we do for split transaction "errors". Not exactly a tooltip, but close-enough.

psybers · 2024-05-25T06:34:15Z

Does this affect setting up bank sync? Here is a scenario (based on real world):

I have 10 accounts that SimpleFIN lists with identical names (ugg...). I want to import two of them from the dialog, so I select 'Create new account' next to both.

Currently, this would create two identically named accounts in Actual. If I want to then disambiguate them, I could rename one/both. Or, as in my case, maybe I don't really care and leave them the same because there will not be transfers to/from them.

fix: removes ability to have duplicate account names on front-end

0eba229

ref: actualbudget#316

trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label May 18, 2024

github-actions bot changed the title ~~fix: removes ability to have duplicate account names on front-end~~ [WIP] fix: removes ability to have duplicate account names on front-end May 18, 2024

trafico-bot bot added 🚧 WIP Still work-in-progress, please don't review and don't merge and removed 🔍 Ready for Review Pull Request is not reviewed yet labels May 18, 2024

Merge branch 'master' of github.com:actualbudget/actual into actual-316

2d1a6cd

Tigatok changed the title ~~[WIP] fix: removes ability to have duplicate account names on front-end~~ fix: removes ability to have duplicate account names on front-end May 18, 2024

trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed 🚧 WIP Still work-in-progress, please don't review and don't merge labels May 18, 2024

MatissJanis reviewed May 18, 2024

View reviewed changes

Tigatok added 3 commits May 18, 2024 15:06

fix: prevent duplicate names when editing an account name

92097f0

feat: prevents editing name to duplicate, quotes update

7a27a69

fix: fix state issue on error and update verbiage

6831a0c

youngcw linked an issue May 20, 2024 that may be closed by this pull request

Duplicate Account names should not be possible. #316

Open

MatissJanis reviewed May 20, 2024

View reviewed changes

youngcw added this to the v24.6.0 milestone May 22, 2024

youngcw removed this from the v24.6.0 milestone May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: removes ability to have duplicate account names on front-end #2776

fix: removes ability to have duplicate account names on front-end #2776

Tigatok commented May 18, 2024

netlify bot commented May 18, 2024 •

edited

github-actions bot commented May 18, 2024 •

edited

github-actions bot commented May 18, 2024 •

edited

MatissJanis left a comment

MatissJanis May 18, 2024

MatissJanis May 18, 2024

MatissJanis May 18, 2024

MatissJanis May 18, 2024

Tigatok May 18, 2024

Tigatok commented May 18, 2024

MatissJanis left a comment

MatissJanis May 20, 2024

Tigatok May 21, 2024

MatissJanis May 20, 2024

joel-jeremy commented May 21, 2024

Tigatok commented May 21, 2024

MatissJanis commented May 22, 2024

psybers commented May 25, 2024

-              setNameError({ error: nameError, message: 'Name is required' });
+              if (!name) {
+                  setNameError({ error: true, message: 'Name is required' });
+                  return;
+              }

fix: removes ability to have duplicate account names on front-end #2776

Are you sure you want to change the base?

fix: removes ability to have duplicate account names on front-end #2776

Conversation

Tigatok commented May 18, 2024

netlify bot commented May 18, 2024 • edited

✅ Deploy Preview for actualbudget ready!

github-actions bot commented May 18, 2024 • edited

Bundle Stats — desktop-client

github-actions bot commented May 18, 2024 • edited

Bundle Stats — loot-core

MatissJanis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tigatok commented May 18, 2024

MatissJanis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joel-jeremy commented May 21, 2024

Tigatok commented May 21, 2024

MatissJanis commented May 22, 2024

psybers commented May 25, 2024

netlify bot commented May 18, 2024 •

edited

github-actions bot commented May 18, 2024 •

edited

github-actions bot commented May 18, 2024 •

edited