Fixed the 'X-ReviewBoard-Diff-For' mail header to include modified files.

Review Request #9206 — Created Sept. 22, 2017 and submitted

Information

Review Board
release-2.0.x
127a5b0...

Reviewers

The email generated after posting a review had an incomplete
'X-ReviewBoard-Diff-For' header. The header appeared for newly created files
and moved files but didn't appear for files that were only modified. The code
that was responsible for adding the header in question was ignoring modified
files. A check for modified files was added to the respective if statements and
a 'modified' attribute was added to the FileDiff class.

Posted reviews for a Tester Repository containing an edited file 'README.md'
and a created file 'FileA' to recreate the bug.

Header generated from a posted review with both a created file and an edited
file:

X-ReviewBoard-Diff-For: FileA

X-ReviewBoard-Diff-For only appears for the created file, 'FileA'.

After the fix, the same test was performed again with a created file 'FileA'
and an edited file 'README.md'.

Header generated from a posted review with an edited file and a created file:

X-ReviewBoard-Diff-For: FileA
X-ReviewBoard-Diff-For: README.md

X-ReviewBoard-Diff-For appears for both 'FileA' and 'README.md'.

Ran unit tests.

Description From Last Updated

The subject could also be improved by using the form: Fixed the X-ReviewBoard-Diff-For mail header to include modified files. This …

chipx86chipx86

Perhaps too much info in Testing Done. It's hard to see the meat of the testing with all the headers. …

chipx86chipx86

Descriptions and Testing Done should wrap at around 70-75 characters. The first line of the description can go away. The …

chipx86chipx86

This change will need unit tests. There's at least one existing test for this header. You should add a test …

chipx86chipx86

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

While the others don't have it, we're trying to introduce docstrings for every new function. These are always in the …

chipx86chipx86

We're now testing every single state except one in these cases. So instead, I'd suggest: if not filediff.is_new: and if …

chipx86chipx86

W293 blank line contains whitespace

reviewbotreviewbot

These blank lines should be removed.

chipx86chipx86

This isn't quite what we want. The function shouldn't be stating what it's doing internally (that's an implementation detail). Note …

chipx86chipx86

These blank lines should go away.

chipx86chipx86

No blank line.

chipx86chipx86

You can use: self.assertIn('X-ReviewBoard-Diff-For', message.rb_headers)

chipx86chipx86

I'd group the variable with the latter set of assertions, instead of the former.

chipx86chipx86

These should use self.assertIn.

chipx86chipx86

There should only be a single space between words.

brenniebrennie

Can you add a comment explaining what this is testing and include the bug number?

daviddavid

This should be a comment rather than part of the docstring (since the docstring is used for the test name).

daviddavid

Sorry, one more thing. Let's move this into the body of the method, just below the docstring.

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

hmarceau
hmarceau
mike_conley
chipx86
  1. 
      
  2. Show all issues

    Perhaps too much info in Testing Done. It's hard to see the meat of the testing with all the headers. Keep it straightforward and to the point.

    I'd also encourage using Markdown constructs more. You can use backticks for things like header names and class names, and triple backticks for blocks of literals like code samples or the blocks of e-mail headers.

  3. Show all issues

    Descriptions and Testing Done should wrap at around 70-75 characters.

    The first line of the description can go away. The bug number should go in the Bugs field, and the Review Board version number (btw, "Review Board" and not "ReviewBoard") should just be a detail on the bug report (as it really applies to both older and newer versions than 2.0.30).

  4. Show all issues

    This change will need unit tests. There's at least one existing test for this header. You should add a test ensuring that all expected states are handled correctly.

  5. reviewboard/diffviewer/models.py (Diff revision 2)
     
     
    Show all issues

    While the others don't have it, we're trying to introduce docstrings for every new function. These are always in the form of:

    """One-line summary."""
    

    or:

    """One-line summary.
    
    Multi-line description.
    """
    

    Function docstrings have some additional requirements (which are documented in Notion) but for properties, this is all you need. Search elsewhere in the codebase for @property and look for docstrings for examples.

  6. reviewboard/notifications/email.py (Diff revision 2)
     
     
     
    Show all issues

    We're now testing every single state except one in these cases. So instead, I'd suggest:

    if not filediff.is_new:
    

    and

    if not filediff.deleted:
    
  7. 
      
chipx86
  1. 
      
  2. Show all issues

    The subject could also be improved by using the form:

    Fixed the X-ReviewBoard-Diff-For mail header to include modified files.
    

    This spells out first that this is fixed, and what it is fixed for. A bit easier to read, more consistent with other changes.

  3. 
      
hmarceau
Review request changed
Summary:
Email header, 'X-ReviewBoard-Diff-For', now appears for Modified Files.
Fixed the 'X-ReviewBoard-Diff-For' mail header to include modified files.
Description:
~  

This patch is a response to bug report 4572, on ReviewBoard release 2.0.30.

~  
~  

The email generated after posting a review had an incomplete 'X-ReviewBoard-Diff-For' header. The header appeared for newly created files and moved files but didn't appear for files that were only modified. The code that was responsible for adding the header in question was ignoring modified files. A check for modified files was added to the respective if statements and a 'modified' attribute was added to the FileDiff class.

  ~

The email generated after posting a review had an incomplete

  ~ 'X-ReviewBoard-Diff-For' header. The header appeared for newly created files
  ~ and moved files but didn't appear for files that were only modified. The code
  + that was responsible for adding the header in question was ignoring modified
  + files. A check for modified files was added to the respective if statements and
  + a 'modified' attribute was added to the FileDiff class.

Testing Done:
~  

Posted reviews for a Tester Repository containing an edited file 'README.md'

  ~

Posted reviews for a Tester Repository containing an edited file 'README.md'

    and a created file 'FileA' to recreate the bug.

   
~  

Header generated from a posted review with both a created file and an edited file:

  ~

Header generated from a posted review with both a created file and an edited

  + file:

   
~  

boundary="===============1791673369560208835=="

~   MIME-Version: 1.0
  ~
X-ReviewBoard-Diff-For: FileA
  ~
-   Subject: Review Request 2: attempting to recreate bug 4572,
-   through a tester repository
-   From: hmarceau@laurentian.ca
-   To: hmarceau@laurentian.ca
-   Date: Fri, 22 Sep 2017 19:23:45 -0000
-   Message-ID: 20170922192345.10918.802@hmarceau-VAIO
-   X-ReviewBoard-URL: http://example.com/
-   Auto-Submitted: auto-generated
-   Sender: hp.marceau@gmail.com
-   X-ReviewGroup:
-   X-Auto-Response-Suppress: DR, RN, OOF, AutoReply
-   X-ReviewRequest-URL: http://example.com/r/2/
-   X-Sender: hp.marceau@gmail.com
-   X-ReviewBoard-Diff-For: FileA
-   Reply-To: hmarceau@laurentian.ca
-   X-ReviewRequest-Repository: Tester Repository

   
   

   
~  

'X-ReviewBoard-Diff-For' only appears for the created file, 'FileA'.

  ~

X-ReviewBoard-Diff-For only appears for the created file, 'FileA'.

   
   

   
   

   
   

After the fix, the same test was performed again with a created file 'FileA'

    and an edited file 'README.md'.

   
   

Header generated from a posted review with an edited file and a created file:

   
~  

boundary="===============1047217417420848096=="

~   MIME-Version: 1.0
~   Subject: Review Request 3: attempting to recreate bug 4572,
  ~
X-ReviewBoard-Diff-For: FileA
  ~
X-ReviewBoard-Diff-For: README.md
  ~
-   through a tester repository
-   From: hmarceau@laurentian.ca
-   To: hmarceau@laurentian.ca
-   Date: Fri, 22 Sep 2017 20:21:20 -0000
-   Message-ID: 20170922202120.16185.20047@hmarceau-VAIO
-   X-ReviewBoard-URL: http://example.com/
-   Auto-Submitted: auto-generated
-   Sender: hp.marceau@gmail.com
-   X-ReviewGroup:
-   X-Auto-Response-Suppress: DR, RN, OOF, AutoReply
-   X-ReviewRequest-URL: http://example.com/r/3/
-   X-Sender: hp.marceau@gmail.com
-   X-ReviewBoard-Diff-For: FileA
-   X-ReviewBoard-Diff-For: README.md
-   Reply-To: hmarceau@laurentian.ca
-   X-ReviewRequest-Repository: Tester Repository

   
~  

--===============1047217417420848096==

  ~

X-ReviewBoard-Diff-For appears for both 'FileA' and 'README.md'.

-   MIME-Version: 1.0
-   Content-Type: text/plain; charset="utf-8"
-   Content-Transfer-Encoding: 7bit

   
~  

  ~

Ran unit tests.

-  
-  

'X-ReviewBoard-Diff-For' appears for both 'FileA' and 'README.md'.

Bugs:
Commit:
d4f06682716a71d32d903449511b6d4b8a5485fe
9c0e77fbba0f94767e5958c77ba5c4c76034596c

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hmarceau
chipx86
  1. 
      
  2. reviewboard/diffviewer/models.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These blank lines should be removed.

  3. reviewboard/diffviewer/models.py (Diff revision 4)
     
     
     
     
     
     
    Show all issues

    This isn't quite what we want. The function shouldn't be stating what it's doing internally (that's an implementation detail). Note how it's just putting the logic of the code into words, which doesn't really help say what the purpose of this function is. It shouldn't even be documented as acting like a function, because as a property it's intended to be more like a value on a class.

    Instead, it should say something like: "Whether this file is a modification to an existing file of the same name."

  4. reviewboard/notifications/email.py (Diff revision 4)
     
     
     
     
     
     
     
     
    Show all issues

    These blank lines should go away.

  5. reviewboard/notifications/tests.py (Diff revision 4)
     
     
     
     
    Show all issues

    No blank line.

  6. reviewboard/notifications/tests.py (Diff revision 4)
     
     
    Show all issues

    You can use:

    self.assertIn('X-ReviewBoard-Diff-For', message.rb_headers)
    
  7. reviewboard/notifications/tests.py (Diff revision 4)
     
     
     
     
    Show all issues

    I'd group the variable with the latter set of assertions, instead of the former.

  8. reviewboard/notifications/tests.py (Diff revision 4)
     
     
     
    Show all issues

    These should use self.assertIn.

  9. 
      
hmarceau
brennie
  1. 
      
  2. reviewboard/diffviewer/models.py (Diff revision 5)
     
     
    Show all issues

    There should only be a single space between words.

  3. reviewboard/notifications/email.py (Diff revision 5)
     
     
     
     
     
     

    This is so much nicer to read :)

  4. 
      
hmarceau
hmarceau
  1. Ship It!

  2. 
      
david
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 6)
     
     
    Show all issues

    Can you add a comment explaining what this is testing and include the bug number?

    1. Do you want me to add a comment to the code, or to the review request?

    2. In the code, please.

  3. 
      
hmarceau
david
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 7)
     
     
     
     
    Show all issues

    This should be a comment rather than part of the docstring (since the docstring is used for the test name).

  3. 
      
hmarceau
david
  1. 
      
  2. reviewboard/notifications/tests.py (Diff revision 8)
     
     
     
     
    Show all issues

    Sorry, one more thing. Let's move this into the body of the method, just below the docstring.

  3. 
      
hmarceau
david
  1. Ship It!
  2. 
      
hmarceau
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (a25b589)