1172030 - Improve the display of validation results for automated signing validation

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Add-on Validation, defect, P3)

Description

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

The current validation page for unlisted and beta add-on shows a list of warnings, some of which may have the label Signing Severity: high. The warnings with that label prevent the file from being accepted automatically and signed. As such, they should be shown as errors instead of warnings, so they appear at the top and are easier for developers to identify and try to correct.

Comment 1

[Kris Maglione [:kmag]]

There are a few components to this that we need for unlisted add-ons in particular:

 1) Toggle to hide validation results which do not apply to automated signing (hidden by default)
 2) Toggle to hide validation results which are unchanged from previous uploads, and thus being ignored (hidden by default)
 3) Indicate which messages apply to signing, with their assigned severity (trivial, low, medium, high)
 4) Indicate which messages are unchanged from previous uploads, and being ignored.

 5) Display additional suggestions about how to address particular warnings. (These are being added on the validator side as part of bug 1183399)

For all add-ons, ideally:

 6) Group messages with the same ID/warning/description, and list the locations of individual occurrences below the descriptions.

An example of a current validation results page:

https://people.mozilla.org/~kmaglione/images/ccbd4e03b8e58096.png
https://people.mozilla.org/~kmaglione/images/990d795cb3830e0d.png

The results in the second screenshot with "Signing severity: high" are the messages that apply to automated signing (relevant to points 1 and 3 above).

These also happen to be two messages with the same ID/warning/description (point 6 above), which should ideally be collapsed into one message.

Comment 2

[Kris Maglione [:kmag]]

https://github.com/mozilla/olympia/pull/614

Comment 3

[[github robot]]

Commits pushed to master at https://github.com/mozilla/olympia

https://github.com/mozilla/olympia/commit/3edeca348c5e27d9172da2010afa36664bb440da
Bug 1172030: Part 1 - Store automated signing status on FileUpload before validating.

https://github.com/mozilla/olympia/commit/abaa4ab9ad472db77b904150a309afa741d92b16
Bug 1172030: Part 2 - Improve the display of validation results for auto-signed add-ons.

Comment 4

[Kris Maglione [:kmag]]

Leaving this open, since there are a few points from comment 1 which aren't addressed by this patch, and we still need UX input.

Comment 5

[Markus Jaritz [:designakt] (UX)]

(In reply to Kris Maglione [:kmag] from comment #4)
> Leaving this open, since there are a few points from comment 1 which aren't
> addressed by this patch, and we still need UX input.

What is the UX input you need on this bug. What questions are still open?
I will try to reply on all comments as far as I can guess what is needed.


(In reply to Jorge Villalobos [:jorgev] from comment #0)
> The current validation page for unlisted and beta add-on shows a list of
> warnings, some of which may have the label Signing Severity: high. The
> warnings with that label prevent the file from being accepted automatically
> and signed. As such, they should be shown as errors instead of warnings, so
> they appear at the top and are easier for developers to identify and try to
> correct.

This sounds like a good idea. Something that blocks the process should be show in the summary. So either having an additional category "Singing" in the Summary, or for each category increase signing warnings to errors as Jorge suggested.


(In reply to Kris Maglione [:kmag] from comment #1)
>  1) Toggle to hide validation results which do not apply to automated
> signing (hidden by default)
>  2) Toggle to hide validation results which are unchanged from previous
> uploads, and thus being ignored (hidden by default)

If we hide those results by default, why would the users need to see them?
How many people would want to see the extended results?


>  3) Indicate which messages apply to signing, with their assigned severity
> (trivial, low, medium, high)

As I see in your screenshot, this is already indicated…
What does Signing Severity mean? Does it mean no signing? Or just more intense manual review?


>  4) Indicate which messages are unchanged from previous uploads, and being
> ignored.

Is this the same topic as in 2) ?


>  5) Display additional suggestions about how to address particular warnings.
> (These are being added on the validator side as part of bug 1183399)

sounds like this is solved.


>  6) Group messages with the same ID/warning/description, and list the
> locations of individual occurrences below the descriptions.

Great Idea. Would make the results much more readable.

Comment 6

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

(In reply to Markus Jaritz [:maritz] (UX) from comment #5)
> (In reply to Kris Maglione [:kmag] from comment #1)
> >  1) Toggle to hide validation results which do not apply to automated
> > signing (hidden by default)
> >  2) Toggle to hide validation results which are unchanged from previous
> > uploads, and thus being ignored (hidden by default)
> 
> If we hide those results by default, why would the users need to see them?
> How many people would want to see the extended results?

The other warnings can relate to code quality issues or even potential problems in the add-on, like broken compatibility. I think most developers will be focused on getting automatic validation, but the other issues should be available for those looking to improve their add-on as much as possible.

> >  3) Indicate which messages apply to signing, with their assigned severity
> > (trivial, low, medium, high)
> 
> As I see in your screenshot, this is already indicated…
> What does Signing Severity mean? Does it mean no signing? Or just more
> intense manual review?

As I understand it, trivial and low severity warnings will block automatic signing only the first time they come up. On following submissions they will be ignored. The others will always block automatic signing, unless a reviewer marks them as unimportant.

> >  4) Indicate which messages are unchanged from previous uploads, and being
> > ignored.
> 
> Is this the same topic as in 2) ?

See my answer for (3).

Comment 7

[Mark Striemer [:mstriemer]]

I just had a conversation with Markus and here's what we came up with:

1) On the submission page instead of showing "0 errors and 3 messages" show "1 warning and 3 messages" if there are warnings that block automatic signing.
2) On the full results page show the yellow warning icon at the top for tests (General Tests, Security Tests, etc) if there are any warnings that block validation.
3) Instead of upgrading warnings that block validation to errors downgrade trivial warnings to notices (we should probably do the same for non-trivial warnings that are being ignored but we didn't discuss that).
4) Within a grouping of tests order the warnings by their severity so that higher severity warnings are at the top.

Comment 8

[Markus Jaritz [:designakt] (UX)]

Thank you for the summary. Sounds great.
One comment:

(In reply to Mark Striemer [:mstriemer] from comment #7)
> 1) On the submission page instead of showing "0 errors and 3 messages" show
> "1 warning and 3 messages" if there are warnings that block automatic
> signing.

I think we should use the same wording as on the validator: X errors and X warnings

Comment 9

[[github robot]]

Commits pushed to master at https://github.com/mozilla/amo-validator

https://github.com/mozilla/amo-validator/commit/dc2d281521b2f369fb3ca1afe961c7de7063a5a6
Change trivial unlisted warnings to notices (bug 1172030)

https://github.com/mozilla/amo-validator/commit/c74b05338789444eabf5eebdfd0750e34eb0142f
Merge pull request #329 from mstriemer/trivial-unlisted-as-notice-1172030

Change trivial unlisted warnings to notices (bug 1172030)

Comment 10

[[github robot]]

Commit pushed to master at https://github.com/mozilla/olympia

https://github.com/mozilla/olympia/commit/37681653aef21ffbd6cb646d722a88c382b4bdd1
Improve the display of validation results (bug 1172030)

Mention warnings in the validation summary on submission.
Use warning icon in validation results for tests with warnings.
Sort validation messages by signing severity.
amo-validator==1.9.1 trivial signing severity as notices.
Fixes #777