-
Notifications
You must be signed in to change notification settings - Fork 299
Allow HyperlinkedRelatedField to be used with related urls #529
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
Conversation
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.
Thanks @housleyjk This looks like a very reasonable fix. Testing should be improved though - see inline comments above.
example/serializers.py
Outdated
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.
Not so ideal to change from ResourceRelatedField to HyperlinkedRelatedField as now the one but not the other will be tested.
Hence I think it would be better to add a new HyperlinkedRelatedField. Maybe comments could be used?
Codecov Report
@@ Coverage Diff @@ ## master #529 +/- ## ========================================== + Coverage 94.31% 94.33% +0.01% ========================================== Files 60 60 Lines 3713 3725 +12 ========================================== + Hits 3502 3514 +12 Misses 211 211
Continue to review full report at Codecov.
|
Ok, I've improved the testing for this feature as requested and done my best to verify that it is correct. I hope you will forgive me renaming comment_set -> comments. I thought it was more consistent with the related name entries already in use.
Thanks a lot @housleyjk I totally agree renaming comments to comment_set is much cleaner.
Uh oh!
There was an error while loading. Please reload this page.
Fixes #521
Description of the Change
The details in #521 are not clear, but I ran into a very similar
SkipFieldissue when trying to use HyperlinkedRelatedFields and the RelatedMixin. The HyperlinkedRelatedField generates the correct url for the related link (e.g. /authors/1/entries), but visiting the url gives a 500 error because of an unhandledSkipFieldexception. In other words, without these changes, it is possible to give a related link in the payload, but that link won't actually work.Checklist
test_retrieve_related_many()test. So this code is covered by unit tests.)CHANGELOG.mdAUTHORS