-
-
Notifications
You must be signed in to change notification settings - Fork 128
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
Add unit tests for partial input optional field behaviour in update mutations #638
Add unit tests for partial input optional field behaviour in update mutations #638
Conversation
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @SupImDos - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @SupImDos - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have skipped reviewing this pull request. It looks like we've already reviewed the commit cf21c16 in this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this! ❤️
Description
As noted in the module's docstring, this PR adds some unit tests for Strawberry-Django's handling of partial input optional fields in update mutations, specifically when their values are omitted or explicitly set to
null
, for different variations of model fields:These tests stem from the fact that the GraphQL type-system doesn't distinguish between optional and nullable. That is, type
T!
is both required and non-nullable (i.e., must be supplied and cannot benull
), but typeT
is both optional and nullable (i.e., can be omitted and can benull
).These tests uncovered what I believe is a small bug in
update_m2m
, where a value ofNone
wasn't being explicitly checked.For reference, we were going to add similar unit tests to our project, but thought they might be useful upstream.
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Add unit tests for handling partial input optional fields in update mutations, covering various model field types and ensuring correct behavior when values are omitted or set to null. Fix a bug in the
update_m2m
function to handleNone
values correctly.Bug Fixes:
update_m2m
function where a value ofNone
was not being explicitly checked, ensuring correct handling ofNone
values.Tests: