Comments can be marked as "Blocking" for submission

Review Request #1285 — Created Nov. 30, 2009 and discarded

Information
trimbo
Review Board
Reviewers
reviewboard
jparise 
Serious problems with reviewed code sometimes need a way to indicate that parts must be fixed before a checkin, this code change attempts to create a solution for that by allowing a reviewer to mark an inline comment as "submission blocking".

A the review requester can still select "submitted" if comments have been marked as submission blocking but never fixed.  We did not want to prevent that because it creates a potential workflow issue.

This has bugs right now but I wanted to get feedback as soon as possible.

Bugs I'm trying to verify/fix:

  * Existing comments, when clicked on, do not indicate they were marked as blocking in the pop up blue dialog box.
  * "Ship it!" is still available to a reviewer who has marked a comment as blocking.  It should not be available to be chosen if that reviewer marked something as submission blocking.


Locally against alpha 1.1 tip.
chipx86
chipx86 
  1. I want us to think more about the terminology. I want to be able to find some different wording. The problem with saying that we're blocking submission is that we actually don't. Maybe someday we could optionally enforce it, but we don't today, and I think it's a bit misleading. Maybe "Required before submitting" or something along those lines.
  2. This is how an inline comment is indicated as blocking.
    Think I'd prefer different text here, but I'm having trouble thinking of something. Checkboxes should use sentence casing though. The one here should be more like: "Blocks submission" or "Blocks code submission."
    Think I'd prefer different text here, but I'm having trouble thinking of something.

Checkboxes should use sentence casing though. The one here should be more like: "Blocks submission" or "Blocks code submission."
  3. I'd prefer "blocksSubmission" everywhere in the change instead of "blockit". It's more descriptive that way.
  4. Excess space after the =
  5. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) 
     
     
     
     
     
    Now that we have two places that influence the button (this and the textarea's keyup handler), we need to have a single function that manages the state of the function. It should take into account both the text and this checkbox, and both the keyup handler and blockCheck.click should call it.
  6. Can't we test this.blockIt directly? Shouldn't need to parse it I think. We should make sure we never are able to set anything but 0 or 1.

Also, space after "if"
  7. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) 
     
     
     
     
    Indentation level is off. Should only be indented 4 spaces.

Make sure you're using spaces and not tabs everywhere.

Also, class should be set using attr(), and each thing in the chain should be on its own line.
  8. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) 
     
     
     
     
     
     
     
    Whole thing can be shortened to:

$("#id_blockit")[0].checked = (comment.blockIt == 1);
  9. reviewboard/reviews/models.py (Diff revision 1) 
     
     
     
    Will need an evolution file for this. See the existing ones in the evolution/ directories.

The text in the first parameter should be human-readable. Same as on the comment box.
  10. reviewboard/templates/reviews/comments_dlg.html (Diff revision 1) 
     
     
     
     
     
    The <input /> tag should be on its own line, and there should be a space before />

The indentation of these lines should be one space only.
  11. Indented too far.

What is "gr" ?
  12. Should be aligned with the <pre> above.

Make sure you're using spaces and not tabs.
  13. I imagine instead of JavaScript, you could just test against the template variable using template tags and set the attribute right on the Ship It checkbox during creation.
  14. Space after the ","
  15. reviewboard/webapi/json.py (Diff revision 1) 
     
     
     
    Should just be:

blockIt = (request.POST['blockit'] == "1")
  16. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    17. 




  

 









     

    

   

  

  

   
   Loading...