1164488 - Automatic validation and signing should also do everything a manual review does

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect)

Description

[Mathieu Agopian [:magopian]]

At the moment, the code executed when a file is automatically signed/reviewed (if it passes validation) is pretty basic, and might miss some things that are done in the manual review path.

Automatic review/signing is done in:
- https://github.com/mozilla/olympia/blob/641c73776613bf27838ac1dcc8e5fb2cbb0bc71d/apps/devhub/views.py#L1219
- https://github.com/mozilla/olympia/blob/641c73776613bf27838ac1dcc8e5fb2cbb0bc71d/apps/devhub/views.py#L1256
- https://github.com/mozilla/olympia/blob/641c73776613bf27838ac1dcc8e5fb2cbb0bc71d/apps/devhub/views.py#L1389

Manual review/signing is done in:
- https://github.com/mozilla/olympia/blob/641c73776613bf27838ac1dcc8e5fb2cbb0bc71d/apps/editors/helpers.py#L656
- https://github.com/mozilla/olympia/blob/641c73776613bf27838ac1dcc8e5fb2cbb0bc71d/apps/editors/helpers.py#L718
- https://github.com/mozilla/olympia/blob/641c73776613bf27838ac1dcc8e5fb2cbb0bc71d/apps/editors/helpers.py#L767
- https://github.com/mozilla/olympia/blob/641c73776613bf27838ac1dcc8e5fb2cbb0bc71d/apps/editors/helpers.py#L821

Comment 1

[Mathieu Agopian [:magopian]]

Also, an automatic validation/signature should be logged as such and be visible in the addon history log.

Comment 2

[Mathieu Agopian [:magopian]]

Also, automatically reviewed/signed files shouldn't be visible in the review queues: https://bugzilla.mozilla.org/show_bug.cgi?id=1121238#c11

Comment 3

[Mathieu Agopian [:magopian]]

PR: https://github.com/mozilla/olympia/pull/563

STR: when uploading an unlisted addon that passes validation, make sure that:
1/ the log appears in the "recent changes" for the granted preliminary review
2/ that an email is sent to tell the developer that his addon got (automatically) preliminary reviewed (check the "fake mails" in the admin)
3/ that the version is properly seen as preliminary reviewed in the devhub, and signed

Comment 4

[Mathieu Agopian [:magopian]]

Fixed in https://github.com/mozilla/olympia/commit/8f05f50ac905fa0c2d9bdd1f4684d3907c36c38b

Comment 5

[Madalin Cotetiu]

Verified in FF38(Win7) in addons-dev.allizom.org
1. The granted preliminary review appears in the logs: http://screencast.com/t/2j1SsIwDtI

2. The email is sent but from what I'm seeing it looks like a mail for a manual review: http://screencast.com/t/RLoGCq6FwG  Shouldn't be something like: "
Your add-on, $name, $version, has passed our automatic tests and is now signed and ready for you to download at $VERSION_URL"     I thin the messages are reversed in this etherpad https://etherpad.mozilla.org/unlisted-dev-emails

3. The version is seen as prelim. reviewed and is signed.
Also https://bugzilla.mozilla.org/show_bug.cgi?id=1121238#c11 is no longer reproducing.

I think we can close this bug and track 2. in Bug 1121238 (If there is really something wrong, maybe I'm mistaking)

Comment 6

[Mathieu Agopian [:magopian]]

Regarding point 2, I didn't use this email (maybe I should have?), but instead used the comment "automatic validation", I thought it was understandable.

Comment 7

[Madalin Cotetiu]

Probably that would work as well. I think we should ask :jorgev anyway about this as he is the one who created that etherpad.

Comment 8

[Jorge Villalobos [:jorgev] (he/him)]

Madalin is correct, the messages in the etherpad are reversed, so they need to be switched around. I updated the etherpad to reflect the correct messages now.

Comment 9

[Madalin Cotetiu]

I will close this bug and as per comment 9 will reopen Bug 1121238

Comment 10

[Emiliano Heyns]

Will automatically signed unlisted extensions get review status 'preliminary'? What would that mean in practice?

Comment 11

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Emiliano Heyns from comment #10)
> Will automatically signed unlisted extensions get review status
> 'preliminary'? What would that mean in practice?

Please use mailing lists for such questions/discussions.