Expand on X-ReviewBoard-ShipIt by adding X-ReviewBoard-Conversation and X-ReviewClosed for commonly filtered e-mails.

Review Request #7803 — Created Dec. 10, 2015 and discarded

Information

Review Board

Reviewers

On our own team, we have found X-ReviewBoard-ShipIt very useful for picking out e-mails that contain actionable content (i.e. [Ship It]). In our group, the next most actionable content is "someone has commented on code / someone has responded to comments"

Now that we have X-ReviewBoard-ShipIt and X-ReviewBoard-ShipIt-Only to help filter [Ship It] messages, I figure it makes sense to add something like X-ReviewBoard-Conversation for those who want to filter specifically for messages containing review responses (or other conversation).

If there were something like X-ReviewBoard-Conversation or X-ReviewBoard-Comments to mark when a conversation is taking place (as opposed to just an updated diff/description, etc.) that would cover all of our most common e-mail filter requests.

Added a unit test for X-ReviewBoard-Conversation

If anyone has tips on the best way to test review open/close or replies that would be greatly appreciated. I'd love to unit test those too (although this code is running live-patched on our team's server full-time and works fine for us)

Description From Last Updated

The way that X-ReviewBoard-ShipIt-Only works is that the review request must meet the following conditions: There are no diff / …

brenniebrennie

With my above comments, I feel like there should be 2 cases: 1 is conversation and one is not-conversation. This …

brenniebrennie

Col: 80 E501 line too long (119 > 79 characters)

reviewbotreviewbot

We wrap long test docstrings as: """Test test test test test test test test test """

brenniebrennie

Missing a test for review replies.

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
        reviewboard/notifications/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
        reviewboard/notifications/tests.py
    
    
  2. reviewboard/notifications/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (119 > 79 characters)
    
  3. 
      
YU
brennie
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 1)
     
     

    This will be a single character, so you may want to transform this into a longer string.

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

    The way that X-ReviewBoard-ShipIt-Only works is that the review request must meet the following conditions:

    1. There are no diff / attachment comments.
    2. The text of the review is either "Ship It!" or empty.
    3. The review has a ship it.

    I feel like it would be inappropriate to have X-ReviewBoard-Conversation for all review emails, because a conversation email should never be a shipit-only email.

    Thoughts?

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

    With my above comments, I feel like there should be 2 cases: 1 is conversation and one is not-conversation. This is open to debate.

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

    We wrap long test docstrings as:

    """Test test test test
    test test test
    test test
    """
    
  6. reviewboard/notifications/tests.py (Diff revision 1)
     
     
    Show all issues

    Missing a test for review replies.

  7. 
      
david
Review request changed

Status: Discarded

Loading...