pawolf

4199: Comment Categorization extension allows empty string even when required

pawolf 
What version are you running?


Review Board 2.5.3, Comment Categorization extension 1.0.1


What's the URL of the page containing the problem?


n/a


What steps will reproduce the problem?


    

  1. Install extension -- sudo easy_install -U rbcommenttype
    2. 

  2. Restart apache
    3. 

  3. Use the admin/extension page to enable the extension, configure several types, check 'Require comment type' and click 'Save'.
    4. 

  4. Go to a review and add a new comment.
    5. 





What is the expected output? What do you see instead?


I would expect the select widget to not have a blank option.  Instead, the first (and initially selected) option is an empty string.  The blank option is also shown when I change the configuration to not require a comment type.


What operating system are you using? What browser?


Windows 7, FireFox 44.0.2


Please provide any additional information below.
pawolf
#1 pawolf 
In looking at the code, I think the problem stems from CommentTypeUtilMixin in commmentType.js.  Since I've configured the extension to require a comment type, 'allowEmpty' on line 29 evaluates to false.  The check 'if (!allowEmpty) {' on line 25 evaluates to true and inserts an empty option.  This works fine for the 'review' dialog for unpublished comments which don't have a comment type set, but isn't the correct behavior for new comments being added in the 'comment' dialog.


One way to fix this would be to create two implementations of function 'selectExistingType'.  The current implementation would be used by the 'review' dialog, and a new one to used by the 'comment' dialog would not check 'type', since it's not set when this dialog is opened when adding a new comment.  Lines 22 - 25 and line 34 could be deleted.


Another way would be to keep the 'Save' button disabled until a value is selected if comment type is set, although this might be a bit tricky (and perhaps undesireable) since this button and its behavior is part of the base support and not this extension.
pawolf
#2 pawolf 
I've attached a patch which seems to work for me.  Not sure if this is the way you would fix it or not.
  • +
    rb-extension-pack.patch 
    diff --git a/rbcommenttype/rbcommenttype/static/js/commentType.js b/rbcommenttype/rbcommenttype/static/js/commentType.js
    index f5299da..b15fb27 100644
    --- a/rbcommenttype/rbcommenttype/static/js/commentType.js
    +++ b/rbcommenttype/rbcommenttype/static/js/commentType.js
    @@ -23,13 +23,21 @@
                 $select.val(type);
             } else {
                 if (!allowEmpty) {
    -                /*
    -                 * If a comment existed before users were forced to choose a
    -                 * type, prepend an empty option even if types aren't required.
    -                 * This way we don't auto-select something for users if they
    -                 * didn't choose it.
    -                 */
    -                $select.prepend('<option selected/>');
    +                if (this.hasOwnProperty('commentDialog')) {
    +                    /*
    +                     * If a new comment is being created, initialize the type to
    +                     * the first option if types are required.
    +                     */
    +