802561 - Add function to force re-validate all manifests

Status: VERIFIED FIXED

(Marketplace Graveyard :: Reviewer Tools, defect)

Description

[Andrew Williamson [:eviljeff]]

During the re-review job manifests are only revalidated when they've changed.  This is fine normally but when there are validator changes then they aren't picked up.  What is needed is some way of telling the re-review job to check ALL manifests, regardless of whether they've changed or not, and flagging them for re-review as appropriate.
Either on demand or during the next job run is fine.

Comment 1

[Tarek Ziadé (:tarek)]

From my first look at the code I assume I need to:

1 - loop over all apps and add them in the RereviewQueue table
2 - get 1- triggered when a validator changes

for 2- what's unclear to me is where to find the validators and how/where they are modified.

The only 'validators' reference I found was in mkt/developers/forms.py and I am not sure this is related.

Comment 2

[Andrew Williamson [:eviljeff]]

(In reply to Tarek Ziadé (:tarek) from comment #1)
> From my first look at the code I assume I need to:
> 
> 1 - loop over all apps and add them in the RereviewQueue table

Not all apps (I'll be there for ages clearing them all!).  All app manifests should be validated and then /just/ those that fail added to the RereviewQueue table as per the normal criteria.

> 2 - get 1- triggered when a validator changes

Or a manual trigger that can be set somewhere in the /admin pages.

> for 2- what's unclear to me is where to find the validators and how/where
> they are modified.
> 
> The only 'validators' reference I found was in mkt/developers/forms.py and I
> am not sure this is related.

https://github.com/mozilla/app-validator

Comment 3

[Andrew Williamson [:eviljeff]]

this may help:
https://github.com/mozilla/zamboni/blob/master/mkt/webapps/tasks.py#L91

its where the validator check skips further checks if the hash hasn't changed (and we want it not to skip it)

Comment 4

[Rob Hudson [:robhudson]]

We have a cron that checks all public hosted apps each day:
https://github.com/mozilla/zamboni/blob/master/scripts/crontab/crontab.tpl#L47

Currently, it fetches the manifest and only if it has changed does it re-validate with the app validator (https://github.com/mozilla/app-validator). If there are errors during validation, it flags it for re-review (by adding it to the RereviewQueue table). If there are no errors, it does further checks.

My original thought was to change the `_update_manifest` call in mkt/webapps/tasks.py to take an extra arg that tells it to bypass the file hash check. Then tie this to a ./manage.py command so we can manually kick this off when the reviewers want us to.

Comment 5

[Andrew Williamson [:eviljeff]]

(In reply to Rob Hudson [:robhudson] from comment #4)
> My original thought was to change the `_update_manifest` call in
> mkt/webapps/tasks.py to take an extra arg that tells it to bypass the file
> hash check. Then tie this to a ./manage.py command so we can manually kick
> this off when the reviewers want us to.

Can this be somewhere in Server Tools menu on /admin instead (like the other similar functions)?  I'd prefer not have to have to ask IT to run a command each time.

Comment 6

[Tarek Ziadé (:tarek)]

ok so tell me if this is correct:

1. add a flag to _update_manifest so the hash control is by-passed
2. add a button somewhere in admin so people can run this command through-the-web

Comment 7

[Andrew Williamson [:eviljeff]]

(In reply to Tarek Ziadé (:tarek) from comment #6)
> ok so tell me if this is correct:
> 
> 1. add a flag to _update_manifest so the hash control is by-passed
> 2. add a button somewhere in admin so people can run this command
> through-the-web

yes.  A page and a link from Manage Apps or Server Tools.

Comment 8

[Tarek Ziadé (:tarek)]

started the work at https://github.com/tarekziade/zamboni/compare/802561-revalidate-manifest

adding 2. now

Comment 9

[Tarek Ziadé (:tarek)]

in the UI, 

do you want to be able to pick which apps you want to force an update on, or do you want a big button that loops over ALL apps ? if it's the latter is this filter the right one ?

- type = amo.ADDON_WEBAPP
- is_packaged = False
- status = amo.STATUS_PUBLIC
- disabled_by_user = False

Comment 10

[Andrew Williamson [:eviljeff]]

(In reply to Tarek Ziadé (:tarek) from comment #9)
> in the UI, 
> 
> do you want to be able to pick which apps you want to force an update on, or
> do you want a big button that loops over ALL apps ? if it's the latter is
> this filter the right one ?
> 
> - type = amo.ADDON_WEBAPP
> - is_packaged = False
> - status = amo.STATUS_PUBLIC
> - disabled_by_user = False

ALL apps, and yeah, that filter. 

(The same as the cron job uses https://github.com/mozilla/zamboni/blob/master/apps/addons/management/commands/process_addons.py#L31)

Comment 11

[Tarek Ziadé (:tarek)]

Feature added at : https://github.com/tarekziade/zamboni/compare/802561-revalidate-manifest

I guess it's ready for a first review

Comment 12

[Andrew Williamson [:eviljeff]]

If the contention issues in bug 804634 do get sorted and it goes ahead we'll need this function so adding as a blocker.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Andrew Williamson [:eviljeff] from comment #12)
> If the contention issues in bug 804634 do get sorted and it goes ahead we'll
> need this function so adding as a blocker.

Btw - you guys are good to move forward. Contention issues are resolved.

Comment 14

[Tarek Ziadé (:tarek)]

Pushed at : https://github.com/mozilla/zamboni/commit/4194ea1c8fd016aefc93ae4d53897c0bac56a522

Comment 15

[Andrew Williamson [:eviljeff]]

Tested on -dev and looks like its working. Thanks!