If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Review reply links posted to Bugzilla have non-existent anchors

RESOLVED FIXED

Status

MozReview
General
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: smacleod, Assigned: botond, Mentored)

Tracking

Development/Staging

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.

Updated

2 years ago
Assignee: nobody → mcote
(Assignee)

Comment 1

2 years ago
Stealing with permission.
Assignee: mcote → botond
(Assignee)

Comment 2

2 years ago
Created 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.
(Assignee)

Comment 3

2 years ago
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.
Attachment #8623823 - Flags: review?(smacleod)
(Reporter)

Updated

2 years ago
Attachment #8623823 - Flags: review?(smacleod)
(Reporter)

Comment 4

2 years ago
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.
(Reporter)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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.
Attachment #8623823 - Flags: review?(smacleod)
(Assignee)

Comment 7

2 years ago
Thanks for the suggestion, this is indeed a better design.
(Reporter)

Comment 8

2 years ago
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.
Attachment #8623823 - Flags: review?(smacleod)
(Assignee)

Comment 9

2 years ago
Ah, good call - there was indeed one test that needed updating!
(Assignee)

Comment 10

2 years ago
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.
Attachment #8623823 - Flags: review?(smacleod)
(Reporter)

Comment 11

2 years ago
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.
Attachment #8623823 - Flags: review?(smacleod) → review+
(Assignee)

Comment 12

2 years ago
Thanks! Landed in https://hg.mozilla.org/hgcustom/version-control-tools/rev/cdf3e4937489.
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.