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...