Addons-server: /files/compare/ sometimes doesn't take in consideration manual reviews.

[spoji]

Describe the problem and steps to reproduce it:

This Review page gives the reviewer a compare link on version 2.0.1 that show a diff between 2.0.1 and 1.2.3

What did you expect to happen?

The compare link should show a diff between 2.0.1 and 2.0.0. Version 2.0.0 as been previously approved by a human.

Version 2.0.0 was not reviewed by the auto-approval script. This is probably why this problem occurs.

Anything else we should know?

Issue seems to be related to code
src\olympia\reviewers\views.py#744-746

This looks like a "Good first bug". If someone is willing to mentor me a bit on this, I would love to try it out.

[diox]

Hold on. Did you approve a web extension yourself ?

[spoji]

Yes. I've probably opened the page and reviewed it before the auto-approval script had time to run.

[diox]

Sneaky :)

So, what's happening is that the version compare code already handles manually approved versions in theory, but the here the version was manually approved yet the auto approval script ran on it. It was locked (I'm guessing by yourself), so it didn't approve it, but left a weight.

The version compare code needs an additional condition in the queryset to handle this - checking that an autoapproval summary with a negative verdict is present.

I don't have time to mentor this properly right now, I've made an untested patch that would need an unit test to go with:

diff --git a/src/olympia/reviewers/views.py b/src/olympia/reviewers/views.py
index 8338b8c..df23533 100644
--- a/src/olympia/reviewers/views.py
+++ b/src/olympia/reviewers/views.py
@@ -788,8 +788,12 @@ def review(request, addon, channel=None):
         show_diff = version and (
             addon.versions.exclude(id=version.id).filter(
                 # We're looking for a version that was either manually approved
-                # or auto-approved but then confirmed.
+                # (either it has no auto approval summary, or it has one but
+                #  with a negative verdict because it was locked by a reviewer
+                #  who then approved it themselves), or auto-approved but then
+                # confirmed.
                 Q(autoapprovalsummary__isnull=True) |
+                Q(autoapprovalsummary__verdict=amo.NOT_AUTO_APPROVED) |
                 Q(autoapprovalsummary__verdict=amo.AUTO_APPROVED,
                   autoapprovalsummary__confirmed=True)
             ).filter(

[CuriousLearner]

Hey,

I'd like to take up this task. I'll issue a PR soon :)

[CuriousLearner]

PR issued: https://github.com/mozilla/addons-server/pull/7961

[AlexandraMoga]

I have verified this on AMO stage by following the steps provided:

1. submitted a new add-on version (1.0) (auto-approved)
2. submitted second version (1.1) while add-on was locked by a reviewer and approved it manually
3. submitted a third version (1.2) which was auto-approved.

I've checked the Compare tool for version 1.2 and the version file was correctly compared with the previous version 1.1.
Here is the add-on I've tested with: https://reviewers.addons.allizom.org/en-US/reviewers/review/compare-file

I'm going to mark this issue as verified fixed.