498072 - Enable diff viewer for xpi files for re-nominated add-ons

Status: RESOLVED WONTFIX

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

Description

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

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11
Build Identifier: 

The usual reason for an editor deciding that a add-on isn't ready for the public yet, is that there a things that have to be corrected before (like namespacing etc).
When the user update his add-on and re-nominated, it's just like an update. Enabling the diff viewer in this case would be very helpful.

Reproducible: Always

Comment 1

[u278084]

I don't think this is a good idea because when you find a code problem (ie. namespacing) you stop reviewing code and inform the author. So when the author re-uploads, the code that changed from the public version will no longer be indicated.

Comment 2

[Wil Clouser [:clouserw]]

I agree with Cesar and am wontfixing this.  If you have strong feelings we can get more opinions from other editors.

Comment 3

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

Instead of showing a "Diff" link, we could show a dropdown-list where the reviewer can choose the version he wants to compare the current nomination to.

I don't know how complex this would be.

Comment 4

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

I'd be for an opt-in implementation. By default you get the non-diffed code, and as in comment #3, a drop down where you can compare against past versions, or the latest at least.

Comment 5

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

Removing tracking bug 526809 to cut back on bugmail. From now on let's use the [required amo-editors] whiteboard flag to keep track of our bugs.

Comment 6

[Axel Grude [:realRaven]]

(In reply to comment #4)
> I'd be for an opt-in implementation. By default you get the non-diffed code,
> and as in comment #3, a drop down where you can compare against past versions,
> or the latest at least.

Agree to this comment - there is 2 approaches of reviewing - incremental (stopping at first showstopper error and asking the author for update) and complete (in that case a diff would be perfect, to see whether the Add-On author has addressed any issues of the last refusal).

Comment 8

[Wil Clouser [:clouserw]]

Is this still something we want?  Andy wrote the tools in such a way that this should be relatively easy, we just need a place to add the link.

Or if we don't want it, please wontfix.

Comment 9

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

I'd like to have it. Other comments?

Comment 10

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

Here's how I think it should work. For all nominations, next to the View Contents link for the version being reviewed, we have a checkbox that says "Enable Diff". If clicked, the checkbox is replaced with the diff link and a warning message appears reminding the editor that all of the source code should be reviewed.

Comment 11

[Wil Clouser [:clouserw]]

(In reply to comment #10)
> Here's how I think it should work. For all nominations, next to the View
> Contents link for the version being reviewed, we have a checkbox that says
> "Enable Diff". If clicked, the checkbox is replaced with the diff link and a
> warning message appears reminding the editor that all of the source code should
> be reviewed.

I'm not an editor, but "Enable diff" sounds like an extra step - what is the point of it?

Comment 12

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

I don't want editors to confuse nominations and updates. It's easy to forget what you're reviewing if you open multiple review pages from different queues, or if you walk away and do something else for a while and then come back to the page. If the "compare" link is available right away, it's easy to incorrectly assume this is an update being reviewed and only the source changes should be looked at.

I want this to be explicitly enabled so the editor is more aware of what's going on.

Comment 13

[Wil Clouser [:clouserw]]

Andy wrote the diff tool URLs if you have questions.  They were written with the github $id1...$id2 comparison style to make this easy.

Comment 14

[Gregory Koberger (:gkoberger)]

Andy, I'm giving this to you since you know more about the diff stuff than I do :)

Comment 15

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

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

Comment 16

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

I don't think this is all that necessary. Nominations should be reviewed entirely, and rejected nominations have their files disabled, making them inaccessible to the diff viewer.