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: check if contact address or name is already present #955

Merged
merged 10 commits into from Jan 15, 2019

Conversation

@dated
Copy link
Contributor

commented Jan 11, 2019

Proposed changes

Adds validation to the new contact form, disabling the first steps Next button and the Done button and showing a helper text if the address or name has already been added as a contact.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

@dated dated changed the title fix: check if contact address is already present fix: check if contact address or name is already present Jan 11, 2019

@j-a-m-l j-a-m-l self-requested a review Jan 14, 2019

@j-a-m-l
Copy link
Contributor

left a comment

I'm trying to create a new contact using the address of 1 of my wallets, and, instead of displaying the error on the form, and blocking the buttons, I can continue until the end.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

That's intended, I didn't see a problem with wanting to add a wallet as a contact. It checks for already present contacts only.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

So while i wouldn't forbid it, a notice would be appropriate that reminds the user that the address has already been added as a wallet.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

The current implementation doesn't allow duplicating wallets and contacts. Even in your PR and error toast is triggered after clicking on "Done". It would be better to display those messages on the form and disallow the buttons.

Keep in mind that adding an existing wallet as a contact doesn't add any value and it promotes confusing behaviours. For instance: which labels would be used to display it on the transaction table?

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

Noted! I'll change it accordingly, and differentiate between address already present as wallet and address already present as contact in the helper text.

dated added some commits Jan 14, 2019

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

I like your idea of showing a different message if the contact address already exists as a wallet, but:

  • Change the message when there is a contact already, instead of using "The wallet address X already exists"
  • Use the error (red) colour on the messages for those messages
  • Check that the contact name is not a wallet name yet. This is not forbidden but it should be to not confuse users, or, at least, a message (on grey) should be displayed.

dated added some commits Jan 14, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

Changed as requested. Regarding your third point, I think the same name for a wallet and a contact should not be allowed. I added the according validation.

@j-a-m-l
Copy link
Contributor

left a comment

Thanks

@j-a-m-l j-a-m-l merged commit 9e54607 into ArkEcosystem:develop Jan 15, 2019

1 check passed

ci/circleci: test-node-9 Your tests passed on CircleCI!
Details

@dated dated deleted the dated:contact-validation branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.