fix: clean up "register random person" test #190

jose · 2025-08-28T12:52:23Z

jose commented

2025-08-28 12:52:23 +00:00

Remove redundant "import User Zero" action
Remove out-of-scope actions from test (sending a gift to an unrelated entity, deleting the contact)
Update imports based on changes

- Remove redundant "import User Zero" action - Remove out-of-scope actions from test (sending a gift to an unrelated entity, deleting the contact) - Update imports based on changes

jose added 4 commits 2025-08-28 12:52:23 +00:00

feat: add duplicate account import prevention f51408e32a

- Add duplicate check in ImportAccountView before account import
- Add duplicate check in ImportDerivedAccountView for derived accounts
- Add safety check in saveNewIdentity function to prevent duplicate saves
- Implement user-friendly warning messages for duplicate attempts
- Add comprehensive error handling to catch duplicate errors from saveNewIdentity
- Create Playwright tests to verify duplicate prevention functionality
- Add documentation for duplicate prevention implementation

The system now prevents users from importing the same account multiple times
by checking for existing DIDs both before import (pre-check) and during
save (post-check). Users receive clear warning messages instead of
technical errors when attempting to import duplicate accounts.

Files modified:
- src/views/ImportAccountView.vue: Add duplicate check and error handling
- src/views/ImportDerivedAccountView.vue: Add duplicate check for derived accounts
- src/libs/util.ts: Add duplicate prevention in saveNewIdentity
- test-playwright/duplicate-import-test.spec.ts: Add comprehensive tests
- doc/duplicate-account-import-implementation.md: Add implementation docs

Resolves: Prevent duplicate account imports in IdentitySwitcherView

refactor: move duplicate account import warnings to notification constants c4f2bb5e3a

- Add NOTIFY_DUPLICATE_ACCOUNT_IMPORT constant for import warnings
- Add NOTIFY_DUPLICATE_DERIVED_ACCOUNT constant for derived account warnings
- Update ImportAccountView.vue to use notification constants
- Update ImportDerivedAccountView.vue to use notification constants
- Update test file to use notification constants for assertions

Centralizes notification messages for better maintainability and consistency
with the existing notification system.

Files modified:
- src/constants/notifications.ts: Add new notification constants
- src/views/ImportAccountView.vue: Replace hardcoded messages with constants
- src/views/ImportDerivedAccountView.vue: Replace hardcoded messages with constants
- test-playwright/duplicate-import-test.spec.ts: Update test assertions

chore - reorder duplication test 96e4d3c394

- Rename the test to run it earlier in the test suite

fix: clean up "register random person" test 40fa38a9ce

- Remove redundant "import User Zero" action
- Remove out-of-scope actions from test (sending a gift to an unrelated entity, deleting the contact)
- Update imports based on changes

jose added 1 commit 2025-08-28 13:05:31 +00:00

fix: change import User Zero function e67c97821a

- Use the ./account route to mimic real-world use

trentlarson reviewed 2025-08-29 23:38:08 +00:00

doc/duplicate-account-import-implementation.md

						
				@@ -0,0 +23,4 @@

				- ✅ **User-friendly error messages**  

				  - **Time**: 2025-01-27T14:30:00Z  

				  - **Evidence**: Clear warning messages in both import views

				  - **Verify at**: ImportAccountView and ImportDerivedAccountView notification calls

trentlarson commented

2025-08-29 23:38:08 +00:00

This looks like a log file that doesn't really help other people (except maybe in a report). If you want to keep it in our codebase, do me a favor and move it into a directly like "log" that's under the "doc" directory.

trentlarson reviewed 2025-08-29 23:41:33 +00:00

src/libs/util.ts

						
				@@ -629,0 +637,4 @@

				        `Account with DID ${identity.did} already exists. Cannot import duplicate account.`,

				      );

				    }

trentlarson commented

2025-08-29 23:41:33 +00:00

Thanks for guarding against this.

Note that there's a $getAllAccountDids method in the PlatformServiceMixin that could be used here instead of direct SQL. (Not critical.)

Thanks for guarding against this. Note that there's a `$getAllAccountDids` method in the PlatformServiceMixin that could be used here instead of direct SQL. (Not critical.)

trentlarson marked this conversation as resolved

trentlarson reviewed 2025-08-29 23:45:48 +00:00

src/views/ImportDerivedAccountView.vue

						
				@@ -195,0 +224,4 @@

				      this.$logError("Error checking for duplicate account: " + error);

				      return false;

				    }

				  }

trentlarson commented

2025-08-29 23:45:48 +00:00

Since this same logic is in multiple places, I suggest using the same methods for them all, eg. the one in lib/util.

trentlarson reviewed 2025-08-29 23:55:09 +00:00

src/views/ImportAccountView.vue

						
				@@ -226,0 +244,4 @@

				        error instanceof Error ? error.message : String(error);

				      if (

				        errorMessage.includes("already exists") &&

				        errorMessage.includes("Cannot import duplicate account")

trentlarson commented

2025-08-29 23:55:09 +00:00

Since the sentence in the error message is user-oriented, someone could change it and that would break this logic. I suggest put the whole message for the user inside the error, eg: userMessage: "This account already..."

I won't stop it from merging, but it's worth another look and maybe a conversation with Matthew and/or me.

Since the sentence in the error message is user-oriented, someone could change it and that would break this logic. I suggest put the whole message for the user inside the error, eg: `userMessage: "This account already..."` I won't stop it from merging, but it's worth another look and maybe a conversation with Matthew and/or me.

trentlarson marked this conversation as resolved

jose commented

2025-09-01 12:09:11 +00:00

@trentlarson most of your review items here have been resolved in a different PR, from which this one borrowed for the updated test suite:

#189

Unneeded doc removed
Duplicate logic unified
Error message testing simplified

That last one still only checks for a partial (but specific enough for its case) match to accommodate the variable inserted in the full message. Unless you think it's better to test the full message including the variable?

As for $getAllAccountDids, Cursor pointed out efficiency concerns between retrieving all DIDs first vs. directly finding a match. Does that check out?

@trentlarson most of your review items here have been resolved in a different PR, from which this one borrowed for the updated test suite: https://gitea.anomalistdesign.com/trent_larson/crowd-funder-for-time-pwa/pulls/189 - Unneeded doc removed - Duplicate logic unified - Error message testing simplified That last one still only checks for a partial (but specific enough for its case) match to accommodate the variable inserted in the full message. Unless you think it's better to test the full message including the variable? As for `$getAllAccountDids`, Cursor pointed out efficiency concerns between retrieving all DIDs first vs. directly finding a match. Does that check out?

trentlarson commented

2025-09-01 12:46:39 +00:00

Yes, that's a good point: knowing the DID, it's best to call a method that retrieves that exact DID, eg something like $getAccount rather than retrieving them all.

Yes, that's a good point: knowing the DID, it's best to call a method that retrieves that exact DID, eg something like `$getAccount` rather than retrieving them all.

jose merged commit 2f05d27b51 into master

2025-09-01 14:00:16 +00:00

jose referenced this issue from a commit

2025-09-01 14:00:16 +00:00

Merge pull request 'fix: clean up "register random person" test' (#190) from playwright-test-00-fix into master

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: trent_larson/crowd-funder-for-time-pwa#190