Add publishing signals

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

Information

Review Board
master
0acef89...

Reviewers

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
  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. 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. 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. Aha, I didn't notice the WebAPIError.with_message() method. I'll pass custom PublishError messages with that.

  3. 
      
MC
david
  1. 
      
  2. We've switched over to the new-style except PublishError as e

  3. except PublishError as e

  4. except PublishError as e

  5. 
      
MC
david
  1. One last nit.

  2. Alignment isn't quite right here.

  3. 
      
MC
david
  1. Ship It!

  2. 
      
MC
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (ab84ccd)
Loading...