-
Notifications
You must be signed in to change notification settings - Fork 301
Prefetch default included resources #900
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
Prefetch default included resources #900
Conversation
Sorry, but I have no idea why the test is failing. |
Thanks for your contribution. PR number 900... wow! This is certainly a good addition to add support for default included resources. We might even consider this to be a bug that this has not been the case in the first place... In terms of tests for quality assurance all PRs in DJA need to have a test to prove that the change actually does what it is supposed too. It is never too late to start learning about testing though... for instance this is a good tutorial but there are many others out there. It also seems with your change you discovered a potential another problem that inlcudes currently do not work when using relationship view. See failing test where a relationship view now prefetches default resources and fails. Some more research needs to be done. If you think this is a bit too hard of a task we can also convert this PR to a issue report and somewhere else can have a look at it and have your PR as a reference. Do you wanna give it or shot or shall we convert it to a bug report? What do you think? |
I'm not sure if the test environment is different than the normal environment, but Anyway, this is the best I can reach in testing. If anyone is willing to do more, please feel free to take over. |
So the tests use the plural (normal I believe) form of resources, while the end-user example uses the singular form. |
Codecov Report
@@ Coverage Diff @@
## master #900 +/- ##
=======================================
Coverage 97.68% 97.69%
=======================================
Files 58 58
Lines 3115 3120 +5
=======================================
+ Hits 3043 3048 +5
Misses 72 72
Continue to review full report at Codecov.
|
@SafaAlfulaij As of changes merged in #914 could you rebase this to master? |
It seems that this is the only issue preventing #911. I can free up sometime this coming Wednesday to look at rebasing this to master if that helps? |
@simkimsia Thanks for your offer. Really appreciated. I got around today to look at this PR and hope a new version of DJA will be out soon. |
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.
@SafaAlfulaij I made some small adjustment and the PR is now ready for merging. Thanks for your work!
Description of the Change
Pass the serializer to
select_related
andprefetch_related
defaultincluded_resources
. Without this, only resources specified by?include
are used.Checklist
CHANGELOG.md
updated (only for user relevant changes)AUTHORS