Build GeneralComment Model

Review Request #6375 — Created Sept. 26, 2014 and submitted

nicolexin
Review Board
master
cd5ccbf...
reviewboard, students

Reviewboard currently support 3 types of comments: Diff commment, File attachment comments, Screenshot comments.
This is a new type of comments: General comments.

A general comment on a review request is used when a comment is not tied to specific lines of code or a special file attachment, and an issue is opened. Examples include suggestions for testing or pointing out errors in the change description.

This review request is for GeneralComment backend model.

Five test added in reviews/tests.py:
1. test_review_detail_general_comment_ordering
2. test_init_with_general_comments
3. test_init_with_mix
4. test_init_general_comment_with_replies
5. test_save_reply_comment_to_general_comment

All test passed.

Previously test 1 failed due to use diff viewer 'get' method on general comments.
Figured out general comments behave the same as screenshot/file attachment comments.

  • 0
  • 0
  • 24
  • 22
  • 46
Description From Last Updated
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
  2.  'models' imported but unused
    
  3.  'six' imported but unused
    
  4.  '_' imported but unused
    
  5.  'BaseComment' imported but unused
    
  6. Col: 1
     E265 block comment should start with '# '
    
  7. Col: 11
     W292 no newline at end of file
    
  8. reviewboard/reviews/models/review.py (Diff revision 1)
     
     
    Col: 1
     E265 block comment should start with '# '
    
  9. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
  2. reviewboard/reviews/admin.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3.  'models' imported but unused
    
  4.  '_' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 2)
     
     
     list comprehension redefines 'file_attachment' from line 585
    
  6. reviewboard/testing/testcase.py (Diff revision 2)
     
     
     undefined name 'issue_opened'
    
  7. reviewboard/testing/testcase.py (Diff revision 2)
     
     
     undefined name 'issue_opened'
    
  8. 
      
  1. 
      
  2. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)
     
     
     
     
     
     
     

    Something you might want to consider is encapsulating this notion of a comment's context within each type of comment. For example you can then just to:

    context_id = comment.get_context_id() if bool(comment)

    or use the original condition comment != "" instead of converting it to a boolean value.

    It reduces to having this context logic outside the class it represents. This is just my opinion though, it would be interesting for one of the mentors to give their input but both ways are valid.

    1. Well I personally perfer the original one since this context-type is only used in the html templates, to communicate with front-end. So the context logic info is not quite useful in my backend class since we already know its type, we don't need to store extra data.

  3. 
      
  1. 
      
    1. Just one minor thing, otherwise it looks great so far!

  2. reviewboard/reviews/admin.py (Diff revision 2)
     
     

    Is this supposed to match raw_id_fields?
    If so, it's missing an 'e' in "general"

    1. Yeah, it was a stupid typo, gonna fix it right away.

  3. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
  2. reviewboard/reviews/admin.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3.  'models' imported but unused
    
  4.  '_' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 3)
     
     
     list comprehension redefines 'file_attachment' from line 585
    
  6. reviewboard/testing/testcase.py (Diff revision 3)
     
     
     undefined name 'issue_opened'
    
  7. reviewboard/testing/testcase.py (Diff revision 3)
     
     
     undefined name 'issue_opened'
    
  8. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 4)
     
     
     list comprehension redefines 'file_attachment' from line 585
    
  3. 
      
david
  1. You'll also need to update fetch_issue_counts in reviewboard/reviews/models/review_request.py

  2. reviewboard/webapi/resources/__init__.py (Diff revision 4)
     
     
     
     
     
     
     

    Shouldn't this be in the API change? (/r/6431)

  3. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/notifications/tests.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/notifications/tests.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
  2. reviewboard/reviews/admin.py (Diff revision 5)
     
     
    Col: 57
     W292 no newline at end of file
    
  3. Col: 29
     W292 no newline at end of file
    
  4. reviewboard/reviews/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/notifications/tests.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/notifications/tests.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/notifications/tests.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/notifications/tests.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/notifications/tests.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/notifications/tests.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    
  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
  2. 
      
brennie
  1. 
      
  2. Should this be "gcomment"?

  3. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
  2. 
      
chipx86
  1. Looking great! A few small comments.

  2. reviewboard/reviews/admin.py (Diff revision 11)
     
     
     

    Generally, we like to keep these in alphabetical order. FileAttachmentComment clearly isn't, but can you reorder these so they'll be in the right order?

  3. reviewboard/reviews/admin.py (Diff revision 11)
     
     

    Should be text. (We fixed the above cases recently.)

  4. reviewboard/reviews/admin.py (Diff revision 11)
     
     
     

    Same comment about order.

  5. This should be fleshed out more to describe the purpose of these comments.

    Also, you should always use complete sentences with trailing periods.

    For reference, multi-line docstrings are in the form of:

    """Single-line summary.
    
    Multi-line docstring.
    """
    
  6. reviewboard/reviews/models/general_comment.py (Diff revision 11)
     
     
     

    Should use single quotes.

  7. reviewboard/reviews/models/review.py (Diff revision 11)
     
     
     

    Single quotes.

  8. reviewboard/reviews/tests.py (Diff revision 11)
     
     
     

    Alphabetical order.

  9. reviewboard/reviews/views.py (Diff revision 11)
     
     
     

    Alphabetical order.

  10. reviewboard/testing/testcase.py (Diff revision 11)
     
     
     

    This is unrelated to your change. Can you revert it?

  11. 
      
  1. 
      
  2. reviewboard/reviews/tests.py (Diff revision 11)
     
     
     
     

    These should be single quotes as well.

    1. I seem to get the idea that string works like an identifier should be in single quotes, not sure if these should be changed. Maybe let's wait for a mentor to comment on this.

    2. That's fine but later on in the same file:
      def test_review_detail_file_attachment_visibility(self):
      caption_1 = 'File Attachment 1'
      caption_2 = 'File Attachment 2'
      ...
      uses single quotes so it should at least be consistent within the file.

  3. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
    
    
  2. 
      
  1. 
      
  2. reviewboard/reviews/admin.py (Diff revision 12)
     
     

    Why is there an apostrophe(,) at the end of this function call?

    1. Sorry, I mean comma, not apostrophe.

    2. This isn't a function call, it's a tuple with a length of 1

  3. reviewboard/reviews/admin.py (Diff revision 12)
     
     

    and here.

  4. reviewboard/reviews/tests.py (Diff revision 12)
     
     
     
     

    Why not use a list instead of 3 variables?

    1. Just to be consistent, and using a list is not gonna saving me some space anyway.

  5. reviewboard/reviews/tests.py (Diff revision 12)
     
     
     

    There should be a blank line between these lines.

    1. Can I ask why? coz previous code does not include a blank line here

  6. reviewboard/reviews/tests.py (Diff revision 12)
     
     
     
     

    Should use a loop instead of repeating a function call.

    for _ in range(3):
    <function here>

    1. Again, I'm being consistent with previous code, I think it's fine coz 3 is not too much?

  7. reviewboard/reviews/views.py (Diff revision 12)
     
     

    There should be a trailing apostrophe(,) after 'general_comments': []

    1. I meant, comma intsead of apostrophe here too.

    2. Yeah, previous code didn't have it, but it's good to add.

      The reason is that, if we have to add another item to this list in the future, it's a single insert, rather than a change to a line and an insert (which keeps patches smaller and reduces what they have to touch).

  8. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/managers.py
    
    
  2. 
      
chipx86
  1. Looks good! I only have a couple comments left.

  2. This should probably be more specific, like:

    "A comment on a review request not tied to code or a file."

  3. "Examples" and "testing".

  4. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/managers.py
    
    
  2. 
      
  1. 
      
  2. reviewboard/reviews/models/general_comment.py (Diff revisions 13 - 14)
     
     

    Minor grammar fix, it should read:
    "...review request that is not tied..."

  3. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/reviews/admin.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/__init__.py
        reviewboard/reviews/models/review_request.py
        reviewboard/reviews/models/review.py
        reviewboard/reviews/models/general_comment.py
        reviewboard/reviews/managers.py
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/nicole_xin/general-comments (117bd9f)
Loading...