704384 - Compare links for old versions of an add-on always compare against latest public version

Status: RESOLVED DUPLICATE of bug 661909

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

Description

[Erik Vold]

When I look at https://addons.mozilla.org/en-US/editors/review/add-on-builder-helper, pg 3, and open each compare in a tab, notice the harness-options.json file's following line:

"classID": "{306665a3-08fc-4127-b412-9384f77abb48}", 

with a different random classID, which is not possible.

So I see:
v1. A
v2. A -> B
v3. A -> C
v4. A -> D

When I expected to see:

v1. A
v2. A -> B
v3. B -> C
v4. C -> D

But the rest of the diffs do seem to follow that pattern, it's just the one line...

I also see a "error undefined" in red at the top of most pages which may be related.

Comment 1

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

"error undefined" is bug 703991. Let's wait for that one to be fixed and see if it makes a difference here.

Comment 2

[Andrew Williamson [:eviljeff]]

Its the compare links that are unexpected (I won't say broken as I'm not sure what the intended behaviour is)

1.2.1.1 (aka 137624) is
https://addons.mozilla.org/en-US/firefox/files/compare/137624...136018/
1.2.1 (aka 136018) is
https://addons.mozilla.org/en-US/firefox/files/compare/136018...136018/
0.0.20 (aka 131985) is
https://addons.mozilla.org/en-US/firefox/files/compare/131985...136018/

So every compare link is comparing that version to 1.2.1.  I assume the intended behaviour is that version compared to the previous full/prelim approved version.

Comment 3

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

Hmm, maybe that summary change won't clarify things much.

If we have these versions: A < B < C < D < E, and they are all approved, clicking on the compare link should compare them against the previous version, not against E every time.

We want to know the differences with the last approved version that preceded whatever version we want to compare.

Comment 4

[Andrew Williamson [:eviljeff]]

(In reply to Jorge Villalobos [:jorgev] from comment #3)
> Hmm, maybe that summary change won't clarify things much.
> 
> If we have these versions: A < B < C < D < E, and they are all approved,
> clicking on the compare link should compare them against the previous
> version, not against E every time.

atm they're all being compared against D.  IMO being compared against E is probably the most useful.

Comment 5

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

I'm not so sure about usefulness, but it would seem logical that the compare feature compares against a version in the past, not the future. One usually diffs back, not forward.

There's a separate feature request to allow arbitrary comparisons (without URL juggling), which should make this all much easier. For the moment we should figure out how we want it to work by default.

Comment 6

[Andrew Williamson [:eviljeff]]

I suppose logically each version should be against the previous (as the link is in each version).

Comment 7

[Erik Vold]

(In reply to Andrew Williamson [:eviljeff] from comment #6)
> I suppose logically each version should be against the previous (as the link
> is in each version).

yes, that is how things used to work right?

Comment 8

[Erik Vold]

(In reply to Andrew Williamson [:eviljeff] from comment #2)
> Its the compare links that are unexpected (I won't say broken as I'm not
> sure what the intended behaviour is)
> 
> 1.2.1.1 (aka 137624) is
> https://addons.mozilla.org/en-US/firefox/files/compare/137624...136018/
> 1.2.1 (aka 136018) is
> https://addons.mozilla.org/en-US/firefox/files/compare/136018...136018/
> 0.0.20 (aka 131985) is
> https://addons.mozilla.org/en-US/firefox/files/compare/131985...136018/
> 
> So every compare link is comparing that version to 1.2.1.  I assume the
> intended behaviour is that version compared to the previous full/prelim
> approved version.

I can't imagine why that would be intended.

This makes the problem much more clear though so thanks.

Comment 9

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

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...

Comment 10

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

Resetting a bunch of missed milestones. Sorry for the bugspam.

Comment 11

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

This is low priority. We should find a way to make it easy to compare arbitrary versions, and in this case I think it makes sense to have the comparison use the previous version, not the latest one.

Comment 12

[Kris Maglione [:kmag]]

Merging these since my patch takes care of both of them.