1085698 - Fix and test review interaction with Bugzilla

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Gregory Szorc [:gps]]

We have some gaps in test coverage for review board. I'll be writing some tests to round out the test coverage.

Comment 1

[Gregory Szorc [:gps]]

Attachment: bz://1085698/gps

Comment 2

[Gregory Szorc [:gps]]

/r/238 - reviewboard: add a test for publishing against an invalid bug

Pull down this commit:

hg pull review -r 77aa12223b49a42d0bc30282f45449631f14c4ce

Comment 3

[Gregory Szorc [:gps]]

Changing purpose of bug a little bit because as part of writing tests, I found a bug causing review flags to not get set in Bugzilla.

Comment 4

[Gregory Szorc [:gps]]

/r/239 - bugzilla: add flags to dump-bugs output
/r/238 - reviewboard: add a test for publishing against an invalid bug
/r/240 - reviewboard: add a create-review command
/r/241 - reviewboard: add a test that verifies Bugzilla review interaction
/r/242 - reviewboard: add an add-reviewer mach command
/r/243 - reviewboard: properly set review flag requestee (bug 1085698)

Pull down these commits:

hg pull review -r 0cf28a587b574a1121bd20cfcd69813f01a88590

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8508312 [details]
bz://1085698/gps

The good news is this patch series fixes the bug where RB doesn't add the f? flag to Bugzilla :)

Comment 6

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/239/#review200

Ship It!

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/238/#review201

Ship It!

Comment 8

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/240/#review199

Ship-It!

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 1)
> +    @CommandArgument('--public', action='store_true',
> +            help='Whether to make this review public')

allowing public=False is mostly useless since we can't add comments to the review after, and we can't publish it.

We'll probably end up implementing methods so that we can craft reviews with comments in the tests, so I guess that's fine for now.

Comment 9

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/241/#review202

Ship It!

Comment 10

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/242/#review203

Ship It!

Comment 11

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/243/#review198

Fix it, then Ship-It!

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +from rbbz.models import (
> +    BugzillaUserMap,
> +    get_or_create_bugzilla_users,
> +)

Can we stick with the style convention used above (it's what RB core uses)
``` python
from rbbz.models import (BugzillaUserMap,
                         get_or_create_bugzilla_users)
```

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +    for u in review_request_draft.target_people.all():
> +        if not using_bugzilla:
> +            reviewers.append(u.get_username())
> +
> +        bum = BugzillaUserMap.objects.get(user=u)
> +
> +        user_data = b.get_user_from_userid(bum.bugzilla_user_id)

I wonder how slow this is going to be for a large number of reviewers...

Comment 12

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/237/#review204

Fix the one commit, then Ship-It all.

Comment 13

[Gregory Szorc [:gps]]

https://reviewboard-dev.allizom.org/r/240/#review205

> allowing public=False is mostly useless since we can't add comments to the review after, and we can't publish it.
> 
> We'll probably end up implementing methods so that we can craft reviews with comments in the tests, so I guess that's fine for now.

Yeah, this confused me a little as I was developing the patch. I had to look and see what the HTML was doing on reviewboard-dev to grasp at how reviews are stored and incrementally updated.

We can always improve this later.

Comment 14

[Gregory Szorc [:gps]]

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