1152903 - Review reply links posted to Bugzilla have non-existent anchors

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Steven MacLeod [:smacleod]]

When cross-posting links for replies to reviews (i.e. a comment(s) in response to someone's review of a review request) we are generating a link which has a hash(#) component to a non-existent anchor. For an example see https://bugzilla.mozilla.org/show_bug.cgi?id=1144653#c3 which was a link generated by the comment https://reviewboard.mozilla.org/r/5863/#comment_rc7969-5691

Since these replies can have multiple comments, we can't really link to just a specific comment like I did in the example above, we need to link to the parent review which the reply is targeted at.

We're generating these faulty links now because replies are represented as reviews themselves inside of Review Board, just with special fields set. Our code is taking the ID for this special reply review and generating a link, assuming it works just like reviews themselves.

We'll need to update this code to intelligently check that the review is a reply and then generate the link for the parent review. This should be a pretty straightforward fix.

Comment 1

[Botond Ballo [:botond]]

Stealing with permission.

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

Rather than trying to link to a specific comment, link to the parent review
which the reply is targeted at.

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8623823 [details]
MozReview Request: mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

Rather than trying to link to a specific comment, link to the parent review
which the reply is targeted at.

Comment 4

[Steven MacLeod [:smacleod]]

Comment on attachment 8623823 [details]
MozReview Request: mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

https://reviewboard.mozilla.org/r/11595/#review10105

This looks good for the most part, and should fix the problem, but I have some issues around the implementation due to how things were done previously. I'd appreciate if we could take the opportunity to clean this up as part of your patch.

::: pylib/rbbz/rbbz/extension.py:103
(Diff revision 1)
>  def review_or_request_url(review_or_request, site=None, siteconfig=None):

I'm not actually a fan of this function retrieving the url for both review requests,
and reviews to start with - We always know which we have so I'm not sure just
sticking all of the logic in this `review_or_request_or_..._url` function is the best
way to go forward.

I believe it was done this way because the url for a review request or a review is
generated with identical code - but that's something that's more abstract than just
those objects, many models in Review Board share this pattern.

What I propose we should do for this fix is transform `review_or_request_url` into
`def get_obj_url(obj, site=None, siteconfig=None)` which is the implementation before
your change, with the `review_or_request` arg renamed `obj`.

Update all the callsites(1) with the exception of the one which passes `reply` - for
that we create a new function:
```
def get_reply_url(reply, site=None, siteconfig=None):
    """<docstring about why we generate the base_reply_to url>"""
    return get_obj_url(reply.base_reply_to, site=site, siteconfig=siteconfig)
```

(1) https://dxr.mozilla.org/hgcustom:version-control-tools/search?q=review_or_request_url&case=true&redirect=true

::: pylib/rbbz/rbbz/extension.py:110
(Diff revision 1)
> +    # Bug 1152903: since replies can have multiple comments, we can't link to
> +    # a specific comment, we need to link to the parent review which the reply
> +    # is targeted at.

When we move this to the docstring I think it's fine to drop the reference to the bug
number (if someone really wants to track it down they can check the blame and find the
commit).

::: pylib/rbbz/rbbz/extension.py:465
(Diff revision 1)
>      url = review_or_request_url(reply)

This is the callsite for replies, where we'll want to call the new `get_reply_url(reply)`
method.

Comment 5

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/11595/#review10107

> This is the callsite for replies, where we'll want to call the new `get_reply_url(reply)`
> method.

Ah, didn't mean to make this an issue as part of my review, just wanted to call it out for you - /me drops.

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8623823 [details]
MozReview Request: mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

Rather than trying to link to a specific comment, link to the parent review
which the reply is targeted at.

Comment 7

[Botond Ballo [:botond]]

Thanks for the suggestion, this is indeed a better design.

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8623823 [details]
MozReview Request: mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

https://reviewboard.mozilla.org/r/11595/#review10223

This looks great. Can you give `./run-tests hgext/reviewboard/tests/` a run though, I'm almost certain this is going to change the output of a couple that look at bugzilla comments. If it does can you update those as well please.

Comment 9

[Botond Ballo [:botond]]

Ah, good call - there was indeed one test that needed updating!

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8623823 [details]
MozReview Request: mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

Rather than trying to link to a specific comment, link to the parent review
which the reply is targeted at.

Comment 11

[Steven MacLeod [:smacleod]]

Comment on attachment 8623823 [details]
MozReview Request: mozreview: fix review reply links posted to bugzilla (bug 1152903); r=smacleod

https://reviewboard.mozilla.org/r/11595/#review10423

Excellent, thanks for fixing this! Sorry for the delay on the review, weekend + whistler made me slower than I'd have liked.

Comment 12

[Botond Ballo [:botond]]

Thanks! Landed in https://hg.mozilla.org/hgcustom/version-control-tools/rev/cdf3e4937489.