Link comments in review e-mails to the review page instead of the diff

Review Request #214 — Created Jan. 26, 2008 and submitted

Information

Review Board SVN (deprecated)
trunk
322

Reviewers

This is something that's been irritating a lot of people, including me.  When
one gets a review e-mail from Review Board, it has the comments with links in
it.  These links have traditionally pointed to the relevant lines in the diff
viewer.  This is almost never what people want -- they want to look at the
review in the context of the source, and respond.

It makes a lot more sense to link to the relevant spot on the review page, from
which people can get to the diff if they want.  This change does exactly that,
adding anchors to the review so we can link directly.
Checked link addresses in the e-mail preview page.
chipx86
  1. 
      
  2. /trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    This change is going to break the links in the review detail page. Right now you can click on the filename for the part of a diff on a comment and jump to the right place in the diff. It uses Comment.get_absolute_url to do this, though. We have the same thing with ScreenshotComment.get_absolute_url.
    
    This might be a good opportunity to start migrating to the new URL stuff in Django. Through a slightly different syntax for the entries we care about in urls.py, we can give a name to a URL mapping. We can then look up that url using the parameters passed and the name of the mapping. This would let us use two different mappings for these comments, one for the review request page and one for the diff or screenshot page.
    1. The new URL stuff looks cute, but not really appropriate for building URLs with anchors (at least, not unless we want 6 copies of the diff URL).  I've created a get_review_url function instead.
    2. Ugh, good point.
      
      Okay, the get_review_url function works well enough. Change looks good. Ship it!
  3. Some browsers still choke on this. This should be <a name="..."></a>
  4. 
      
Loading...