Improve the logic for collapsing any entry with a review.

 






 

 


  Review Request #9281
  —
  Created Oct. 18, 2017 and submitted 

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  release-3.0.x
 









 
 

  
 









 
 

  9280
 









 
 

  95f9cc8...
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
The logic for computing the collapse state for reviews (and anything

containing reviews by way of status updates) has been updated to better

show off the entries that users most likely want to see.


The new logic will expand review entries (or anything containing

reviews) if:


    

  1. The review is being seen by the user for the first time
    2. 

  2. There's open issues (including those in verifying states)
    3. 

  3. There's a new published reply
    4. 

  4. There's a pending draft replying to a body field or a comment
    5. 



Much of this is the same as before, but there are some notable changes:


    

  1. 

    Previously, only draft replies to comments were considered when
    
   determining the collapsing state. Draft replies to body top/bottom
    
   fields were not included.
    

    2. 

  2. 

    Review entries are no longer automatically expanded if newer than the
    
   latest change description, or automatically collapsed if older than
    
   it. The change description is no longer consider for review entries
    
   at all, providing more focus on the updates to the entries
    
   themselves.
    

    3. 

  3. 

    For the Initial Status Updates entry and Change entries containing
    
   status updates, only draft replies to comments were previously
    
   considered. Now, the collapsing logic for reviews apply to these
    
   as well when there are status updates with reviews.
    

    4. 



Thanks to Connor Yoshimoto for prototyping the new collapsing logic for

the page.

 








  


 
 

  
Unit tests pass.


Tested with a very comprehensive review request I have that has entries

of all kinds in all states needed for testing. Checked each entry and

saw that the collapsed states all made sense with regards to the new logic.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
F841 local variable 'reply' is assigned to but never used

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
F841 local variable 'reply' is assigned to but never used

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
F841 local variable 'reply' is assigned to but never used

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
F841 local variable 'reply' is assigned to but never used

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
F841 local variable 'reply' is assigned to but never used

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
F841 local variable 'reply' is assigned to but never used

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Now that we have better knowledge about the reviews within, we can do better here. Instead of always expanding if …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
This could be simplified to just say "there aren't any status updates with reviews that should be expanded". That covers …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
F841 local variable 'reply' is assigned to but never used

      		
      
       reviewbotreviewbot
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 failed.
 





 

 JSHint
 passed.
 













 
flake8

 


 










  

 





 














 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
Addressed Review Bot complaints.





 



  

   
Commit:

   
    
-52cd3343eafa2666b875f776f6cc808eabaf313f
+7ec0ef9134167b704a344a1c7838daafca599022

   		
  




  

   
Diff:

   
    
 Revision 2 (+247 -75)
Show changes
 
    reviewboard/reviews/detail.py 
    reviewboard/reviews/tests/test_entries.py 
 

   		
  



 









 

  
Checks run (1 failed, 1 succeeded)



  

 flake8
 failed.
 





  

 JSHint
 passed.
 





 







 
flake8

 


 















  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/detail.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Now that we have better knowledge about the reviews within, we can do better here.
    

    Instead of always expanding if there aren't any change descriptions, I think it would be better to check the last read timestamp against the timestamps of the initial status updates. That way it behaves the same as the other entries--if the user hasn't seen it, or if any of the reviews within have something new or important, it's expanded.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Sounds reasonable. I'm going to make that a separate change, since this one is focusing on logic related to reviews.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/detail.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This could be simplified to just say "there aren't any status updates with reviews that should be expanded". That covers the case where there aren't any status update at all.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-3.0.x (7ef507e)





 


 










  

 









     

    

   

  

  

   
   Loading...