Front-end for general comments

Review Request #6506 — Created Oct. 25, 2014 and submitted

Information

Review Board
master
33ec3a2...

Reviewers

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.

The general comment backend model is built. (review request 6375)
The general comment web-api also done. (review request 6431)
This review request is for implemention of general comments at front-end.

  • Now the general comment can be displayed as a review box on the page
  • Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good.
  • A email is sent when a general comment is created and replied.

How to create a general comment:
1. On a review request page, click ‘Add Comment’ to the right of ‘Review’ button.
2. A green box will pop up. Write your general comment and tick the checkboxes if needed.
3. Click ‘OK’ to save the comment, then the green box will disappear and there will be a review draft banner on the top of the page.
4. If you wish to have multiple general comments in the same review, repeat step 1-3.
5. Once finished, click 'Publish' on the review draft banner to publish the review. (or you can discard or edit the review just like publishing other comments)

All js-tests passed.

Tests including:
1. reviewReplyEditor with general comments
2. General comment model
3. reviewReply with general comments
4. reviewDialogView with general comments


Description From Last Updated

Indentation issue? Tabs?

chipx86chipx86

No need for this, since we're not extending anything. Just assign directly without _.defaults().

chipx86chipx86

Missing trailing comma.

chipx86chipx86

Reviewables are just there to provide a way to review files. We won't need to provide this for general comments, …

chipx86chipx86

By passing an ID, you're telling the comment dialog that this comment already exists on the server, which isn't true. …

chipx86chipx86

It should probably return null, instead of false. When comparing in JavaScript, you want to use === instead of ==. …

chipx86chipx86

Col: 13 E265 block comment should start with '# '

reviewbotreviewbot

Col: 13 E265 block comment should start with '# '

reviewbotreviewbot

It displays a comment dialog, not a review dialog.

chipx86chipx86

This is the default, so no need to specify it.

chipx86chipx86

This needs documentation above it.

chipx86chipx86

This will happen automatically. No need to specify it.

chipx86chipx86

This is the default. No need to specify it.

chipx86chipx86

These are also all defaults. No need to override.

chipx86chipx86

The trailing comma must be removed. In JavaScript, this is technically a syntax error, which will break IE and some …

chipx86chipx86

This isn't the right place for this. Instead, the code that sets up the dialog for creating new general comments …

chipx86chipx86

Doesn't look like this got fixed? Also, missing space before the {.

chipx86chipx86

We should probably load these before other comments, so that they'll appear at the top of the review.

chipx86chipx86

Should be aligned like the other ones.

chipx86chipx86

For all the HTML you're introducing, make sure you're using 1-space indentation, and that it's aligned with respect to the …

chipx86chipx86

Missing a var statement.

chipx86chipx86

I'd rather not introduce a new event for this (which we do below). Instead, let's do what the other call …

chipx86chipx86

This was marked as fixed in two of my prior reviews, but isn't fixed. This function doesn't return a boolean, …

chipx86chipx86

Blank line between these. This should also be generalComment, or probably comment.

chipx86chipx86

We only use one var statement. So above, do: var comment = ..., dlg;

chipx86chipx86

This isn't needed anymore.

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    
    
  2. 
      
NI
chipx86
  1. We just made some changes to some files that may result in some merge difficulties.

    When you get a chance, I'd like to see a screenshot of an e-mail with general comments.

  2. Show all issues

    Indentation issue? Tabs?

  3. Show all issues

    No need for this, since we're not extending anything. Just assign directly without _.defaults().

  4. Show all issues

    Missing trailing comma.

  5. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/models/generalCommentBlockModel.js
        reviewboard/static/rb/js/models/generalReviewableModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/models/generalCommentBlockModel.js
        reviewboard/static/rb/js/models/generalReviewableModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
    
    
  2. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/models/generalCommentBlockModel.js
        reviewboard/static/rb/js/models/generalReviewableModel.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/models/generalCommentBlockModel.js
        reviewboard/static/rb/js/models/generalReviewableModel.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    By passing an ID, you're telling the comment dialog that this comment already exists on the server, which isn't true. You shouldn't pass an ID.

    Also note how the data you're passing doesn't match the function signature for createGeneralComment.

  3. 
      
chipx86
  1. 
      
  2. Show all issues

    Reviewables are just there to provide a way to review files. We won't need to provide this for general comments, since "General" isn't a type of reviewable thing. General comments are independent of any files.

    Likewise, you won't need GeneralCommentBlock.

  3. reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 3)
     
     
     
     
     
    Show all issues

    It should probably return null, instead of false.

    When comparing in JavaScript, you want to use === instead of ==. Seems weird, but that's how it works. === is a string comparison, whereas == attempts to cast things.

    Also, blank line between this conditional and the next return statement.

  4. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/generalReviewableView.js
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/models/generalCommentBlockModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/js/models/generalReviewableModel.js
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/generalReviewableView.js
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/static/rb/js/models/generalCommentBlockModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/static/rb/js/models/generalReviewableModel.js
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    
    
  2. reviewboard/staticbundles.py (Diff revision 4)
     
     
    Show all issues
    Col: 13
     E265 block comment should start with '# '
    
  3. reviewboard/staticbundles.py (Diff revision 4)
     
     
    Show all issues
    Col: 13
     E265 block comment should start with '# '
    
  4. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
NI
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
chipx86
  1. Can you also show screenshots for the review dialog and comment dialog?

  2. 
      
chipx86
  1. One more piece of feedback, unrelated to code. Each review request's summary, description, and testing done should be fully self-descriptive, allowing a reviewer or someone down the road to read through it and know exactly what you're doing, why, and how, without having to dive into the code. We'll also be using these for the commit message. This also massively helps when we later are looking for the source of regressions, or are drafting release notes.

    You can browse through any of my commits in the repository for examples of how these should look.

  2. 
      
chipx86
  1. 
      
  2. Show all issues

    It displays a comment dialog, not a review dialog.

  3. Show all issues

    This is the default, so no need to specify it.

  4. Show all issues

    This needs documentation above it.

  5. Show all issues

    This will happen automatically. No need to specify it.

  6. Show all issues

    This is the default. No need to specify it.

  7. reviewboard/static/rb/js/resources/models/generalCommentModel.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These are also all defaults. No need to override.

  8. Show all issues

    The trailing comma must be removed. In JavaScript, this is technically a syntax error, which will break IE and some other browsers.

  9. Show all issues

    This isn't the right place for this. Instead, the code that sets up the dialog for creating new general comments on the review request page should call this when the comment has been saved.

  10. Show all issues

    Doesn't look like this got fixed?

    Also, missing space before the {.

  11. Show all issues

    We should probably load these before other comments, so that they'll appear at the top of the review.

  12. Show all issues

    Should be aligned like the other ones.

  13. reviewboard/templates/reviews/boxes/review.html (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    For all the HTML you're introducing, make sure you're using 1-space indentation, and that it's aligned with respect to the other HTML.

    Also make sure that the nesting looks correct. For instance, here, it looks like the <div> is nested inside the <a>, but it's not. The </div> also isn't aligning with the <div>.

    This will all be a bit easier to see when you've made sure they're aligned with other HTML and using the proper indentation size.

  14. 
      
NI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    Missing a var statement.

  3. Show all issues

    I'd rather not introduce a new event for this (which we do below). Instead, let's do what the other call sites do, which is create the comment and listen to that comment's 'saved' event instead.

  4. Show all issues

    This was marked as fixed in two of my prior reviews, but isn't fixed. This function doesn't return a boolean, but rather must return a string or null.

  5. 
      
NI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
ML
  1. Unless I missed it, it might be useful to upload a view (or just describe in words in the description) of how to actually create a GeneralComment. For example, how did you get the green display box to submit the issue/general comment?

  2. 
      
chipx86
  1. Last 3 changes! Then it's going in :)

  2. Show all issues

    Blank line between these.

    This should also be generalComment, or probably comment.

  3. Show all issues

    We only use one var statement. So above, do:

    var comment = ...,
        dlg;
    
  4. Show all issues

    This isn't needed anymore.

  5. 
      
NI
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/views/reviewDialogView.js
        reviewboard/templates/notifications/reply_email.html
        reviewboard/static/rb/js/resources/models/reviewReplyModel.js
        reviewboard/templates/notifications/reply_email.txt
        reviewboard/static/rb/js/resources/models/generalCommentModel.js
        reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
        reviewboard/templates/notifications/review_email.html
        reviewboard/templates/reviews/review_request_actions_primary.html
        reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
        reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/models/reviewReplyEditorModel.js
        reviewboard/templates/notifications/review_email.txt
        reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
NI
Review request changed
Status:
Completed
Change Summary:
Pushed to ucosp/nicole_xin/general-comments (60a48e6)