trent_larson
/
crowd-funder-for-time-pwa
4
1
Fork 2
Code Issues Pull Requests 18 Releases 5 Wiki Activity
Labels Milestones

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

Merged
jose merged 2 commits from playwright-test-00-fix into master 2 days ago
Conversation 2 Commits 2 Files Changed 2
jose commented 6 days ago
Owner
  • 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 6 days ago
f51408e32a feat: add duplicate account import prevention
c4f2bb5e3a refactor: move duplicate account import warnings to notification constants
96e4d3c394 chore - reorder duplication test
40fa38a9ce fix: clean up "register random person" test
jose added 1 commit 6 days ago
e67c97821a fix: change import User Zero function
trentlarson reviewed 4 days ago
doc/duplicate-account-import-implementation.md
- ✅ **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 4 days ago
Poster
Owner

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.

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 4 days ago
src/libs/util.ts
`Account with DID ${identity.did} already exists. Cannot import duplicate account.`,
);
}
trentlarson commented 4 days ago
Poster
Owner

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 4 days ago
src/views/ImportDerivedAccountView.vue
this.$logError("Error checking for duplicate account: " + error);
return false;
}
}
trentlarson commented 4 days ago
Poster
Owner

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

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 4 days ago
src/views/ImportAccountView.vue
error instanceof Error ? error.message : String(error);
if (
errorMessage.includes("already exists") &&
errorMessage.includes("Cannot import duplicate account")
trentlarson commented 4 days ago
Poster
Owner

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 2 days ago
Poster
Owner

@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 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 2 days ago
Owner

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 2 days ago
The pull request has been merged as 2f05d27b51.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
Labels
Apply labels
Clear labels
No items
No Label
Milestone
Set milestone
Clear milestone
No items
No Milestone
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'playwright-test-00-fix'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes