Include site root in e-mail URLs once
Review Request #9448 — Created Dec. 18, 2017 and submitted
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 athttp://example.com/rb
, the URL for
a review request would be generated ashttp://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 ofget_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" |
chipx86 | |
Bug 4612 |
david | |
Unit tests are needed for the context variables for these paths, so we can be sure we don't regress again. … |
chipx86 | |
Can we add a unit test for this? |
david | |
It took me a sec to realize what we were doing here. We're just ensuring we have a URL with … |
chipx86 | |
Add another blank line here. |
david | |
Swap these. |
chipx86 | |
Given the class name, I think this needs a docstring explaining bug 4612. Ideally, the class name would be more … |
chipx86 | |
This must be in the format of:: ... _Bug 4612: https://.... |
chipx86 |
-
-
-
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.
-
reviewboard/notifications/email/message.py (Diff revision 1) 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?
Change Summary:
Added unit tests.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Bugs: |
|
||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+368 -33) |
Checks run (2 succeeded)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+369 -33) |
Checks run (2 succeeded)
-
-
-
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.
Change Summary:
Addressed Christian's comments
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+381 -33) |
Checks run (2 succeeded)
-
-
reviewboard/notifications/tests.py (Diff revision 4) This must be in the format of::
... _Bug 4612: https://....
Change Summary:
Addressed Christian's comments
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+381 -33) |