Disable commenting when there's a pending draft of a review request.

 






 

 


  Review Request #4143
  —
  Created May 12, 2013 and submitted 

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  release-1.7.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Disable commenting when there's a pending draft of a review request.

A common mistake is to put up a new diff or file attachment and then try
to comment on it before publishing the draft. This doesn't work, and
will fail on an API level, but we didn't tell the user at any point that
they should expect problems doing that.

Now, the dialog will tell the user they must publish before they can
comment on the diff or file. All the fields will be disabled.

Note that bug #831 requests that we *allow* commenting. That's a much
bigger problem to solve, and one we won't be solving as part of this
change. This just fixes the unexpected failure case.

 








  


 
 

  
Uploaded a file and tried commenting on it, and on the diff for that
review request. It wouldn't let me. Saw the error and saw that the fields
were all disabled.

Published the review request and tried again. This time I could leave a
comment.

 







 









 





  








 

  
  


   

    

     

     

     

    

   


   

  

 










 






  

 








 














 





 

  

   




reviewbot




   

   





  

 



 

  


   

   

    
 
reviewbot


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/templates/reviews/comments_dlg.html


    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




david




   

   





  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    I'm not sure I entirely understand the workflow from the user's point of view. Can you clarify? Screenshots might help.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Basically, the comment dialog is pre-rendered. We already handle the "you're not logged in" bit when we render that.

When we render any page with a comment dialog, and there's a draft of the review request (which is passed to all reviewable pages as a 'draft' context parameter), we'll add the part saying that there's a draft that must be published.

Since we always trigger a page reload when clicking Publish, this is always a valid assumption today (unless you publish in another page and then forget to reload, but that's not a big problem since they can just reload the page with the comment dialog).

I can post a screenshot.
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   





  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Hmm. So I'm not thrilled with the UX, but I suppose it's better than what we have.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'm not either, but it fixes the bug. What would you propose?
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      I think it's pretty confusing to pop up something that looks like the comment dialog but doesn't work. Nobody is going to read the text and find out why. I'd rather it look completely different from the comment dialog, limiting what's shown to only the instructions for what to do.

I'm also a little bit wary about not being able to add comments to things which are already published (in the event that the draft is an update), but I suppose that a much rarer case.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      I think that'll all be a lot easier to do properly with the new JavaScript codebase. Anything we do for 1.7 is going to be messier and will have to be rewritten from scratch.
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      It looks like it wouldn't be too hard to change it to hide the form fields in the error case, and maybe even change which buttons are shown.
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      Okay, that I can do.
      
 
      

      6. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




reviewbot




   

   





  

 



 

  


   

   

    
 
reviewbot


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/reviews.js
    reviewboard/templates/reviews/comments_dlg.html


    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...