Implement addon PUT as full-update or create #19342
Conversation
eviljeff
force-pushed
the
15353-api-lang-pack-submit
branch
from
f36cfee to
2b5d2d1
Compare
eviljeff
force-pushed
the
15353-api-lang-pack-submit
branch
from
2b5d2d1 to
e484af6
Compare
src/olympia/addons/views.py
Outdated
| serializer = self.get_serializer(data=request.data) | ||
| serializer.is_valid(raise_exception=True) | ||
| serializer.save(guid=self.kwargs['guid']) | ||
|
|
||
| return Response(serializer.data, status=status.HTTP_201_CREATED) |
There was a problem hiding this comment.
Couldn't you use self.create(request, *args, **kwargs) here instead ?
There was a problem hiding this comment.
And, related to that... wouldn't self.action be incorrect here with your current code ?
There was a problem hiding this comment.
It is possible to use self.create (and I almost did) but we'd need to override the save method to include guid (and either check the action in there so we only did it for PUT) so I didn't think it helped much. I can though?
There was a problem hiding this comment.
I'm not following what you mean about self.action - can you expand on it and/or include an example scenario?
There was a problem hiding this comment.
When you reach this code, self.action is still update, but we're doing a creation
There was a problem hiding this comment.
It is possible to use
self.create(and I almost did) but we'd need to override thesavemethod to includeguid(and either check the action in there so we only did it for PUT) so I didn't think it helped much. I can though?
No, you're right, it's not worth it.
There was a problem hiding this comment.
When you reach this code,
self.actionis stillupdate, but we're doing a creation
mmm, yeah. I think by that point the use of self.action is over, and the criteria applied for update tends to be stricter than create anyway, but I should update self.action I guess.
fixes mozilla/addons#7908
This allows submission of all types of add-on, not just langpacks (as mozilla/addons#7908 talks about), but should we limit PUT on
addon/<slug>/to just langpacks as per issue? Pros: The existing signing API allowed this style PUT to create or update so it'd be restoring that functionality. Cons: it's generally not a "good idea" to have an API where you don't know what the request will do - it could create a new version for an existing add-on, or it could create a new add-on - both outcomes could be a surprise if you weren't expecting it. There's also some subtle differences around metadata in the manifest (the name, description, and homepage would be extracted for a new add-on; they wouldn't be for a new version) which are non-obvious.Another feature this patch restores from the signing API (partially) is the ability to specify the GUID via the url if it's not in the manifest. Pros: It's a convenient feature; Cons: it's against the direction of travel regarding future changes to require the manifest specify the guid explicitly so it's possibly unwise to encourage that use.
Note: the issue talks about auto-generating the summary + license + category for a langpack - the patch doesn't do that as it's now trivial for the client to supply this in the POST data.