Fix comment issue bugs.

Review Request #3998 — Created March 23, 2013 and submitted

Information

Review Board
master

Reviewers

Fix comment issue bugs.

Changing comment issue states was pretty broken. Some of this was just code that
was never updated when the comments were converted to Backbone. Once I updated
this, I ran into the trouble that existing DiffComment objects were failing to
validate because they didn't have a fileDiffId. This isn't something which is
returned as part of the API responses (for a reason that I haven't figured out
yet), but it's not required when we just want to change the issue state, so I've
changed the validator to only require it if the comment is new.
Fixed, dropped, and reopened some issues.
Description From Last Updated

I think this is CommentIssueManager not providing everything that the Comment classes now expect. This seems fine for now, but …

chipx86chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
      Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/models/diffCommentModel.js
    
    
  2. 
      
chipx86
  1. 
      
  2. I think this is CommentIssueManager not providing everything that the Comment classes now expect. This seems fine for now, but we should probably consider whether we need to change CommentIssueManager or not.
    
    The only reservation I have is that, in the typical case, fileDiffID *should* be there, even for new comments (they are when the diff viewer creates them), so I'm not sure ignoring validation is really correct, but it's okay for now.
    
    My only ask for this change is that we have a comment explaining the above here, maybe also a TODO in CommentIssueManager where it's calling createDiffComment and friends.
    1. CommentIssueManager doesn't know the fileDiffID either. Ideally I think it should be in the API response when we get the comment, but I'm not seeing it in there and I don't know why.
  3. 
      
david
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
      Ignored Files:
        reviewboard/static/rb/js/models/commentIssueManagerModel.js
        reviewboard/static/rb/js/models/diffCommentModel.js
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (5044448).
Loading...