Include site root in e-mail URLs once

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

Information

Review Board
release-3.0.x
1b1ecab...

Reviewers

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.

Description From Last Updated

Typo in the description on the 3rd line: "exapmle"

chipx86chipx86

Bug 4612

daviddavid

Unit tests are needed for the context variables for these paths, so we can be sure we don't regress again. …

chipx86chipx86

Can we add a unit test for this?

daviddavid

It took me a sec to realize what we were doing here. We're just ensuring we have a URL with …

chipx86chipx86

Add another blank line here.

daviddavid

Swap these.

chipx86chipx86

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

chipx86chipx86

This must be in the format of:: ... _Bug 4612: https://....

chipx86chipx86
david
  1. 
      
  2. Bug 4612

  3. Can we add a unit test for this?

  4. 
      
chipx86
  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. 
      
brennie
brennie
david
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 2)
     
     

    Add another blank line here.

  3. 
      
brennie
chipx86
  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. 
      
brennie
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 4)
     
     

    This must be in the format of::

    ... _Bug 4612: https://....
    
  3. 
      
brennie
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

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