Include site root in e-mail URLs once

Review Request #9448 - Created Dec. 18, 2017 and submitted

Barret Rennie
Review Board
release-3.0.x
4612
1b1ecab...
reviewboard

The refactoring of e-mails in Review Board 3.0 ended up causing a
regression where URLs in e-mails would have the site root applied twice.
For example, for a server hosted at http://example.com/rb, the URL for
a review request would be generated as http://example.com/rb/rb/r/1.
This was due to no longer computing the site URL (which was the scheme +
domain only), but instead using the result of get_server_url() which
includes the site root. This regression wasn't apparent due to a lack of
testing on configurations where the site root was anything other than
/.

Now, we pass in the correct value for site_url, computing it from
build_server_url for the / URL (which clobbers the site root) and
stripping the trailing / (so we don't get double / in URLs).

Previewed e-mails on a server where the site root was not / and saw
that the generated URLs were correct.

Ran unit tests.

  • 0
  • 0
  • 9
  • 0
  • 9
Description From Last Updated
David Trowbridge
  1. 
      
  2. Bug 4612

  3. Can we add a unit test for this?

  4. 
      
Christian Hammond
  1. 
      
  2. Typo in the description on the 3rd line: "exapmle"

  3. Unit tests are needed for the context variables for these paths, so we can be sure we don't regress again. They should test with/without site roots and with/without local sites.

  4. It took me a sec to realize what we were doing here. We're just ensuring we have a URL with a / at the end, and then stripping that away, right?

    Can you make this a utility function in this file so that we don't repeat the logic, and so that there's a docstring that describes exactly what we're doing?

  5. 
      
Barret Rennie
Barret Rennie
David Trowbridge
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 2)
     
     

    Add another blank line here.

  3. 
      
Barret Rennie
Christian Hammond
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 3)
     
     
     

    Swap these.

  3. reviewboard/notifications/tests.py (Diff revision 3)
     
     

    Given the class name, I think this needs a docstring explaining bug 4612. Ideally, the class name would be more self-describing.

    Same in other places where this is mentioned.

  4. 
      
Barret Rennie
David Trowbridge
  1. Ship It!
  2. 
      
Christian Hammond
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 4)
     
     

    This must be in the format of::

    ... _Bug 4612: https://....
    
  3. 
      
Barret Rennie
Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (cbc43f9)
Loading...