Added new client side hook for validating draft reviews

Review Request #8052 — Created March 10, 2016 and updated

imadueme
Review Board
release-2.5.x
8051
8053
reviewboard, students

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.

Loading file attachments...

Description From Last Updated

/**

daviddavid

Maybe assert _.isFunction(this.get('validatePublish')) ?

daviddavid

This should say "validatePublish", not "shouldPublish"

daviddavid

Formatting here doesn't follow our style. We never do blocks without {}, because they're error prone (remember the "goto error" ...

daviddavid

Let's leave this inside the publish method rather than in the promise done method.

daviddavid

No space between "function" and "()"

daviddavid

No space between "function" and "()"

daviddavid

No space between "function" and "()"

daviddavid
reviewbot
  1. 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
    
    
  2. 
      
imadueme
david
  1. 
      
  2. Maybe assert _.isFunction(this.get('validatePublish')) ?

  3. This should say "validatePublish", not "shouldPublish"

  4. reviewboard/static/rb/js/resources/models/draftReviewModel.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     

    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);
    }
    
  5. Let's leave this inside the publish method rather than in the promise done method.

  6. No space between "function" and "()"

  7. No space between "function" and "()"

  8. No space between "function" and "()"

  9. 
      
imadueme
Review request changed
reviewbot
  1. 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
    
    
  2. 
      
Loading...