Added new client side hook for validating draft reviews
Review Request #8052 — Created March 10, 2016 and discarded
Added new client side hook for validating draft reviews.
The idea behind this hook is that some extensions add client side widgets
which shouldn't be skipped accidentally when the user is reviewing code.
For example, with the checklist extension, they shouldn't forget to check off
all the items on their checklist.This code is generic in that it support any type of extension as well as multiple
extensions hooking into the publish validation simultaneously. Extensions can
choose to allow, abort, or warn about a review being published. Warnings
will be presented as a browser confirmation dialog, giving the user the choice to
either force the publish or cancel it. Abortions will be presented as a simple
alert box with a message about why the validation failed.See the attachement below for an example.
Manually tested this with the Checklist Extension.
Tested this feature with multiple hooks at the same time.
Tested each combination of 2 hooks doing allow, abort, and warning actions.
| Description | From | Last Updated |
|---|---|---|
|
/** |
 david
|
|
|
Maybe assert _.isFunction(this.get('validatePublish')) ? |
david
|
|
|
This should say "validatePublish", not "shouldPublish" |
david
|
|
|
Formatting here doesn't follow our style. We never do blocks without {}, because they're error prone (remember the "goto error" … |
david
|
|
|
Let's leave this inside the publish method rather than in the promise done method. |
david
|
|
|
No space between "function" and "()" |
david
|
|
|
No space between "function" and "()" |
david
|
|
|
No space between "function" and "()" |
david
|
- Groups:
-
-
-
-
-
Formatting here doesn't follow our style. We never do blocks without {}, because they're error prone (remember the "goto error" bug in openssl recently?).
Also, all variables should be defined (though not necessarily assigned) at the top of the function, since 'var' is hoisted to the top.
I also think we probably shouldn't always put "Warning:" into the confirmation message--let's leave that up to the user of the hook.
This should be formatted as:
function setValidation(isValid, message, allowContinue) { var forcePublish, validatePublish; if (isValid) { deferred.resolve(); } else if (allowContinue) { forcePublish = confirm( message + '\n' + gettext('Do you want to continue publishing?')); if (forcePublish) { deferred.resolve(); } else { deferred.reject(); } } else { deferred.reject(); } validatePublish = hook.get('validatePublish'); validatePublish(this, setValidation); } -
-
-
-
-
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/extensions/models/publishingDraftReviewHookModel.js reviewboard/static/rb/js/resources/models/draftReviewModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/extensions/models/publishingDraftReviewHookModel.js reviewboard/static/rb/js/resources/models/draftReviewModel.js
Tool: Pyflakes Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/extensions/models/publishingDraftReviewHookModel.js reviewboard/static/rb/js/resources/models/draftReviewModel.js Tool: PEP8 Style Checker Processed Files: reviewboard/staticbundles.py Ignored Files: reviewboard/static/rb/js/extensions/models/publishingDraftReviewHookModel.js reviewboard/static/rb/js/resources/models/draftReviewModel.js