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