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

Bug - Improve unique email validation #4103

Merged
merged 7 commits into from
Sep 28, 2022
Merged

Conversation

esizer
Copy link
Member

@esizer esizer commented Sep 28, 2022

Summary

This resolves an issue with not being able to update an existing user due to the unique email validation by ignoring the user you are updating.

Testing

  1. Regenerate the queries npm run codegen --workspaces
  2. Build all projects npm run production --workspaces
  3. Navigate tot /admin/users/
  4. Edit a user and update field other than email
  5. Ensure you can save the user
  6. Edit it again, using an existing email
  7. Ensure you get the unique email error
  8. Navigate to /talent/profile
  9. Go to to the "About Me" form and repeat steps 4-7
  10. Logout and register as a new user
  11. Attempt to create account with an existing email
  12. Ensure you get the unique email error

@esizer esizer added the bug Something isn't working. label Sep 28, 2022
@petertgiles
Copy link
Contributor

What bug does this fix? How do we test it?

@vd1992
Copy link
Contributor

vd1992 commented Sep 28, 2022

What bug does this fix? How do we test it?

https://talent-cloud.slack.com/archives/C0259G6KXJ6/p1664382823644029

@esizer
Copy link
Member Author

esizer commented Sep 28, 2022

What bug does this fix? How do we test it?

Sorry @petertgiles . I was in such a rush to get this up I forgot to write a proper description with testing instructions.

Essentially, when we added the unique email rule for /create-account it was also triggering for updates and would not allow a user to update their own account since their email already existed. This ignores the user email marching the id passed to the mutation.

@petertgiles
Copy link
Contributor

when we added the unique email rule for /create-account it was also triggering for updates

Oh no! That's what I get for ignoring Cypress I guess. 🤦 Thanks for fixing this so quickly.

@esizer
Copy link
Member Author

esizer commented Sep 28, 2022

Oh no! That's what I get for ignoring Cypress I guess. 🤦 Thanks for fixing this so quickly.

Felt responsible since I approved it. I don't like that cypress is being marked as "skipped" rather than "failed" right now.

@petertgiles petertgiles self-requested a review September 28, 2022 18:54
@esizer
Copy link
Member Author

esizer commented Sep 28, 2022

@petertgiles Sorry, I did not realize the massive diff for schema.gql. The important lines are:

input UpdateUserAsAdminInput
@validator(class: "App\\GraphQL\\Validators\\UpdateUserInputValidator") {
id: ID

input UpdateUserAsUserInput
@validator(class: "App\\GraphQL\\Validators\\UpdateUserInputValidator") {
id: ID

Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

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

Nice job! This fixes the error for me. Two small things to check on.

api/graphql/schema.graphql Outdated Show resolved Hide resolved
@esizer esizer requested a review from petertgiles September 28, 2022 19:18
Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

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

I think you got it! 🏅

@esizer esizer merged commit 4aa9bbc into main Sep 28, 2022
@esizer esizer deleted the bug/improve-unique-email-validation branch September 28, 2022 20:21
@patcon
Copy link
Contributor

patcon commented Sep 29, 2022

Thanks a million @esizer! I think this may have resolved everything after #4108 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants