Add the approve_addons management command (bug 1199234) #697
Conversation
|
|
||
| addons = Addon.with_unlisted.filter( | ||
| guid__in=args, | ||
| status__in=[amo.STATUS_NOMINATED, amo.STATUS_UNREVIEWED]) |
There was a problem hiding this comment.
Can we add amo.STATUS_PENDING as well, please?
|
What is the |
|
r+wc |
magopian
force-pushed
the
1199234-bulk-approval-script
branch
from
8e07d76
to
e60ffd0
Compare
|
I updated the PR, thanks @wagnerand. The |
|
Thank you! |
| # Provide the file to review/sign to the helper. | ||
| helper.set_data({'addon_files': [addon.latest_version.files], | ||
| 'comments': 'bulk approval'}) | ||
| if addon.status in [amo.STATUS_NOMINATED, amo.STATUS_PENDING]: |
There was a problem hiding this comment.
We need to make sure STATUS_PENDING is what we want (cf #700 (comment))
There was a problem hiding this comment.
I'd argue that it's instead STATUS_LITE_AND_NOMINATED: https://github.com/mozilla/olympia/pull/700/files#diff-0383f36a36f392e46d9070ecdc5184a3R17
There was a problem hiding this comment.
We can add STATUS_LITE_AND_NOMINATED, but that is not about pending updates.
There was a problem hiding this comment.
As discussed, I'm pretty sure there's no specific status for pending updates.
There was a problem hiding this comment.
Ok, I tracked this down, it should be:
if addon.status in [amo.STATUS_NOMINATED, amo.STATUS_LITE_AND_NOMINATED, amo.STATUS_PUBLIC]:
STATUS_PUBLIC is for pending updates.
magopian
force-pushed
the
1199234-bulk-approval-script
branch
from
e60ffd0
to
bd8259a
Compare
|
Updated |
| addons = Addon.with_unlisted.filter( | ||
| guid__in=args, | ||
| status__in=[amo.STATUS_NOMINATED, amo.STATUS_UNREVIEWED, | ||
| amo.STATUS_PENDING]) |
There was a problem hiding this comment.
We ruled out amo.STATUS_PENDING. Let's delete it?
There was a problem hiding this comment.
Good catch, I rewrote it to make it DRYer ;)
magopian
force-pushed
the
1199234-bulk-approval-script
branch
from
bd8259a
to
24a30fe
Compare
|
|
||
| FULL_REVIEW_STATUSES = [amo.STATUS_NOMINATED, amo.STATUS_LITE_AND_NOMINATED, | ||
| amo.STATUS_PUBLIC] | ||
| PRELIM_REVIEW_STATUSES = [amo.STATUS_UNREVIEWED, amo.STATUS_LITE] |
There was a problem hiding this comment.
Can we move these to constants/base.py?
|
This could really use some tests, but I suppose we can do without them for now. r+wc |
magopian
force-pushed
the
1199234-bulk-approval-script
branch
from
24a30fe
to
b923a60
Compare
|
Updated. Indeed, it would be nice to have tests... I came up with this quick and dirty script that should only be used a couple of time, and only during the "signing transition" (where developers suddenly realize they need to upload all their add-ons to have them signed). |
magopian
force-pushed
the
1199234-bulk-approval-script
branch
2 times, most recently
from
8c710e5
to
ba3fdf7
Compare
|
Completely reworked the code, added tests, I'm not a bit more confident about it ;) |
| assert file1.reload().status == file_status | ||
| assert file2.reload().status == file_status | ||
|
|
||
| return (addon, file1, file2, review_type) |
There was a problem hiding this comment.
This "parametrized pytest fixture" will result in the tests using it to be run once for each "use case". This will result in the following:
There was a problem hiding this comment.
Also, this is not the usual pytest fixture. This is a pytest fixture that uses parametrization. In this case, we're combining the two super-powers of pytest (http://pytest.org/latest/fixture.html#parametrizing-a-fixture), which makes it a bit more hairy than usual.
magopian
force-pushed
the
1199234-bulk-approval-script
branch
from
ba3fdf7
to
47dc3ab
Compare
| 'comments': 'bulk approval'}) | ||
| if review_type == 'full': | ||
| # Already fully reviewed, or waiting for a full review. | ||
| helper.handler.process_public() |
There was a problem hiding this comment.
Does any logging occur in this method? Logging that this was a result of a script would be nice.
There was a problem hiding this comment.
Yep, it's all taken care of already, it adds a log stating that it's an approval done by the "mozilla" user, with the comment bulk approval from three lines above
There was a problem hiding this comment.
I added a test for the presence of those logs
| ReviewerScore.award_points(self.request.amo_user, self.addon, status) | ||
| if self.request: | ||
| ReviewerScore.award_points(self.request.amo_user, self.addon, | ||
| status) |
There was a problem hiding this comment.
are there tests for the skipping of award points?
There was a problem hiding this comment.
I don't think so, I'll double check
There was a problem hiding this comment.
Added, thanks for pointing it out!
|
I think a bit of logging would be good, so we can find out a month or two later exactly how an addon got approved. Other than that lgtm. |
magopian
force-pushed
the
1199234-bulk-approval-script
branch
from
47dc3ab
to
0432e85
Compare
|
Added some logging, tests, and chunking, thanks for the review @andymckay, would you mind giving it another look? Also maybe @wagnerand and @kmaglione ? |
|
lgtm |
Add the approve_addons management command (bug 1199234)
Fixes bug 1199234