1170155 - Make reviewboard "bugs" link in the right-hand column work before publishing a review request

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P2)

Description

[:Gijs (he/him)]

I would like to be able to check which bug I've pointed the review request to before publishing. Now I can't easily.

I guess an alternative would be linkifying bug numbers in patch summaries. I don't know if there's a separate bug on file for that? (I couldn't find one, but I imagine I'm just not looking in the right places?)

Comment 1

[Botond Ballo [:botond]]

Is it cool if I take this bug?

Comment 2

[Steven MacLeod [:smacleod]]

(In reply to Botond Ballo [:botond] from comment #1)
> Is it cool if I take this bug?

Definitely - I've assigned it to you.

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: reviewboardmods: populate the 'Bugs' field in review drafts (bug 1170155). r=smacleod

reviewboardmods: populate the 'Bugs' field in review drafts (bug 1170155). r=smacleod

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8650159 [details]
MozReview Request: reviewboardmods: populate the 'Bugs' field in review drafts (bug 1170155). r=smacleod

This is a first crack. I tested it in my local test environment and it seems to be working.

I didn't remove the code that sets the bugs_closed field upon publishing; should I, or might we still need it for some edge cases where the bug isn't known at draft creation time?

Comment 5

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/16557/#review14901

Looks good for the most part - I *do* think we should remove the code for setting the bug field when publishing as you mentioned in the bug.

::: hgext/reviewboard/hgrb/proto.py:279
(Diff revision 1)
> +            'bug': str(reviewid.bug) if reviewid.bug else '',

is it possible that `reviewid.bug` is falsey here? I would have assumed not since we require a reviewid with a valid bug.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:49
(Diff revision 1)
> +                    'bug': <bug-id>,

I feel like this should be an optional field - eventually we're going to allow commits without bugs (which we'll auto file a bug for, but maybe that happens before this step... hmmm).

If we are assuming this is required though, I'd rather not allow empty string entries through - I think we want to assume a valid bug number is required.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:436
(Diff revision 1)
> +    all_bugs = ','.join(set(commit['bug'] for commit in commits['individual']))

as it stands this will possibly allow the empty string into the set. We should be fine though if we're removing that part from above.

Comment 6

[Botond Ballo [:botond]]

(In reply to Steven MacLeod [:smacleod] from comment #5)
> ::: hgext/reviewboard/hgrb/proto.py:279
> (Diff revision 1)
> > +            'bug': str(reviewid.bug) if reviewid.bug else '',
> 
> is it possible that `reviewid.bug` is falsey here? I would have assumed not
> since we require a reviewid with a valid bug.

I assumed it was, given that we check for it above [1]. 

Looking at the ReviewID constructor, the only case where `bug` is left falsey but no exception is thrown is if the bug number is zero. Is that perhaps something we meant to disallow but overlooked?

> ::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:49
> (Diff revision 1)
> > +                    'bug': <bug-id>,
> 
> I feel like this should be an optional field - eventually we're going to
> allow commits without bugs (which we'll auto file a bug for, but maybe that
> happens before this step... hmmm).

If we end up allowing that, and the auto-filing doesn't happen yet by this time, I guess we can adjust this code then.

> If we are assuming this is required though, I'd rather not allow empty
> string entries through - I think we want to assume a valid bug number is
> required.

Do you mean that _post_reviews should check for this and throw an exception if the bug string is empty?

[1] https://dxr.mozilla.org/hgcustom_version-control-tools/rev/b21e237d6b5ec962898a999df305dd1bcd49bee3/hgext/reviewboard/hgrb/proto.py?offset=0#232

Comment 7

[Steven MacLeod [:smacleod]]

(In reply to Botond Ballo [:botond] from comment #6)

Sorry for the delay on this reply!

In the future, could you reply to review feedback in Review Board rather than
bugzilla (so it becomes part of the threaded conversation on the review I made).
There should be an "add comment" link by each issue/comment I made which you can
use to build a reply then publish.

> > is it possible that `reviewid.bug` is falsey here? I would have assumed not
> > since we require a reviewid with a valid bug.
> 
> I assumed it was, given that we check for it above [1].

Ah looking again we *aren't* enforcing the bug in reviewid on the server (I guess
that's only enforced on the client side right now). I guess this is fine for now
and we can update it if we properly enforce on the server.

 
> Looking at the ReviewID constructor, the only case where `bug` is left
> falsey but no exception is thrown is if the bug number is zero. Is that
> perhaps something we meant to disallow but overlooked?

Yes, this has been overlooked, nice catch. Could you maybe patch that up while
you're doing this stuff?


> > ::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:49
> > (Diff revision 1)
> > > +                    'bug': <bug-id>,
> > 
> > I feel like this should be an optional field - eventually we're going to
> > allow commits without bugs (which we'll auto file a bug for, but maybe that
> > happens before this step... hmmm).
> 
> If we end up allowing that, and the auto-filing doesn't happen yet by this
> time, I guess we can adjust this code then.

Ya that's fine.

 
> > If we are assuming this is required though, I'd rather not allow empty
> > string entries through - I think we want to assume a valid bug number is
> > required.
> 
> Do you mean that _post_reviews should check for this and throw an exception
> if the bug string is empty?

Just throwing an exception would be problematic, we'd want to send a nice
formated error message back down to the hg client. For now, instead of
setting the bug number to an empty string in this case, just don't include
the bugs field in the request to Review Board.

Comment 8

[Botond Ballo [:botond]]

Addressed review comments, posting updated patch for review. Sorry for the long turnaround time!

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8650159 [details]
MozReview Request: reviewboardmods: populate the 'Bugs' field in review drafts (bug 1170155). r=smacleod

reviewboardmods: populate the 'Bugs' field in review drafts (bug 1170155). r=smacleod

Comment 10

[Steven MacLeod [:smacleod]]

Comment on attachment 8650159 [details]
MozReview Request: reviewboardmods: populate the 'Bugs' field in review drafts (bug 1170155). r=smacleod

https://reviewboard.mozilla.org/r/16557/#review17031

Looks good, thanks again for taking care of this :)

::: pylib/rbbz/rbbz/extension.py:357
(Diff revision 2)
> -    # At this point, we know that the bug ID that we've got
> +    # Note that the bug ID has already been set when the review was created.
> -    # is valid and accessible.
> -    review_request_draft.bugs_closed = str(bug_id)

This will mean any pushed, but unpublished, review requests which exist when we deploy won't get their bug number set.

One way around this would be to heave this change be a separate commit which would be deployed after waiting a period of time from the other deploy.

I'm tempted to say lets just ignore the problem as it should be pretty rare if we deploy when things aren't active. I guess I'll leave this here as a record that it was thought about.

Comment 11

[Botond Ballo [:botond]]

Thanks! Landed:

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

Comment 12

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/ba432f038ee3ec8d62d07f1c346ab1a5b9f8c662
reviewboard: update test to reflect bugs saved on draft review request (bug 1170155)