If an admin searches for a user by email, don't crash if no user can be found #2295
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
mattwr18
requested changes
Nov 20, 2019
There was a problem hiding this comment.
Sorry to keep saying the same thing, but I think that we cannot give a review on a PR with a title Fix #<issue_number>
- I don't know what issue 🐛 [Bug] Filter user by email leads to 500 error if email can't be found #2294 fixes, which means instead of the
PRcreator taking an extra minute to update the title with something meaningful, you are asking thePRreviewer to take their time to figure out what issue thePRsolves. - sometimes I don't have time to give an actual review, but would like a high-level overview of what issue the
PRaddresses - and maybe most importantly, when we create our changelog and release, this
PR, if merged like so, will only appear asFix #2294, which again means the people helping to manually test the new features/bug fixes will need to spend their time to figure out what they need to be testing.
Please, update the title accordingly, and I'll be happy to give time and attention to reviewing this PR.
Thanks ;)
roschaefer
changed the title
|
@mattwr18 is that better? |
|
great @roschaefer !! I'll give this a review now 😸 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ideally we would not have this inconsistent edge case that we filter
through the user type. Much better would be a graphql query like:
{ User(filter: { primary_email: { email: "something@example.org" } }) { id name slug } }EDIT
I tried to implement the above behaviour and I think it's OK to have this edge case. Reason: you could nest
_UserFilterindefinitely. So it becomes complex and risky to create agraphql-shieldrule based onargs.filter.primary_email.Keeping it simple stupid and having this one inconsistency is OK I would say.
Fix #2294