Add a signal for when the issue status of a comment gets updated.

 






 

 


  Review Request #14297
  —
  Created Jan. 17, 2025 and updated 

 




















 

  

   Information

  





 
 

  maubin
 









 
 

  Review Board
 









 
 

  release-7.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This change adds a signal for when the issues status of a comment gets

updated. This signal is needed for the WIP Bitbucket Server Pull Request

integration (and future pull request integrations), which will listen to

the signal so that it can update the review request approval status

displayed on the pull request whenever an issue is dropped/fixed/re-opened.

It's also just generally useful to have.


While here, I also fixed up two bugs that I noticed with our comments:


We override the save method for the BaseComment class, but failed to pass

any **kwargs to django.db.models.Model.save(), so full model updates

were happening every time, even if update_fields was passed. I added a

unit test that had I used to check this.


We use an attribute called _loaded_issue_status to track the previous

issue status on a comment, and compare it to the new issue status on updates.

Previously, we only set this attribute on comment initialization. So if one

instance of a comment went from open, to dropped, to re-opened, the issue

state wouldn't actually update from dropped to re-opened. Our issues status

updating logic would compare the re-opened state to the initial open state

that was set during initialization, even though the real previous issue

state at the time was dropped. This was never an issue in production

because when toggling issue statuses from the UI, a new comment object

instance would be created upon each toggle. This is now fixed by

setting the _loaded_issue_status whenever the issue status changes.


I also fixed some typos in some signal doc comments.

 








  


 
 

  
    

  • Ran unit tests.
    • 

  • Tested creating comments with and without issues, then toggling between open/fixed/dropped/re-opened states.
    • 


 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

   

  
  

   


    

     

      Add a signal for when the issue status of a comment gets updated.
      
This change adds a signal for when the issues status of a comment gets
updated. This signal is needed for the WIP Bitbucket Server Pull Request
integration (and future pull request integrations), which will listen to
the signal so that it can update the review request approval status
displayed on the pull request whenever an issue is dropped/fixed/re-opened.
It's also just generally useful to have.

While here, I also fixed up two bugs that I noticed with our comments:

We override the save method for the `BaseComment` class, but failed to pass
any `**kwargs` to `django.db.models.Model.save()`, so full model updates
were happening every time, even if `update_fields` was passed. I added a
unit test that had I used to check this.

We use an attribute called `_loaded_issue_status` to track the previous
issue status on a comment, and compare it to the new issue status on updates.
Previously, we only set this attribute on comment initialization. So if one
instance of a comment went from open, to dropped, to re-opened, the issue
state wouldn't actually update from dropped to re-opened. Our issues status
updating logic would compare the re-opened state to the initial open state
that was set during initialization, even though the real previous issue
state at the time was dropped. This was never an issue in production
because when toggling issue statuses from the UI, a new comment object
instance would be created upon each toggle. This is now fixed by
setting the `_loaded_issue_status` whenever the issue status changes.

I also fixed some typos in some signal doc comments.

     


    		
    2582feb8cad6ef8f041f64f9e87501c4a25aa144

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
'django.contrib.auth.models.User' imported but unused Column: 5 Error code: F401

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
We can simplify this with: kwargs['update_fields'] = \ set(kwargs['update_fields'} | {'timestamp'}

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Even better syntax for this can be: if update_fields := kwargs.get('update_fields'): kwargs['update_fields'] = { *update_fields, 'timestamp' }

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
I think we can remove the BaseComment part in the description, since it's in the type.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
For this, we need to pass owner=models.Model to ensure the type can always be found. There should be a kgb …

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 failed.
 





 

 JSHint
 passed.
 













 
flake8

 


 










  

 





 














 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david



    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86



    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/reviews/models/base_comment.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  
  
   Show all issues
  
 
    

    



    
    We can simplify this with:
    

    

    kwargs['update_fields'] = \
    set(kwargs['update_fields'} | {'timestamp'}

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/reviews/signals.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  
  
   Show all issues
  
 
    

    



    
    I think we can remove the BaseComment part in the description, since it's in the type.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/reviews/tests/test_base_comment.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  
  
   Show all issues
  
 
    

    



    
    For this, we need to pass owner=models.Model to ensure the type can always be found. There should be a kgb warning if not passed and it's an unbound method.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Oh, I missed the warning because it only gets displayed in the "Captured log calls" section when the unit test fails. If the unit test passes, you don't see any captured logs. Maybe we wanna update the kgb code to use warnings.warn instead of logger.warning so that the warning will always appear in the "Warnings summary" section?
      
 
      

      2. 


 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   




maubin




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  















  

   

    Commits:
   

   

    





 

  
   


    

    Summary
    ID

   

  
  

   


    

    

     

      Add a signal for when the issue status of a comment gets updated.
      
This change adds a signal for when the issues status of a comment gets
updated. This signal is needed for the WIP Bitbucket Server Pull Request
integration (and future pull request integrations), which will listen to
the signal so that it can update the review request approval status
displayed on the pull request whenever an issue is dropped/fixed/re-opened.
It's also just generally useful to have.

While here, I also fixed up two bugs that I noticed with our comments:

We override the save method for the `BaseComment` class, but failed to pass
any `**kwargs` to `django.db.models.Model.save()`, so full model updates
were happening every time, even if `update_fields` was passed. I added a
unit test that had I used to check this.

We use an attribute called `_loaded_issue_status` to track the previous
issue status on a comment, and compare it to the new issue status on updates.
Previously, we only set this attribute on comment initialization. So if one
instance of a comment went from open, to dropped, to re-opened, the issue
state wouldn't actually update from dropped to re-opened. Our issues status
updating logic would compare the re-opened state to the initial open state
that was set during initialization, even though the real previous issue
state at the time was dropped. This was never an issue in production
because when toggling issue statuses from the UI, a new comment object
instance would be created upon each toggle. This is now fixed by
setting the `_loaded_issue_status` whenever the issue status changes.

I also fixed some typos in some signal doc comments.

     


    		
    47fe51e933b5287531dadad5cd429823fdbee04e

   




   


    

    

     

      Add a signal for when the issue status of a comment gets updated.
      
This change adds a signal for when the issues status of a comment gets
updated. This signal is needed for the WIP Bitbucket Server Pull Request
integration (and future pull request integrations), which will listen to
the signal so that it can update the review request approval status
displayed on the pull request whenever an issue is dropped/fixed/re-opened.
It's also just generally useful to have.

While here, I also fixed up two bugs that I noticed with our comments:

We override the save method for the `BaseComment` class, but failed to pass
any `**kwargs` to `django.db.models.Model.save()`, so full model updates
were happening every time, even if `update_fields` was passed. I added a
unit test that had I used to check this.

We use an attribute called `_loaded_issue_status` to track the previous
issue status on a comment, and compare it to the new issue status on updates.
Previously, we only set this attribute on comment initialization. So if one
instance of a comment went from open, to dropped, to re-opened, the issue
state wouldn't actually update from dropped to re-opened. Our issues status
updating logic would compare the re-opened state to the initial open state
that was set during initialization, even though the real previous issue
state at the time was dropped. This was never an issue in production
because when toggling issue statuses from the UI, a new comment object
instance would be created upon each toggle. This is now fixed by
setting the `_loaded_issue_status` whenever the issue status changes.

I also fixed some typos in some signal doc comments.

     


    		
    2582feb8cad6ef8f041f64f9e87501c4a25aa144

   



  
 







   

  





  

   

    Diff:
   

   

    
 Revision 3 (+412 -14)
Show changes
 
 
   reviewboard/reviews/signals.py 
 
   reviewboard/reviews/models/base_comment.py 
 
   reviewboard/reviews/tests/test_base_comment.py 
 

   

  











 

  
Checks run (2 succeeded)



  

 flake8
 passed.
 





  

 JSHint
 passed.
 





 















  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david



    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/reviews/models/base_comment.py
       


        (Diff revisions 2 - 3)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  
  
   Show all issues
  
 
    

    



    
    Even better syntax for this can be:
    

    

    if update_fields := kwargs.get('update_fields'):
    kwargs['update_fields'] = { *update_fields, 'timestamp' }

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 









     

    

   

  

  

   
   Loading...