663252 - Uncollapse the latest N reviews in add-on review logs

Status: VERIFIED FIXED

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

Description

[Kris Maglione [:kmag]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110513 Firefox/6.0a1
Build Identifier: 

Editors nearly always need to read the previous review for an add-on before completing their reviews, to make sure that comments from previous editors were addressed. Locally, I've been uncollapsing the latest three reviews by default, but it would be nice if it were configurable.

Reproducible: Always

Comment 1

[Gregory Koberger (:gkoberger)]

Configurable by whom?

Comment 2

[Kris Maglione [:kmag]]

(In reply to comment #1)
> Configurable by whom?

By individual users/editors.

Comment 3

[Wil Clouser [:clouserw]]

We talked on IRC earlier about expanding a default number of these so this bug can be for that.

The configuration is out of scope for that and something I would wontfix due to complexity and time constraints.

Comment 4

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

We should uncollapse at least the previous 3 history items. I agree that making it configurable is overkill.

Comment 5

[Kris Maglione [:kmag]]

I'm not terribly concerned about configurability, but the editor tools migration mockups contained a preferences page, as I recall, so it seemed a relatively trivial addition. Failing that, I'd be satisfied with a simple cookie- or localStore-based popup panel, which I can't imagine would be the work of more than a minute or two.

Comment 6

[Kris Maglione [:kmag]]

I should add that this probably shouldn't include unreviewed versions.

Comment 7

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

I think it should. It can be confusing otherwise.

Comment 8

[Kris Maglione [:kmag]]

Well, as you can guess, I've already implemented it locally and I don't find it confusing, especially with the contents of the review marked per bug 663250. It seems impractical to take up space with empty reviews when all you're interested in is previous editors' comments.

Comment 9

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

I'm sure you don't, but most people prefer simplicity, and it's not obvious why a number of reviews are collapsed in between when you look at the page for the first time, specially considering that having unreviewed versions is a bit of an edge case.

Comment 10

[Kris Maglione [:kmag]]

Well, as it happens, I think that most people prefer simplicity less than I do, but that's neither here nor there.

Again, I think combined with bug 663250 this feels completely natural. Versions without reviews (in my implementation) appear with an empty title after the date, while others contain their review status. They might alternatively say ‘Unreviewed’, but either way the intention is clear. And anyone who finds it confusing would, I think, understand the reasoning immediately on expanding one or two of them and seeing them empty.

Comment 11

[Gregory Koberger (:gkoberger)]

Jorge, what are we going with?

Comment 12

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

I prefer to show the last N ones regardless of state. But if you think Kris' suggestion is better, go for it.

Comment 13

[Gregory Koberger (:gkoberger)]

I originally made it so that the first 3 were open and the rest were collapsed, but that looked a bit weird (especially since there were only 5 total).  So, I went with a toggle button-- either just the first is open, or they all are.  The state persists via local storage, so it works a bit like a setting.

Kris, it's not exposed anywhere but you can set the number being displayed in local storage.  Just run:

  var s = z.Storage(); s.set('editor_history') = <NUMBER>;

If you find that this doesn't work, let me know and we can rework it.

http://github.com/jbalogh/zamboni/commit/65a04f8

Comment 14

[krupa raj[:krupa]]

Kris,

Is the fix satisfactory? If yes, can you verify the bug? Thanks!

Comment 15

[Kris Maglione [:kmag]]

Sorry, I lost track of the bug.

I can't say that I'm especially happy with the fix. I can change it for my part, but I'm more concerned about other editors completing a review without noticing important comments by past editors. This seems to me to be a major issue, given that we often allow reviews to pass contingent on issues being fixed before the next revision.

Comment 16

[Gregory Koberger (:gkoberger)]

My suggestions:
 - Change pagination to 10 per page, and show 3/10 (3/5 just looks weird).
 - Open all by default, and allow them to collapse all but one by clicking "Toggle."

Comment 17

[Gregory Koberger (:gkoberger)]

Those were two separate solutions, by the way.

Comment 18

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

(In reply to comment #16)
>  - Change pagination to 10 per page, and show 3/10 (3/5 just looks weird).

I think this is a good approach.

Comment 19

[krupa raj[:krupa]]

I filed bug 665970 for comment 16.
I am not filing any bugs about this feature since it will soon change.

The latest 3 entries are uncollapsed by default. verified at https://addons.allizom.org/en-US/editors/review/firepop?num=2

Comment 20

[krupa raj[:krupa]]

Attachment: post-fix screenshot

Comment 21

[krupa raj[:krupa]]

Also, I filed bug 666115

Comment 22

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

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