Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
APIv3 endpoint: allow to modify a Project once it's imported #5952
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
APIv3 endpoint: allow to modify a Project once it's imported #5952
Changes from 7 commits
c3044fc
0601c2d
6629c00
2a393f5
e591184
0bb5528
cdad40f
be0ccb1
61dc78b
ca2a1c6
ad12f1f
13c86e5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I assume it will also raise an error in some cases?
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.
On the "Ship APIv3" PR I was thinking to remove all the redundant docs or un-useful list of fields: avoid things like
slug: the slug of the Project
and just document that ones that are useful.I applied the same concept for status codes, mentioning only the important ones (based on David's comment as well: #4863 (comment))
I did a commit for this in that PR: f7e168f
What do you think?
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.
Seems reasonable 👍
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.
This is a super confusing name. Why does it have
Update
in the name twice?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.
I should probably rename this to
UpdateMixin
only. This class makes PUT to return 204 on success as PATCH.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.
These feel like they should be constants? What is a
superproject
? It still feels weird that it is a method name to me.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.
superproject
is an action name, defined by the class method underProjectViewSet
. We should apply the same permissions restrictions than for a detail action (since it only returns one superproject if exists).list
andretrieve
are DRF standard action names (same asupdate
orpartial_update
).We already talked about this at #5857 (comment)
(I'm adding this comment as a code comment)
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.
What is
source='*'
? It should likely have a comment.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.
*
it's standard for DRF but has a special meaning. It means that the whole object will be passed to theRepositorySerializer
, not just a specific field of it.(see docs https://www.django-rest-framework.org/api-guide/fields/#source)
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.
Does this also allow us to
PATCH
to Versions?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.
Yes. It makes the PUT and PATCH to behave in the same way (because of
UpdatePartialUpdateMixin
: returns 204 on both verbs).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.
It was already working like that, but I moved the
update
method to a Mixin since it's shared with another class now.