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.

 







 









 





  







 

  
Screenshots

  
 



 









 






  

 








 














 





 

  

   




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. 











  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. 












  3. 
 
    
  
  
    
  
 

   
    
    
    





    
    I'd prefer "blocksSubmission" everywhere in the change instead of "blockit". It's more descriptive that way.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    





    
    Excess space after the =
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  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. 






  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. 






  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. 






  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. 






  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. 






  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. 






  11. 
 
    
  
  
    
  
 

   
    
    
    





    
    Indented too far.

What is "gr" ?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    12. 






  12. 
 
    
  
  
    
  
 

   
    
    
    





    
    Should be aligned with the <pre> above.

Make sure you're using spaces and not tabs.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    13. 






  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. 






  14. 
 
    
  
  
    
  
 

   
    
    
    





    
    Space after the ","
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    15. 






  15. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/webapi/json.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    Should just be:

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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    16. 




  16. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    17. 




  

 









     

    

   

  

  

   
   Loading...