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 typed choices, make working with different Django 5x choices options #1539

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GeyseR
Copy link
Contributor

@GeyseR GeyseR commented Dec 26, 2024

An attempt to fix #1476

While working on the fix, I realized that Graphene doesn't support choice fields initialized with a types class itself without calling choices. Fixed this as well.

Django 5.1 introduced a convenient normalize_choices method that I use if a newer django version installed

Copy link
Collaborator

@sjdemartini sjdemartini left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. You mention that this should fix #1476. Could you please add a test or two accordingly for the mutation behavior, to ensure it's fixed and won't regress in the future?

graphene_django/compat.py Show resolved Hide resolved
graphene_django/compat.py Outdated Show resolved Hide resolved
graphene_django/compat.py Show resolved Hide resolved
@@ -61,6 +60,24 @@ def wrapped_resolver(*args, **kwargs):
return blank_field_wrapper(resolver)


class EnumValueField(Field):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this effectively replaces BlankValueField. Can you add a comment or something stating as much, and "marking" BlankValueField as deprecated? I assume it's effectively only ever been used internally, but we shouldn't remove it without a major version bump, unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, my idea was to inherit from the BlankValueField class, but I forgot to. The interesting thing is that I've seen the test that was added when this class was added (test_choice_enum_blank_value), but it didn't catch the bug in my code as the test itself was wrong (the field in the test instance was null instead of blank).
I've tested a bug in the test and classes inheritance, so they match my initial idea, but if you want me to move all the logic to the EnumValueField and deprecate BlankValueField, I can do that

graphene_django/tests/test_converter.py Outdated Show resolved Hide resolved
@GeyseR GeyseR force-pushed the fix-typed-choice-fields branch 2 times, most recently from 34ed25e to 176b3e2 Compare December 26, 2024 23:06
@GeyseR
Copy link
Contributor Author

GeyseR commented Dec 26, 2024

You mention that this should fix #1476. Could you please add a test or two accordingly for the mutation behavior, to ensure it's fixed and won't regress in the future?

the source of the problem was that the library incorrectly resolved typed choice values assigned to model choice fields (which is a totally valid case and can happen not only in mutations). I've added a test_typed_choice_value test that covers that

@sjdemartini
Copy link
Collaborator

You mention that this should fix #1476. Could you please add a test or two accordingly for the mutation behavior, to ensure it's fixed and won't regress in the future?

the source of the problem was that the library incorrectly resolved typed choice values assigned to model choice fields (which is a totally valid case and can happen not only in mutations). I've added a test_typed_choice_value test that covers that

Would you be able to add a test for the mutation specifically, for comprehensiveness and good measure?

@GeyseR GeyseR force-pushed the fix-typed-choice-fields branch from 176b3e2 to ff99248 Compare January 8, 2025 22:01
@GeyseR
Copy link
Contributor Author

GeyseR commented Jan 8, 2025

Would you be able to add a test for the mutation specifically, for comprehensiveness and good measure?

extended the added test with the mutation call. Thanks for the suggestion!

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.

Mutation can not represent TextChoice value
2 participants