• 
  
    
 
    
 
    

    



    
    
    
    
     
    




    
 
    

  
    



    
 

 

  

  

 

    


  
    





    
 
    



  

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

    



 
    
 
    

  Review Request #5200
  —
  Created Jan. 7, 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.
    
          		
      


david

  david
 

      
      
       
      
     
    



     
    
      
       
       
    These should be escaped.
    
          		
      


david

  david
 

      
      
       
      
     
    



     
    
      
       
       
    This should be escaped.
    
          		
      


david

  david
 

      
      
       
      
     
    



     
    
      
       
       
    This should be escaped.
    
          		
      


david

  david
 

      
      
       
      
     
    



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


david

  david
 

      
      
       
      
     
    

        
   
    
  
    
 
    

    




 
    

    



  
    
 
    

    


    


 













    
 



 
    
  
    
   
    



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