Add publishing signals

 






 

 


  Review Request #5723
  —
  Created April 21, 2014 and submitted 

 


















 

  

   Information

  





 
 

  mcote
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 









 
 

  0acef89...
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This adds "publishing" signals, executed before any data is saved.  The intention is that an extension can block publishing if, for example, there's a problem communicating with an outside system, by raising an exception.


I added a PublishError class and made web API publish functions turn them into WebAPIErrors.  This is the part I'm least certain about.  I want to allow extension to define the errors, but it doesn't make sense for the publishing signal handlers to be throwing WebAPIErrors directly.  The compromise I came up with was to just put the PublishError string into a generic WebAPIError with a special user code of 999.  Other ideas welcome; I just wanted something to start a conversation.


The ReviewRequestEditor model already had some basic error handling (a simple alert dialog).  I added similar error handling to DraftReviewBannerView and ReviewReplyDraftBannerView.  As far as I can tell, that covers all interfaces to updating (publishing) reviews, review requests, and review replies.

 








  


 
 

  
All tests pass except some VCS-specific ones, which were skipped.  I also did some manual verification of the full system by throwing PublishError exceptions in an extension and verifying that the message appeared in an alert box in the UI.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Can you define a new error type for this in reviewboard/webapi/errors.py ? You can probably use the same error code/definition …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
We've switched over to the new-style except PublishError as e

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
except PublishError as e

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
except PublishError as e

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Alignment isn't quite right here.

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 








 














 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Ah oops, I just realized I accidentally rebased this patch on master.  I intended this to be on release-2.0.x.  Should I upload a new patch, or will you backport on commit?
      
 
      

      2. 


 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can you define a new error type for this in reviewboard/webapi/errors.py ? You can probably use the same error code/definition for all of the publishing failures.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      So my use case is to provide slightly more detailed error messages to the user than just "publish error".  It might be "authentication expired" or "you don't have permission to write to the associated bug" or even "Bugzilla appears to be down".  In general, since the publish hook is meant for extensions, I think it makes sense to allow the extensions to define their own errors.  I could provide a generic PublishError subclass of WebAPIError to use the same error code (I don't particularly care about the error code value) which accepts and arbitrary message (and maybe http code).
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Errors can have custom messages and payloads, so a hook could conceivably provide additional JSON to provide as part of that. I think that's probably the way to go, since we're talking custom code on both the provider and consumer ends at this point.
      

      I would agree that the root error for this should be a PUBLISH_ERROR, but optionally with the custom payloads.
      

      We always pre-define errors in errors.py, so we shouldn't manually create a WebAPIError anywhere outside that file.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Aha, I didn't notice the WebAPIError.with_message() method.  I'll pass custom PublishError messages with that.
      
 
      

      4. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   





 MC





   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    We've switched over to the new-style except PublishError as e
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    except PublishError as e
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    except PublishError as e
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   





 MC





   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    One last nit.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Alignment isn't quite right here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   





 MC





   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 MC





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-2.0.x (ab84ccd)





 


 










  

 









     

    

   

  

  

   
   Loading...