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

Update get_serializer_class to consider the request method #77

Merged
merged 10 commits into from
May 27, 2024

Conversation

pamella
Copy link
Contributor

@pamella pamella commented May 23, 2024

Description: Describe in a couple of sentence what this PR adds

  • Update get_serializer_class to consider the request method. See the updated docstring.
    • If the request method is GET, it tries to use self.read_serializer_class.
    • If the request method is not GET, it tries to use self.write_serializer_class.
    • If the specific serializer class for the request method is not set, it falls back to self.serializer_class.

Note

Once this PR is approved and merged, I will open another one updating the package version so we can publish it to PyPI.

Dependencies: dependencies on other outstanding PRs, issues, etc.

Resolves #1
Resolves #17
Resolves #44

Merge deadline: List merge deadline (if any)

N/A

Installation instructions: List any non-trivial installation
instructions.

Testing instructions:

  • On a local testing project, get the changes made, and access the API in the browser:
    • Make sure the page renders properly.
    • Make sure GET and non-GET requests (such as POST) work properly.
    • Configure a swagger and make sure the UI works properly.
    • Test views using a mix of serializer_class, read_serializer_class, and write_serializer_class.
Screencast.from.23-05-2024.16.55.32.webm

Reviewers:

  • tag reviewer
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • PR author is listed in AUTHORS

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GenericAPIViewGetSerializerClassTests tests the changes made in this file.

ListModelMixin,
RetrieveModelMixin,
UpdateModelMixin,
)


class GenericAPIView(generics.GenericAPIView):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pamella pamella added the bug Something isn't working label May 23, 2024
@pamella pamella requested a review from fjsj May 23, 2024 20:02
(Eg. admins get full serialization, others get basic serialization)
"""
if hasattr(self, "request"):
if self.request.method in ["GET"]:
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if HEAD, OPTIONS or TRACE would get the write serializer? I imagine they should get the read one, but they don't have a body, so I'm not sure if it would really do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, currently they would get the write serializer. Should I include those values in the condition?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. These should be read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related change: commit 58e95c3. Let me know if there are any adjustments to make to it.

Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

Please check comments

@pamella pamella mentioned this pull request May 27, 2024
12 tasks
@pamella pamella requested a review from fjsj May 27, 2024 14:25
@pamella pamella merged commit 575b1e9 into main May 27, 2024
10 checks passed
@pamella pamella deleted the fix/get_serializer_class branch May 27, 2024 14:47
@pamella pamella mentioned this pull request May 27, 2024
12 tasks
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.

Can't open API in web browser swagger/openapi support Usage failure
2 participants