All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED WONTFIX

Status

P5
enhancement
RESOLVED WONTFIX
10 years ago
3 years ago

People

(Reporter: TheOne, Assigned: andy+bugzilla)

Tracking

unspecified
4.x (triaged)

Details

(Whiteboard: [ReviewTeam])

(Reporter)

Description

10 years ago
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
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.
I agree with Cesar and am wontfixing this.  If you have strong feelings we can get more opinions from other editors.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 3

10 years ago
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.
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---
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.
(Reporter)

Updated

9 years ago
Blocks: 526809
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P5
Target Milestone: --- → Future
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.
Whiteboard: [required amo-editors]
No longer blocks: 526809
(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).
Duplicate of this bug: 597827
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.
Whiteboard: [required amo-editors] → [required amo-editors][ddn]
(Reporter)

Comment 9

8 years ago
I'd like to have it. Other comments?
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.
(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?
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.
Andy wrote the diff tool URLs if you have questions.  They were written with the github $id1...$id2 comparison style to make this easy.
Assignee: nobody → gkoberger
Whiteboard: [required amo-editors][ddn] → [required amo-editors]
Target Milestone: Future → Q2 2011
Target Milestone: Q2 2011 → 4.x (triaged)
Andy, I'm giving this to you since you know more about the diff stuff than I do :)
Assignee: gkoberger → amckay
Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...
Whiteboard: [required amo-editors] → [ReviewTeam]
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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago6 years ago
Resolution: --- → WONTFIX
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.