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

profile: show warning and option to reactivate when contact method is disabled #28

Merged
merged 89 commits into from
Jul 16, 2019

Conversation

Forfold
Copy link
Contributor

@Forfold Forfold commented Jun 24, 2019

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
This PR adds a warning to a contact method if it is currently disabled. The warning icon should have a tooltip directing the user on how to reactivate said contact method.

Additional Info:
This PR also refactors the verification form/dialog to use React hooks, as well as updates the verification mutations to use our latest graphql library.

@Forfold
Copy link
Contributor Author

Forfold commented Jun 26, 2019

Tests passed.

graphql2/schema.graphql Outdated Show resolved Hide resolved
notification/store.go Outdated Show resolved Hide resolved
notification/store.go Outdated Show resolved Hide resolved
notification/store.go Outdated Show resolved Hide resolved
graphql2/schema.graphql Outdated Show resolved Hide resolved
web/src/app/users/ContactMethodVerificationForm.js Outdated Show resolved Hide resolved
web/src/app/users/ContactMethodVerificationDialog.js Outdated Show resolved Hide resolved
web/src/app/users/UserContactMethodList.js Outdated Show resolved Hide resolved
web/src/app/icons/components/Icons.js Outdated Show resolved Hide resolved
@Forfold
Copy link
Contributor Author

Forfold commented Jun 28, 2019

Comments addressed. Outstanding work is to create a new migration for the user_verification_codes table to:

  • Remove user_id and contact_method_value columns
  • Add a contact_method_id column
  • Replace send_to with a bool sent

m17ch
m17ch previously requested changes Jul 2, 2019
web/src/app/users/UserContactMethodList.js Outdated Show resolved Hide resolved
web/src/app/users/UserContactMethodVerificationDialog.js Outdated Show resolved Hide resolved
Copy link
Contributor

@arurao arurao left a comment

Choose a reason for hiding this comment

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

A few things I noticed by functionally testing the PR:

  • Should we add a validation for checking if code entered on UI only consists of numbers and of length 6? Right now, if we enter a string, it goes all the way to the backend and returns unexpected error. It would be better if we avoided it going to the backend and masked the error with some helpful text too.

This is what we see currently:
Screen Shot 2019-07-02 at 12 50 44 PM

It would be nice to instead see a validation error similar to
Screen Shot 2019-07-02 at 1 03 05 PM

  • One nice-to-have validation we could maybe add is to disable the Submit button on the dialog unless the user has hit the Send code button at least once. What do you think?

  • If a user has 2 contact methods, clicking on Verify/Reactivate for the first contact method shows the dialog for actually the second one.

@mastercactapus mastercactapus changed the title show contact method warning when disabled profile: show warning when contact method is disabled Jul 2, 2019
@Forfold
Copy link
Contributor Author

Forfold commented Jul 2, 2019

@arurao Looks like I forgot to change the type of the textfield to type='number' which would disallow typing anything but numbers. Thanks for catching, I will get this fixed

Copy link
Member

@mastercactapus mastercactapus left a comment

Choose a reason for hiding this comment

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

Can you merge latest from master into this? Looks like there's a conflict.

notification/store.go Outdated Show resolved Hide resolved
# Conflicts:
#	web/src/app/users/UserContactMethodList.js
@mastercactapus mastercactapus removed the request for review from m17ch July 16, 2019 19:12
@mastercactapus mastercactapus dismissed m17ch’s stale review July 16, 2019 19:14

OOO, comments were addressed

@Forfold Forfold merged commit 32d6132 into master Jul 16, 2019
@Forfold Forfold deleted the contact-method-warning-disabled branch July 16, 2019 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants