Skip to content

Properly support formatting of link segments in related urls #897

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

Merged

Conversation

platinumazure
Copy link
Contributor

Description of the Change

Add logic to RelatedMixin (related views) and RelationshipView (relationship views) to make sure related field kwarg is properly converted to underscore format.

I don't have a test yet. Would appreciate some guidance on how to proceed. Basically I need to test the view action and make sure the related serializer is found based on the underscore-converted kwarg.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated (no need)
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #897 (2a251b7) into master (7d2970a) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #897   +/-   ##
=======================================
  Coverage   97.67%   97.68%           
=======================================
  Files          58       58           
  Lines        3102     3115   +13     
=======================================
+ Hits         3030     3043   +13     
  Misses         72       72           
Impacted Files Coverage Δ
example/urls_test.py 100.00% <ø> (ø)
tests/test_views.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d2970a...2a251b7. Read the comment docs.

@sliverc
Copy link
Member

sliverc commented Mar 20, 2021

I am a bit confused by the title but is this change not just adding support of formatting links to RelationshipView? As RelatedMixin is not touched in this PR.

When looking at your changes in this PR it occurred to me that we introduced a backwards incompatible change in #876 which we introduce again in this PR as well.

Imagine someone uses camel cased naming for fields in their serializer. After this change in this PR suddenly it would not find the field anymore as it is looking for a field with underscore naming even though they did not configure FORMAT_RELATED_LINKS. A rare case as it is not the python convention but it could certainly happen. In the parser we actually addressed this issue that we only underscore field names when the setting FORMAT_FIELD_NAMES is actually configured. We should do the same with formatting links.

In terms of tests a integration test would work best like one of those

@platinumazure
Copy link
Contributor Author

Only converting to underscore when the setting is enabled makes sense. Thanks for the guidance on the test.

As for RelatedMixin vs RelationshipView, probably just a mistype on my part, but I do want to emphasize that both path/to/resource/relationships/relatedField and path/to/resource/relatedField were both not working correctly, and I'm fixing both in this PR. Only the former uses RelationshipView as far as I understand. But maybe I used the wrong name for the latter URL pattern.

Thanks for the initial review.

@sliverc
Copy link
Member

sliverc commented Apr 27, 2021

In #909 the issue has been addressed that field names are preserved when no formatting has been configured. @platinumazure Do you think you will find time to continue working on this PR?

@platinumazure
Copy link
Contributor Author

@sliverc Hard to say, I've been super busy lately. Sorry.

@sliverc sliverc force-pushed the fix-formatted-relation-links branch from 23c254b to 2a251b7 Compare April 28, 2021 19:04
@sliverc
Copy link
Member

sliverc commented Apr 28, 2021

I had a quick look into this and updated the PR.

To note is that for the formatted url to work dashes also need to be matched by the url pattern like the following:

  url(r'^orders/(?P<pk>[^/.]+)/(?P<related_field>[-\w]+)/$',
      OrderViewSet.as_view({'get': 'retrieve_related'}),
      name='order-related'),

The documentation before only stated \w which only matches underscores but not dashes so dasherize actually did not work. I've also added a hint in the changelog and the documentation for this. Hope this way it will work now. Happy for feedback.

@sliverc sliverc changed the title Ensure RelatedMixin and RelationshipView properly convert related_field to underscore format Properly support formatting of link segments in related urls Apr 28, 2021
@sliverc sliverc marked this pull request as ready for review April 28, 2021 19:09
@sliverc sliverc merged commit a5df955 into django-json-api:master Apr 29, 2021
@platinumazure
Copy link
Contributor Author

Thank you so much @sliverc for finishing this up!

@platinumazure platinumazure deleted the fix-formatted-relation-links branch December 23, 2022 04:27
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.

2 participants