1034188 - Uniquely identify review requests by Bug ID + user ID instead of just Bug ID

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Steven MacLeod [:smacleod]]

We should key squashed review requests on bug ID + user ID so that users can work on the same bug without colliding (e.g. one user doesn't finish and the work is picked up by another).

Currently the way we've modeled things it's not simple to support changing ownership of the review requests. Adding the user ID as part of the key would at least allow multiple contributions until we can come up with something better.

We need to decide on a format that combines bug ID and user ID and put it in the commit-id field instead of "bz://<bug-id>"

rbbz will also need to be updated so that it knows how to convert this new identifier into a bug ID on publish (Or we could just scrap a bit of generality and have the server extension populate the bug field. It could be added as an argument to post_reviews and passed in by the mozilla specific code).

Comment 1

[Mark Côté [:mcote]]

I think that having the hg server extension populate the bug field is a good idea; however, the shortest route here is just modifying the commit ID to include the username.  Having the hg extension do the work shouldn't be a breaking change if we decide to do it down the road.

I propose bz://<userid>/<bugid> since it should be easy to parse.

Comment 2

[Mark Côté [:mcote]]

Opinions as to what the user ID should actually be?  I was thinking the Bugzilla ID at first, but it's likely that in the future we'll just be using some sort of API key for communicating with Review Board/Bugzilla, and we may not have the username handy--although maybe we'd have something similar.

In that case I guess the commit author name should be used?  That's kind of weird, though (e.g. bz://Mark Cote <[e-mail]>/123456), although it could be base64ed.

Finally I guess the current user from the environment?

Comment 3

[Gregory Szorc [:gps]]

I don't understand the requirements for this.

I /think/ what you are requesting is to allow the creation of multiple squashed RB reviews against the same set of commits. In other words, there is a 1:many relationship between an individual commit and squashed commits as opposed to a 1:1 relationship. Yes, technically we can have a 1:many relationship today, but for the same logical commit series, that relationship is effectively 1:1.

Adding this divergence on the basis of the pusher/author seems odd to me. In the use case in the initial comment of a new person taking over a review, wouldn't it better to maintain continuity from an existing review rather than to fragment the "view" into a review?

I just don't understand why this has been proposed. It seems like a suboptimal solution.

Comment 4

[Steven MacLeod [:smacleod]]

(In reply to Gregory Szorc [:gps] from comment #3)
> I /think/ what you are requesting is to allow the creation of multiple
> squashed RB reviews against the same set of commits. In other words, there
> is a 1:many relationship between an individual commit and squashed commits
> as opposed to a 1:1 relationship. Yes, technically we can have a 1:many
> relationship today, but for the same logical commit series, that
> relationship is effectively 1:1.

No, that's not what I'm proposing here. That's kind of what we're doing in
Bug 1034175 though.


> Adding this divergence on the basis of the pusher/author seems odd to me. In

The reasoning behind this is to allow multiple people to post work for a bug.
The current implementation only allows a single user to have 1 set of commits per
bug[1]. If anyone else wanted to post alternate patches, take over the work
etc. it just can't happen.


> the use case in the initial comment of a new person taking over a review,
> wouldn't it better to maintain continuity from an existing review rather
> than to fragment the "view" into a review?

Yes, in this use case it *would* be better to take over the existing review
request and continue with it. Review Board's support for changing ownership of
review requests is not yet complete though.

It would be quite possible for us to add ownership change support through our
extension, especially in a limited and controlled way handled by the hook
or some of our extra UI. I thought for now though we should just allow
each user to have their own set of review requests per bug as it would be
much quicker to implement.


> I just don't understand why this has been proposed. It seems like a
> suboptimal solution.

It's very suboptimal. I was proposing something that could be accomplished quickly,
thinking we'd iterate.

[1] The proposed solution here allows many users to have 1 set of commits per bug
each. I still think this isn't a great solution but early on in the design of this
system there was pushback when I wanted to allow a developer to have more than 1
set of commits per bug. While I disagreed with restricting it to one commit set
per bug, it's definitely easier to implement, and there were more important things
to argue about.

Comment 5

[Mark Côté [:mcote]]

As discussed on IRC, we're going to use a new variable, ircnick, stored in hgrc (under [bugzilla], I guess, even though it's not technically related to Bugzilla per se), and prompt for it if necessary.  The commit ID will be bz://<bugid>/<ircnick>, but <bugid> and <bugid>/<ircnick> will also be acceptable via the --reviewid push option and transformed into the canonical ID.  This will also allow you to easily generate a new review request for the same bug.

Comment 6

[Mark Côté [:mcote]]

Although, I am conflicted.  Bugzilla username is easier and prevents the requirement of another variable.  I think I'll start with that; we can easily switch to ircnick later, since we aren't (yet) putting any meaning into the commit IDs beyond them being unique.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Review for review ID: bz://1034188

/r/82 - Bug 1034188 - Add IRC nick to review identifiers

Comment 8

[Mark Côté [:mcote]]

::: hgext/reviewboard/client.py
(Diff revision 1)
> +        raise util.Abort(_('--reviewid does not take identifiers with username'))

I thought one thing we talked about was that this new format allows a single user to create more than one review request for a bug--to explore two different solutions, for example.  If so, being able to override ircnick on the command line would probably be useful.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8460748 [details]
Review for review ID: bz://1034188

/r/82 - Bug 1034188 - Add IRC nick to review identifiers

Comment 10

[Mark Côté [:mcote]]

Ship It!

Comment 11

[Mark Côté [:mcote]]

I have a patch for rbbz for the rest of this bug, though there's a catch-22 since this bug is required to post multiple reviews for the same bug, and I can't deploy just your patch without rbbz, or publishing would break. :) In the interests of continued dogfooding, I'll just file a new, dependent bug.

Comment 12

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/8265717dfb98