Fix a bug in the RB.UploadAttachmentView refactoring change

 






 

 


  Review Request #10937
  —
  Created March 4, 2020 and submitted 

 


















 

  

   Information

  





 
 

  hxqlin
 









 
 

  Review Board
 









 
 

  release-3.0.x
 









 
 

  
 









 
 

  
 









 
 

  b5ece1b...
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
The change e94bac071aa42fa45ad30d92f975d6d7c114e16a was landed prematurely and introduced

a some regressions. Some of them were fixed in 3a74bdfaee929403eb7ada0c38d286e6c27d0d20 but

a couple remained.


In RB.UploadAttachmentView, the options in initialize needs to be defaulted to an

empty object to prevent reference errors. In addition, the function was referencing 

this.presetCaption and this.attachmentHistory, which were undefined. This caused the 

dialog to have a blank caption when file attachments were updated.

 








  


 
 

  
    

  • dialogView unit tests still pass
    • 



Manual testing:


Adding a file attachment:

- opening the dialog, uploading a file, and submitting it (without editing the caption)

- opening the dialog, uploading a file, editing the caption, and submitting it

- opening the dialog, editing the caption, uploading a file, and submitting it

- opening the dialog, uploading a file, then cancelling the dialog

- opening the dialog, editing the caption, then cancelling the dialog

- opening the dialog, then cancelling the dialog without uploading anything

- verified the 'Upload' button is disabled unless a file is actually uploaded


Updating a file attachment:

- editing the caption for a file attachment inline and then opening the update dialog

to check that the caption is consistent

- opening the update dialog and cancelling the dialog without uploading anything new

- updating a file attachment without changing the caption 

- updating a file attachment and changing the caption

- verified the 'Upload' button is disabled unless a new file is actually uploaded

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Looking at this further, we actually need to break this into two steps: Build the version of options we want …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Since we're already extending this with options, which now contains attachmentHistoryID and presetCaption, we won't need to put them in …

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/js/views/uploadAttachmentView.es6.js
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Looking at this further, we actually need to break this into two steps:
    

      

    1. Build the version of options we want to pass in to both initialize and this.template up-front
      2. 

    2. Build body.
      3. 

    3. Call RB.DialogView.prototype.initialize().
      4. 

    

    The reason is that this is trying to do two things at once, which it cannot do: Set defaults for options, and pass the options (provided or defaults) to the template. The template depends on the defaults set right above, and it's not getting a chance to use them.
    

    So if you split this into three statements, that should solve it.
    

    (Wish we had unit tests for this view, because we'd have caught this earlier.)
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      (Er, "break this into three steps.")
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




hxqlin




   

   

  

 



 















 





 

  

   




chipx86




   

   









  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Since we're already extending this with options, which now contains attachmentHistoryID and presetCaption, we won't need to put them in the initial payload. We can just include body exclusively.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




hxqlin




   

   

  

 



 















 



 

  

   




hxqlin




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-3.0.x (29ae747)





 


 










  

 









     

    

   

  

  

   
   Loading...