Move all our review request field logic into new field classes.

 






 

 


  Review Request #5200
  —
  Created Jan. 6, 2014 and submitted 

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Move all our review request field logic into new field classes.


This introduces a new set of classes and functions for representing

fields on a review request. These classes are responsible for rendering

the list of fields on the review request page, loading values from the

database, saving from a draft, recording change description info, and

rendering that change description info.


Fields are grouped by Fieldsets. There are three default fieldsets:

"main" (the area for Submitter, Description, and Testing Done), "info"

(Repository, Bugs, etc.), and "reviewers". Additional fieldsets can be

registered, and fields placed in them. These will render on the side,

below Reviewers.


This work allows us to simplify a lot of our template code for the

review request, as well as the change description rendering code. It

also allows us to provide a level of extensibility for fields. Soon,

extensions will be able to easily provide their own fields on a review

request.


There's also a small but measurable impact on load times. By reducing

the work the template has to do, one review request I have with about 10

change descriptions saves about 10ms of render time.

 








  


 
 

  
Verified that everything displayed exactly as it did before. This includes

all fields, with and without values, and all combinations of change description

items.


Saved every type of field and published drafts. The new values persisted and

their changedescription entries were recorded and shown.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
This should escape the format parameters.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
These should be escaped.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
This should be escaped.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
This should be escaped.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Instead of calling |safe here, I think it would make more sense to use mark_safe inside the render methods. That …

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 








 














 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/reviews/builtin_fields.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This should escape the format parameters.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/reviews/fields.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    These should be escaped.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/reviews/fields.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This should be escaped.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/reviews/fields.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This should be escaped.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/templates/reviews/review_detail.html
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Instead of calling |safe here, I think it would make more sense to use mark_safe inside the render methods. That way extensions have to make a decisions about whether or not their text is safe.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I went back and forth on this initially.
      

      The whole point of the function is to render HTML that depicts the change. Unless anyone overrides the function, it will always be safe HTML. I think if someone's going to override the function to return something different, it's likely to be more complex than what we provide by default, meaning it's going to be HTML anyway. I can't see someone overriding and just making it an even plainer string.
      

      I think in this case, it's okay to optimize for treating this as safe, and just say that part of the contract is that they escape their values.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      The existing docstrings for the function also say they're rendering to HTML. I'll rename the function to include _html and talk about escaping.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Sounds good.
      
 
      

      4. 


 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  







  

   

    Status:
   

   


    Completed