Expose version list for diffing, rework reviewers api to be more concise#10684
Conversation
We're now mounting any listing/browsing/future diffing APIs under the already existing addon/<addon_id>/ endpoint which makes the whole reviewer APIs behave more consistently. Also implements the listing of versions that can be used for browsing and diffing. Fixes #10432
83bb1de to
b80f7bc
Compare
|
Thanks, noted. |
|
Meh, travis didn't communicate the progress of that test-suite, both are actually green :) |
| def list(self, request, *args, **kwargs): | ||
| """Return all (re)viewable versions for this add-on. | ||
|
|
||
| Full list, no pagination.""" |
There was a problem hiding this comment.
Why no pagination? This seems like a bad idea, some add-ons have a lot of versions...
There was a problem hiding this comment.
Because that list is being shown in a drop-down of some kind and I personally don't think fetching the versions through pagination is a worthwhile undertaking.
I don't know how the current file-viewer handles huge amounts of versions but I'd guess, it just looks very stupid.
There was a problem hiding this comment.
yeah... (see https://reviewers.addons.mozilla.org/en-US/firefox/files/compare/1683453...1679753/ for 83 versions - it's slow)
But if it only every returns minimal information (like the serializer does now) then it's probably okay. It'll just get terrible quickly if we start returning nested serializers and other objects (e.g. translations). Maybe add a comment to the serializer for future us to not add anything crazy to it.
There was a problem hiding this comment.
I could even make the query a bit quicker by reducing the number of fields we are selecting to the ones we actually need which should be fine then.
There was a problem hiding this comment.
id, channel, version shouldn't be terrible.
There was a problem hiding this comment.
Best way to make it super fast (besides caching and accepting that the first request is going to be slow) would be to only select columns that are part of the same composite index ; that way MySQL wouldn't need to fetch the data from disk. Unfortunately we don't have an index that match those fields...
There was a problem hiding this comment.
I personally don't think we have to make it super-fast, fast enough should be fine as long as the loading doesn't time-out and stays responsive for the user.
| def list(self, request, *args, **kwargs): | ||
| """Return all (re)viewable versions for this add-on. | ||
|
|
||
| Full list, no pagination.""" |
There was a problem hiding this comment.
yeah... (see https://reviewers.addons.mozilla.org/en-US/firefox/files/compare/1683453...1679753/ for 83 versions - it's slow)
But if it only every returns minimal information (like the serializer does now) then it's probably okay. It'll just get terrible quickly if we start returning nested serializers and other objects (e.g. translations). Maybe add a comment to the serializer for future us to not add anything crazy to it.
…ise (mozilla#10684) * Expose version list for diffing, rework reviewers api to be more concise We're now mounting any listing/browsing/future diffing APIs under the already existing addon/<addon_id>/ endpoint which makes the whole reviewer APIs behave more consistently. Also implements the listing of versions that can be used for browsing and diffing. Fixes #10432 * Simplify view, don't rely on @action decorator but just make it the detail view * Smaller cleanups, fix permissions * More smaller cleanups, split list/retreive better * Fix link in docs * Improve tests, remove 'should_show_channel' and 'url' * Smaller performance optimization * Fix flake8 * Improve docs
5 participants
We're now mounting any listing/browsing/future diffing APIs under the
already existing addon/<addon_id>/ endpoint which makes the whole
reviewer APIs behave more consistently.
Also implements the listing of versions that can be used for browsing
and diffing.
Fixes mozilla/addons#6327
cc @willdurand @bobsilverberg and @kumar303 that the URLs will unfortunately change and now require an addon-id, that will be given through the link to the code-manager. That addon-id will then be used to retrieve the list of versions that can be used for diffing or other actions later.