Move more template context to the action renderers.

 






 

 


  Review Request #14671
  —
  Created Nov. 5, 2025 and updated 

 




















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  release-7.1.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
In preparation for the final changes in separating actions from their

views, this change pulls out some of the view-specific state from the

actions and sets them in the renderers instead. This includes the

has_parent flag and dom_element_id (which previously required a call

to get_dom_element_id()).


BaseAction.get_dom_element_id() and the JavaScript Action model's

domID attribute are now both deprecated.


There's no good way to shim domID, as it's an action model attribute

and is fundamentally incompatible with per-view dom IDs. We only use

this ourselves for menus, and it doesn't appear we ever actually do

anything with it anywhere, so we're just scrapping it.

 








  


 
 

  
Unit tests pass.


Tested that all actions displayed and worked correctly.

 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

   

  
  

   


    

     

      Move more template context to the action renderers.
      
In preparation for the final changes in separating actions from their
views, this change pulls out some of the view-specific state from the
actions and sets them in the renderers instead. This includes the
`has_parent` flag and `dom_element_id` (which previously required a call
to `get_dom_element_id()`).

`BaseAction.get_dom_element_id()` and the JavaScript `Action` model's
`domID` attribute are now both deprecated.

There's no good way to shim `domID`, as it's an action model attribute
and is fundamentally incompatible with per-view dom IDs. We only use
this ourselves for menus, and it doesn't appear we ever actually do
anything with it anywhere, so we're just scrapping it.

     


    		
    d96528a2c2f20b7aca85a1cb0594fbe4b8fe1657

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Your description says this is deprecated, but it's not marked as such, and the renderer is still calling it. Either …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Oh, also, should we have a version added block here explaining that this method was moved from BaseGroupAction?

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  









  

   

    Change Summary:
   

   

    
Removed the has_parent context variable from BaseAction.get_extra_context() in favor of that from the renderer.

   

  








  

   

    Commits:
   

   

    





 

  
   


    

    Summary
    ID

   

  
  

   


    

    

     

      Move more template context to the action renderers.
      
In preparation for the final changes in separating actions from their
views, this change pulls out some of the view-specific state from the
actions and sets them in the renderers instead. This includes the
`has_parent` flag and `dom_element_id` (which previously required a call
to `get_dom_element_id()`).

`BaseAction.get_dom_element_id()` and the JavaScript `Action` model's
`domID` attribute are now both deprecated.

There's no good way to shim `domID`, as it's an action model attribute
and is fundamentally incompatible with per-view dom IDs. We only use
this ourselves for menus, and it doesn't appear we ever actually do
anything with it anywhere, so we're just scrapping it.

     


    		
    a8d69a2dfdcee94676208d864983ed0c04b43938

   




   


    

    

     

      Move more template context to the action renderers.
      
In preparation for the final changes in separating actions from their
views, this change pulls out some of the view-specific state from the
actions and sets them in the renderers instead. This includes the
`has_parent` flag and `dom_element_id` (which previously required a call
to `get_dom_element_id()`).

`BaseAction.get_dom_element_id()` and the JavaScript `Action` model's
`domID` attribute are now both deprecated.

There's no good way to shim `domID`, as it's an action model attribute
and is fundamentally incompatible with per-view dom IDs. We only use
this ourselves for menus, and it doesn't appear we ever actually do
anything with it anywhere, so we're just scrapping it.

     


    		
    d96528a2c2f20b7aca85a1cb0594fbe4b8fe1657

   



  
 







   

  





  

   

    Diff:
   

   

    
 Revision 2 (+200 -204)
Show changes
 
    reviewboard/actions/base.py 
    reviewboard/actions/renderers.py 
    reviewboard/actions/tests/test_base_action.py 
    reviewboard/actions/tests/test_base_action_renderer.py 
    reviewboard/actions/tests/test_group_action.py 
    reviewboard/static/rb/js/common/actions/views/menuActionView.ts 
    reviewboard/templates/accounts/account_menu_action.html 
    reviewboard/templates/actions/action_base.html 
  4 more
 

   

  











 

  
Checks run (2 succeeded)



  

 flake8
 passed.
 





  

 JSHint
 passed.
 





 















  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david



    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/actions/base.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Your description says this is deprecated, but it's not marked as such, and the renderer is still calling it.
    

    Either we should probably move the actual functionality out of here and actually mark it as deprecated, or change the description.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      It will be in one of the follow-up changes. It's necessary for this change to stay functional.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/actions/renderers.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    Might be nicer to write this as:
    

    

    return {
    **action.get_extra_context(request=request,
                               context=context),
    'action': action,
    'action_renderer': self,
    'attachment_point_id': action.attachment,
    ...
}

    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This wastes a dictionary. We already have a new dictionary to populate. Better to update that than to create a new one and copy from the other new one into it.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Actually I guess either way we have a throw-away dictionary.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      These are cheap enough not to worry too much about this in practice, but I got curious and decided to test things:
      

      

      def test1():
    a = {
      'a': 1,
      'b': 2,
      'c': 3,
    }
    a.update({
        'd': 4,
        'e': 5,
        'f': 6,
    })

def test2():
  a = {
      'a': 1,
      'b': 2,
      'c': 3,
  }
  b = {
      **a,
      'd': 4,
      'e': 5,
      'f': 6,
  }

def test3():
  a = {
      'a': 1,
      'b': 2,
      'c': 3,
  }
  a['d'] = 4
  a['e'] = 5
  a['f'] = 6

      

      

      Results:
      

      


      
Python
dict.update()
{**dict}:
dict[...]

      
      


      
3.8
0.002631750000000002
0.002332332999999999
0.0015086249999999995

      

      
3.9
0.002569000000000002
0.0029753749999999954
0.001817375000000003

      

      
3.10
0.002573375008068979
0.0027398329984862357
0.0016685000155121088

      

      
3.11
0.0025507090031169355
0.00264904199866578
0.0016354580002371222

      

      
3.12
0.002828041004249826
0.0030305830005090684
0.001743832981446758

      

      
3.13
0.003182916989317164
0.003386458003660664
0.00205779101816006

      

      
3.14
0.003014665999216959
0.0031265830039046705
0.002212874998804182

      
      

      

      They all get slower over time, which is sad to see, but dict.update() is the fastest of the two, and of course setting keys individually avoids the iteration and extra dictionary so it's much faster.
      
 
      

      4. 


 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david



    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/actions/renderers.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Oh, also, should we have a version added block here explaining that this method was moved from BaseGroupAction?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      BaseGroupAction and action renderers are all new in 7.1, so I don't think we need to.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david



    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3.