4612: Email notifications generate incorrect URLs when SITE_ROOT is not '/'

jhominal
brennie
brennie

What version are you running?

3.0

What's the URL of the page containing the problem?

The problem manifests in all generated emails

What steps will reproduce the problem?

  1. Configure a Reviewboard site, with a non-empty SITE_ROOT configuration setting
  2. Create a review
  3. Open the email notification for the created review

What is the expected output? What do you see instead?

The link for the review should be the link to the full site (e.g. https://<domain>:<port>/reviews/r/<review_request_id>/ with SITE_ROOT = '/reviews/') but will contain SITE_ROOT twice instead (e.g. https://<domain>:<port>/reviews//reviews/r/<review_request_id>/).

What operating system are you using? What browser?

Not significant

Please provide any additional information below.

After inspecting the code, I have seen that version 2.5.x of Reviewboard was writing this in its email notification templates:

<a href="{{domain_method}}://{{domain}}{{review_request.get_absolute_url}}">{{domain_method}}://{{domain}}{{review_request.get_absolute_url}}</a>

While the 3.0 is written like that:

<a href="{{site_url}}{{review_request.get_absolute_url}}">{{site_url}}{{review_request.get_absolute_url}}</a>

site_url is the result of a call to admin.server.get_server_url, which is used for two distinct notions in the reviewboard code:

  • The root Reviewboard URL (https://<domain>:<port>/reviews/ in my case)
  • The server URL (https://<domain>:<port> in my case)

Currently, get_server_url's implementation returns the root RB URL, but in the case of the email notification, we actually want the server URL without a path. (However, that bug was shipped because there is no difference in output in the default configuration.)

I would propose:

  • Modifying get_server_url so that it always returns the server URL
  • Changing the calls to get_server_url that actually want the root site URL to use a new function (get_site_root_url in reviewboard/admin/server.py) instead
david
#1 david
  • -New
    +PendingReview
  • +brennie
david
#2 david

This was fixed in 3.0.2.

  • -PendingReview
    +Fixed