Enhancements to diff section of review request emails.

Review Request #8237 — Created June 11, 2016 and submitted

Information

Review Board
release-2.5.x
4eaa5f1...

Reviewers

This adds three new/modified features to the diff section of review request emails:

  1. The diff section is now omitted for review requests which do not actually contain diffs. This has the effect of also preventing the View Diff link, which for the no diffs case was previously a link to a page that did not exist.
  2. Update View Diff link to point to the specific version of the diff that is relevant at the time that the email is produced, rather than always referencing the most recent version of the diff. The benefit here is that the link will take you to the diff version that is pertinent relative to the email, even if subsequent diffs have been posted.
  3. Add Show Changes link to provide a shortcut to the relevant interdiff. This mimics the links available in a "Review request changed" element in the main review request UI when an updated diff is posted. Correspondingly, the link is included in emails for all requests where an updated diff has been posted, and the diff revision is greater than one.

Manually tested all new features and examined resulting emails.

  • For item 1, tested review requests both with and without diffs.
  • For item 2, tested review request with multiple diff revisions and verified that View Diff link corresponded to the appropriate specific revision number.
  • For item 3, tested that Show Changes link did not appear for diff revision 1, but did appear for subsequent revisions and had the correct interdiff range.

Description From Last Updated

Can you post screenshots of this? You can use the preview_review_request_email view at http://localhost:8080/r/<id>/preview-email/html/ and http://localhost:8080/r/<id>/preview-email/text/.

brenniebrennie

Same here.

brenniebrennie

The with statement should be above the for loop above so that we're not binding the same variable for each …

brenniebrennie
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/notifications/review_request_email.html
        reviewboard/templates/notifications/review_request_email.txt
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/notifications/review_request_email.html
        reviewboard/templates/notifications/review_request_email.txt
    
    
  2. 
      
gmyers
  1. Posting this to gauge interest in these changes. While I think all three are worthwhile, I have listed them in what I believe is the order of importance. I haven't done a lot with the Django template language, so I'd welcome feedback on that front. Also, I don't know that I've used the most optimal approach for determining if a review request contains a diff.

    There is also one open issue in my mind that I would appreciate guidance on. The View Diff link is unconditionally included in emails. I've followed that approach for Show Changes as well (assuming we are at diff revision >1 where interdiffs actually come into play). But, I think it might make more sense to only include Show Changes when rev > 1 and changes.diff -- that is when the diff has in fact changed. Thoughts?

    1. Are you thinking about comparing source code (for the rev > 1 and changes.diff), or just if there's a new revision? I don't think we want to hide it if there's a new revision with no actual changes, as that's important for reviewers to know so they can say "Hey! You didn't change anything!".

    2. No, I wasn't thinking about comparing code, just the fact that there was a new revision posted. Haven't tried it yet, but I presume changes.diff is True even if a new diff revision has no actual changes.

    3. The change description for an update should indicate if a new diff was uploaded as part of the draft, so we can key off of that.

  2. 
      
brennie
  1. 
      
  2. Can you post screenshots of this?

    You can use the preview_review_request_email view at http://localhost:8080/r/<id>/preview-email/html/ and http://localhost:8080/r/<id>/preview-email/text/.

    1. E-mail preview view...where have you been all my life :)
  3. The with statement should be above the for loop above so that we're not binding the same variable for each filediff.

    1. But, for both of these cases the with-block is outside of the for-loop.  What am I missing?
    2. I misread this! Sorry!

  4. 
      
gmyers
chipx86
  1. Haven't looked at the code yet, but I love the changes. Mind doing this on 2.5?

  2. 
      
gmyers
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/notifications/review_request_email.html
        reviewboard/templates/notifications/review_request_email.txt
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/notifications/review_request_email.html
        reviewboard/templates/notifications/review_request_email.txt
    
    
  2. 
      
gmyers
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/notifications/review_request_email.html
        reviewboard/templates/notifications/review_request_email.txt
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/notifications/review_request_email.html
        reviewboard/templates/notifications/review_request_email.txt
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
gmyers
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (0b27f4f)
Loading...