290191 - Use first-review and second-review for NSPR, NSS, and JSS.

Status: RESOLVED WONTFIX

(bugzilla.mozilla.org :: Administration, task)

Description

[Wan-Teh Chang]

I recently noticed that some products, such as Toolkit,
use "first-review" and "second-review" instead of
"review" and "superreview" on their patches.  Here is
an example:
https://bugzilla.mozilla.org/attachment.cgi?id=180598&action=edit

I'd like the same for our products: NSPR, NSS, and JSS,
because we don't have superreviewers.

Thanks.

Comment 1

[timeless]

one hazard, currently i can do this but you'll lose all old requests. i'd need
justdave's help if you want to keep them. (if you don't, say the word and i'll
do this w/in 12hrs).

Comment 2

[Wan-Teh Chang]

We need to keep the old requests.  This is not urgent.
Thanks!

Comment 3

[Robert Relyea]

I've cleared out my review requests...

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I am willing to take a snapshot of my requests (both those I made to other,
and those others made to me) and then work to restore them after the change
is made to first and second.  I like that idea, though I think it is 
merely cosmetic.  Please let us all know one business day in advance of 
the change so I can make that snapshot.

Comment 5

[Wan-Teh Chang]

If this is too much work, we can live with the current
review and superreview for NSPR, NSS, and JSS.  Like
Nelson said, this is just a cosmetic problem.  I didn't
realize it's so much work.

Comment 6

[Wan-Teh Chang]

timeless:

Now I think it's okay to blow away our old review requests
at the end of next week.

If we do that, what will happen to the review+ and superreview+
markings on our patches?

Will they be gone, will they stay the same, or will they
become first-review+ and second-review+?

(Similarly for the review- and superreview- markings on
our patches.)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

IMO, We definitely do NOT want to lose r+ and r- on old patches!  
If this change would do that, then I think we should not do it.

Comment 8

[Wan-Teh Chang]

Nelson, I agree we don't want to lose r+ and r- on old
patches.  That's exactly why I asked what will happen to
them.  I should also express my opinion about the three
possible outcomes:

review+ and superreview+ be gone: not acceptable
review+ and superreview+ stay the same: preferred
review+ and superreview+ become first-review+ and second-review+: acceptable

Similarly for review- and superreview-.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

The other option is to retire the "superreview" flag and use the "review" flag
twice to indicate first and second review on patches.  This approach requires no
reconfiguration of flags (although some optional reconfiguration--like hiding
the superreview flag for new patches--would improve usability), and you can
begin using it immediately.

The Bugzilla product, which doesn't require two reviews for all patches, uses
this mechanism.  When a patch requires two reviews, the author first requests
review, then he requests "addl. review", which shows up in the UI after an
initial "review" flag has been requested or set.  Both review flags then get
displayed by the attachment, so that if I and Dave Miller both review a patch,
it shows myk: "review+, justdave: review+".

This system has worked well for us, and I know of no disadvantages to it,
although perhaps there are limitations to how one searches for bugs with two
reviews.  Since its implementation requires no reconfiguration, you might try it
with a few patches to see how it works for you, then revisit this request if it
proves inadequate.

Comment 10

[timeless]

reading myk's comment again, he means retiring super-review from the products
mentioned in the summary. given that the main review flag is now multiplicable,
that should be sufficient. (minus problems with trying to migrate any currently
existing sr flags on nspr/nss/jss.)

Comment 11

[Robert Relyea]

The only problem with that suggesting is that we often want to request parrallel
reviews. That way we aren't hung up on the schedule of one reviewer.

bob

Comment 12

[Dave Miller [:justdave]]

(In reply to comment #11)
> The only problem with that suggesting is that we often want to request parrallel
> reviews. That way we aren't hung up on the schedule of one reviewer.

There's nothing stopping you from doing that.  The second review box shows up as
soon as the first one is requested, so just go back and fill it in.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Dave, does this mean that a second visit to bmo is required to set the 
additional review flag after the request to set the first review flag
has been set?  
Would I be able to set both a review request and an additional review
request in the same form submission where I attach a patch to a bug?

Is there a bug that uses this feature with which I can play to test this?

Comment 14

[Wan-Teh Chang]

I'm fine with the new suggestion if we can keep the superreview+
and superreview- markings on old attachments.

Comment 15

[Wan-Teh Chang]

I withdrew this request.  This is just a nice-to-have
and it turns out to be more work than I thought.

We've got used to using superreview as second review,
so we'll continue to do that.  Besides, we can request
additional review (but only after the first review has
been requested, which is a minor inconvenience).