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.

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: contact names should not allow duplication #28249

Merged
merged 20 commits into from
Nov 20, 2024

Conversation

Matt561
Copy link
Contributor

@Matt561 Matt561 commented Nov 1, 2024

Description

This fix prevents users from having duplicate names in their contact list. A warning banner is now displayed in the contact list when duplicates are found. All duplicate contacts will also have a warning icon next to them to help users better identify duplicates.

What is considered a duplicate contact?

A duplicate is a contact with a different address but same name as another contact or account.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

Adding a duplicate contact

  1. Go to Settings > Contacts page
  2. Attempt to add a new contact using an existing contact or account name
  3. An error message will appear underneath the username input and the submit button will be disabled.

Viewing duplicate contact warning banner

  1. In your local environment, go to file ui/components/app/contact-list/contact-list.component.js
  2. On line 138 ({hasDuplicateContacts ? this.renderDuplicateContactWarning() : null} change hasDuplicateContacts to true to display the banner.

Screenshots/Recordings

Before

duplicate-contact-fix-before-demo.mov

After

duplicate-contact-fix-after-demo.mov
Screenshot 2024-11-15 at 1 59 21 PM Screenshot 2024-11-15 at 1 59 28 PM

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@Matt561 Matt561 requested review from a team as code owners November 1, 2024 21:42
Copy link
Contributor

github-actions bot commented Nov 1, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Matt561
Copy link
Contributor Author

Matt561 commented Nov 1, 2024

I have read the CLA Document and I hereby sign the CLA

@Matt561 Matt561 force-pushed the fix/26621-contact-names-should-not-allow-duplication branch from efd4d8f to 16e2f38 Compare November 4, 2024 17:51
@metamaskbot
Copy link
Collaborator

Builds ready [66945ee]
Page Load Metrics (1904 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29320561745488234
domContentLoaded1733204318737838
load1748205819048139
domInteractive198144199
backgroundConnect779292311
firstReactRender522931166230
getState571192010
initialActions00000
loadScripts1196153713548039
setupStore106822189
uiStartup19612586215214670
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 2.19 KiB (0.03%)
  • common: 116 Bytes (0.00%)

Copy link
Contributor

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

I have some questions before I can approve. can you please clarify?

test/data/mock-data.js Outdated Show resolved Hide resolved
ui/components/app/contact-list/contact-list.component.js Outdated Show resolved Hide resolved
ui/components/app/contact-list/contact-list.component.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [f1351c6]
Page Load Metrics (1925 ± 115 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31722791750481231
domContentLoaded156925521891239115
load157325631925239115
domInteractive25176543316
backgroundConnect96834199
firstReactRender47129902110
getState45215157
initialActions01000
loadScripts11041800137918187
setupStore1168272010
uiStartup178428102119253122
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.17 KiB (0.04%)
  • common: 116 Bytes (0.00%)

matthewwalsh0
matthewwalsh0 previously approved these changes Nov 7, 2024
Copy link
Member

@matthewwalsh0 matthewwalsh0 left a comment

Choose a reason for hiding this comment

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

Reviewed minor confirmations ownership changes .

@Matt561 Matt561 removed this pull request from the merge queue due to a manual request Nov 7, 2024
mirceanis
mirceanis previously approved these changes Nov 8, 2024
Copy link
Contributor

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Looks good!

@Matt561
Copy link
Contributor Author

Matt561 commented Nov 15, 2024

👋 Hey all, the design team requested that I add a warning icon next to the duplicate contacts for better clarity. Could I get another review when you get a moment please? 😄

Here are the latest screenshots

Screenshot 2024-11-15 at 1 59 21 PM Screenshot 2024-11-15 at 1 59 28 PM

@Matt561 Matt561 force-pushed the fix/26621-contact-names-should-not-allow-duplication branch from 109530a to b73e844 Compare November 15, 2024 21:10
@metamaskbot
Copy link
Collaborator

Builds ready [e8b47de]
Page Load Metrics (2077 ± 99 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint57925792020396190
domContentLoaded16962517203820096
load17192527207720699
domInteractive15563894
backgroundConnect8104443416
firstReactRender52294996632
getState522731024722
initialActions01000
loadScripts11871969152817383
setupStore6481194
uiStartup194032312408307148
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 4.56 KiB (0.06%)
  • common: 215 Bytes (0.00%)

Copy link
Contributor

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

looks good!

@Matt561 Matt561 added this pull request to the merge queue Nov 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@Matt561 Matt561 added this pull request to the merge queue Nov 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [5d7fcc6]
Page Load Metrics (2282 ± 96 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29927801992698335
domContentLoaded20072633223817684
load20202787228220196
domInteractive3297522010
backgroundConnect9154433718
firstReactRender7439518512560
getState922751385627
initialActions00000
loadScripts14872082168815876
setupStore78215189
uiStartup233737062794330159
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 4.56 KiB (0.06%)
  • common: 215 Bytes (0.00%)

@Matt561 Matt561 added this pull request to the merge queue Nov 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@Matt561 Matt561 added this pull request to the merge queue Nov 20, 2024
Merged via the queue into develop with commit f3cec69 Nov 20, 2024
75 checks passed
@Matt561 Matt561 deleted the fix/26621-contact-names-should-not-allow-duplication branch November 20, 2024 18:50
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.9.0 Issue or pull request that will be included in release 12.9.0 team-identity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants