r96133 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96132‎ | r96133 | r96134 >
Date:18:00, 2 September 2011
Author:salvatoreingala
Status:deferred
Tags:
Comment:
- Added 'required', 'minlength' and 'maxlength' options for 'list' type fields; added corresponding tests.
- Slightly changed the way 'required', 'minlength' and 'maxlength' options work in 'string' fields; added tests to cover some combinations of those params which were not tested.
Modified paths:
  • /branches/salvatoreingala/Gadgets/Gadgets.i18n.php (modified) (history)
  • /branches/salvatoreingala/Gadgets/Gadgets.php (modified) (history)
  • /branches/salvatoreingala/Gadgets/Gadgets_tests.php (modified) (history)
  • /branches/salvatoreingala/Gadgets/backend/GadgetPrefs.php (modified) (history)
  • /branches/salvatoreingala/Gadgets/ui/resources/jquery.formBuilder.css (modified) (history)
  • /branches/salvatoreingala/Gadgets/ui/resources/jquery.formBuilder.js (modified) (history)

Diff [purge]

Index: branches/salvatoreingala/Gadgets/ui/resources/jquery.formBuilder.css
——@@ -80,8 +80,9 @@
8181 cursor: pointer;
8282 }
8383
84 -.formbuilder-field-list {
 84+.formbuilder-list-items {
8585 border: 1px solid #888;
 86+ border-bottom: none;
8687 }
8788
8889 .formbuilder-list-item-container {
Index: branches/salvatoreingala/Gadgets/ui/resources/jquery.formBuilder.js
——@@ -198,7 +198,7 @@
199199 }, mw.msg( 'gadgets-formbuilder-scalar' ) );
200200
201201 /* Functions used by the preferences editor */
202 - function createFieldDialog( params, options ) {
 202+ function createFieldDialog( params, options ) {
203203 var self = this;
204204
205205 if ( !$.isFunction( params.callback ) ) {
——@@ -365,7 +365,7 @@
366366 } );
367367 }
368368
369 - function showEditFieldDialog( fieldDesc, options, callback ) {
 369+ function showEditFieldDialog( fieldDesc, listParams, options, callback ) {
370370 $( { "fields": [ fieldDesc ] } )
371371 .formBuilder( {
372372 editable: true,
——@@ -400,11 +400,14 @@
401401 $( this ).dialog( "close" );
402402
403403 var ListField = validFieldTypes.list;
404 - callback( new ListField( {
405 - type: 'list',
406 - name: name,
407 - field: fieldDesc
408 - }, options ) );
 404+
 405+ $.extend( listParams, {
 406+ type: 'list',
 407+ name: name,
 408+ field: fieldDesc
 409+ } );
 410+
 411+ callback( new ListField( listParams, options ) );
409412 }
410413 },
411414 {
——@@ -1722,9 +1725,26 @@
17231726 self._createItem( true );
17241727 } )
17251728 .appendTo( this.$div );
 1729+
 1730+ //Add a hidden input to attach validation rules on the number of items
 1731+ //We set its value to "" if there are no elements, or to the number of items otherwise
 1732+ this._$hiddenInput = $( '<input/>').attr( {
 1733+ type: 'hidden',
 1734+ name: this.options.idPrefix + this.desc.name,
 1735+ id: this.options.idPrefix + this.desc.name
 1736+ } )
 1737+ .hide()
 1738+ .appendTo( this.$div );
 1739+
 1740+ this._refreshHiddenField();
17261741 }
17271742 inherit( ListField, EmptyField );
17281743
 1744+ ListField.prototype._refreshHiddenField = function() {
 1745+ var nItems = this._$divItems.children().length;
 1746+ this._$hiddenInput.val( nItems ? nItems : "" );
 1747+ };
 1748+
17291749 ListField.prototype._createItem = function( afterInit, itemValue ) {
17301750 var itemDesc = $.extend( {}, this.desc.field, {
17311751 "name": this.desc.name
——@@ -1763,7 +1783,7 @@
17641784 var $itemButtons = $( '<div/>' )
17651785 .addClass( 'formbuilder-list-item-buttons' );
17661786
1767 -
 1787+ var self = this;
17681788 $( '<span/>' )
17691789 .addClass( 'formbuilder-button formbuilder-list-button-delete ui-icon ui-icon-trash' )
17701790 .attr( 'title', mw.msg( 'gadgets-formbuilder-editor-delete' ) )
——@@ -1771,6 +1791,8 @@
17721792 $itemDiv.slideUp( function() {
17731793 deleteFieldRules( itemField );
17741794 $itemDiv.remove();
 1795+ self._refreshHiddenField();
 1796+ self._$hiddenInput.valid(); //force revalidation of the number of items
17751797 } );
17761798 } )
17771799 .appendTo( $itemButtons );
——@@ -1791,6 +1813,9 @@
17921814 .slideDown();
17931815
17941816 addFieldRules( itemField );
 1817+
 1818+ this._refreshHiddenField();
 1819+ this._$hiddenInput.valid(); //force revalidation of the number of items
17951820 } else {
17961821 $itemDiv.appendTo( this._$divItems );
17971822 }
——@@ -1817,7 +1842,28 @@
18181843 };
18191844
18201845 ListField.prototype.getValidationSettings = function() {
1821 - var validationSettings = {};
 1846+ var validationSettings = EmptyField.prototype.getValidationSettings.call( this );
 1847+ hiddenFieldRules = {}, hiddenFieldMessages = {};
 1848+
 1849+ if ( typeof this.desc.required != 'undefined' ) {
 1850+ hiddenFieldRules.required = this.desc.required;
 1851+ hiddenFieldMessages.required = mw.msg( 'gadgets-formbuilder-list-required' );
 1852+ }
 1853+
 1854+ if ( typeof this.desc.minlength != 'undefined' ) {
 1855+ hiddenFieldRules.min = this.desc.minlength;
 1856+ hiddenFieldMessages.min = mw.msg( 'gadgets-formbuilder-list-minlength', this.desc.minlength );
 1857+ }
 1858+
 1859+ if ( typeof this.desc.maxlength == 'undefined' ) {
 1860+ this.desc.maxlength = 1024;
 1861+ }
 1862+ hiddenFieldRules.max = this.desc.maxlength;
 1863+ hiddenFieldMessages.max = mw.msg( 'gadgets-formbuilder-list-maxlength', this.desc.maxlength );
 1864+
 1865+ validationSettings.rules[this.options.idPrefix + this.desc.name] = hiddenFieldRules;
 1866+ validationSettings.messages[this.options.idPrefix + this.desc.name] = hiddenFieldMessages;
 1867+
18221868 this._$divItems.children().each( function( index, divItem ) {
18231869 var field = $( divItem ).data( 'field' );
18241870 $.extend( true, validationSettings, field.getValidationSettings() );
——@@ -1931,9 +1977,23 @@
19321978 "builder": simpleFields.concat( [
19331979 {
19341980 "name": "required",
1935 - "type": "boolean",
 1981+ "type": "select",
19361982 "label": "required",
1937 - "default": false
 1983+ "default": null,
 1984+ "options": [
 1985+ {
 1986+ "name": "not specified",
 1987+ "value": null
 1988+ },
 1989+ {
 1990+ "name": "true",
 1991+ "value": true
 1992+ },
 1993+ {
 1994+ "name": "false",
 1995+ "value": false
 1996+ }
 1997+ ]
19381998 },
19391999 {
19402000 "name": "minlength",
——@@ -2088,23 +2148,64 @@
20892149 }
20902150 } );
20912151
2092 - //Create the dialog to chose the field type
2093 - var $form = $( {
2094 - fields: [ {
2095 - "name": "name",
2096 - "type": "string",
2097 - "label": "name",
2098 - "required": true,
2099 - "maxlength": 40,
2100 - "default": ""
2101 - },
2102 - {
2103 - "name": "type",
2104 - "type": "select",
2105 - "label": "type",
2106 - "options": selectOptions
2107 - } ]
2108 - } ).formBuilder( { idPrefix: 'list-chose-type-' } )
 2152+ //Create the dialog to chose the field type and set list properties
 2153+ var description = {
 2154+ "fields": [
 2155+ {
 2156+ "name": "name",
 2157+ "type": "string",
 2158+ "label": "name",
 2159+ "required": true,
 2160+ "maxlength": 40,
 2161+ "default": ""
 2162+ },
 2163+ {
 2164+ "name": "required",
 2165+ "type": "select",
 2166+ "label": "required",
 2167+ "default": null,
 2168+ "options": [
 2169+ {
 2170+ "name": "not specified",
 2171+ "value": null
 2172+ },
 2173+ {
 2174+ "name": "true",
 2175+ "value": true
 2176+ },
 2177+ {
 2178+ "name": "false",
 2179+ "value": false
 2180+ }
 2181+ ]
 2182+ },
 2183+ {
 2184+ "name": "minlength",
 2185+ "type": "number",
 2186+ "label": "minlength",
 2187+ "integer": true,
 2188+ "min": 0,
 2189+ "required": false,
 2190+ "default": null
 2191+ },
 2192+ {
 2193+ "name": "maxlength",
 2194+ "type": "number",
 2195+ "label": "maxlength",
 2196+ "integer": true,
 2197+ "min": 1,
 2198+ "required": false,
 2199+ "default": null
 2200+ },
 2201+ {
 2202+ "name": "type",
 2203+ "type": "select",
 2204+ "label": "type",
 2205+ "options": selectOptions
 2206+ }
 2207+ ]
 2208+ };
 2209+ var $form = $( description ).formBuilder( { idPrefix: 'list-chose-type-' } )
21092210 .submit( function() {
21102211 return false; //prevent form submission
21112212 } );
——@@ -2123,6 +2224,14 @@
21242225 click: function() {
21252226 var values = $( this ).formBuilder( 'getValues' );
21262227 $( this ).dialog( "close" );
 2228+
 2229+ //Remove properties that equal their default
 2230+ $.each( description.fields, function( index, fieldSpec ) {
 2231+ var property = fieldSpec.name;
 2232+ if ( values[property] === fieldSpec['default'] ) {
 2233+ delete values[property];
 2234+ }
 2235+ } );
21272236
21282237 var $dialog = $( this );
21292238 createFieldDialog( {
——@@ -2132,7 +2241,7 @@
21332242 },
21342243 callback: function( field ) {
21352244 $dialog.dialog( 'close' );
2136 - showEditFieldDialog( field.getDesc(), options, callback );
 2245+ showEditFieldDialog( field.getDesc(), values, options, callback );
21372246 return true;
21382247 }
21392248 }, { editable: true } );
Index: branches/salvatoreingala/Gadgets/Gadgets.i18n.php
——@@ -63,6 +63,9 @@
6464 'gadgets-formbuilder-date' => 'Please enter a valid date.',
6565 'gadgets-formbuilder-color' => 'Please enter a valid color.',
6666 'gadgets-formbuilder-scalar' => 'Valid values are true, false, null, a floating point number or a double quoted string.',
 67+ 'gadgets-formbuilder-list-required' => 'Please create at least one item.',
 68+ 'gadgets-formbuilder-list-minlength' => 'Please insert at least $1 items.',
 69+ 'gadgets-formbuilder-list-maxlength' => 'Please insert no more than $1 items.',
6770 'gadgets-formbuilder-editor-ok' => 'OK',
6871 'gadgets-formbuilder-editor-cancel' => 'Cancel',
6972 'gadgets-formbuilder-editor-move-field' => 'Move this field',
Index: branches/salvatoreingala/Gadgets/Gadgets.php
——@@ -92,7 +92,8 @@
9393 'messages' => array(
9494 'gadgets-formbuilder-required', 'gadgets-formbuilder-minlength', 'gadgets-formbuilder-maxlength',
9595 'gadgets-formbuilder-min', 'gadgets-formbuilder-max', 'gadgets-formbuilder-integer', 'gadgets-formbuilder-date',
96 - 'gadgets-formbuilder-color', 'gadgets-formbuilder-scalar',
 96+ 'gadgets-formbuilder-color', 'gadgets-formbuilder-list-required', 'gadgets-formbuilder-list-minlength',
 97+ 'gadgets-formbuilder-list-maxlength', 'gadgets-formbuilder-scalar',
9798 'gadgets-formbuilder-editor-ok', 'gadgets-formbuilder-editor-cancel', 'gadgets-formbuilder-editor-move-field',
9899 'gadgets-formbuilder-editor-delete-field', 'gadgets-formbuilder-editor-edit-field', 'gadgets-formbuilder-editor-edit-field-title', 'gadgets-formbuilder-editor-insert-field',
99100 'gadgets-formbuilder-editor-chose-field', 'gadgets-formbuilder-editor-chose-field-title', 'gadgets-formbuilder-editor-create-field-title',
Index: branches/salvatoreingala/Gadgets/Gadgets_tests.php
——@@ -274,7 +274,6 @@
275275 'label' => 'some label',
276276 'minlength' => 6,
277277 'maxlength' => 10,
278 - 'required' => false,
279278 'default' => 'default'
280279 )
281280 )
——@@ -282,8 +281,23 @@
283282
284283 $this->assertTrue( GadgetPrefs::isPrefsDescriptionValid( $correct ) );
285284
286 - //Tests with wrong default values
 285+ //Tests with wrong default values (when 'required' is not given)
287286 $wrong = $correct;
 287+ foreach ( array( null, '', true, false, 0, 1, array(), 'short', 'veryverylongstring' ) as $def ) {
 288+ $wrong['fields'][0]['default'] = $def;
 289+ $this->assertFalse( GadgetPrefs::isPrefsDescriptionValid( $wrong ) );
 290+ }
 291+
 292+ //Tests with correct default values (when required is not given)
 293+ $correct2 = $correct;
 294+ foreach ( array( '6chars', '1234567890' ) as $def ) {
 295+ $correct2['fields'][0]['default'] = $def;
 296+ $this->assertTrue( GadgetPrefs::isPrefsDescriptionValid( $correct2 ) );
 297+ }
 298+
 299+ //Tests with wrong default values (when 'required' is false)
 300+ $wrong = $correct;
 301+ $wrong['fields'][0]['required'] = false;
288302 foreach ( array( null, true, false, 0, 1, array(), 'short', 'veryverylongstring' ) as $def ) {
289303 $wrong['fields'][0]['default'] = $def;
290304 $this->assertFalse( GadgetPrefs::isPrefsDescriptionValid( $wrong ) );
——@@ -291,16 +305,36 @@
292306
293307 //Tests with correct default values (when required is false)
294308 $correct2 = $correct;
 309+ $correct2['fields'][0]['required'] = false;
295310 foreach ( array( '', '6chars', '1234567890' ) as $def ) {
296311 $correct2['fields'][0]['default'] = $def;
297312 $this->assertTrue( GadgetPrefs::isPrefsDescriptionValid( $correct2 ) );
298313 }
299314
 315+ $correct = array(
 316+ 'fields' => array(
 317+ array(
 318+ 'name' => 'testString',
 319+ 'type' => 'string',
 320+ 'label' => 'some label',
 321+ 'default' => ''
 322+ )
 323+ )
 324+ );
 325+
300326 //Test with empty default when "required" is true
 327+ $this->assertTrue( GadgetPrefs::isPrefsDescriptionValid( $correct ) );
 328+
 329+ //Test with empty default when "required" is true
301330 $wrong = $correct;
302 - $wrong['fields']['testString']['required'] = true;
303 - $wrong['fields']['testString']['default'] = '';
 331+ $wrong['fields'][0]['required'] = true;
304332 $this->assertFalse( GadgetPrefs::isPrefsDescriptionValid( $wrong ) );
 333+
 334+ //Test with empty default when "required" is false and minlength is given
 335+ $correct2 = $correct;
 336+ $correct2['fields'][0]['required'] = false;
 337+ $correct2['fields'][0]['minlength'] = 3;
 338+ $this->assertTrue( GadgetPrefs::isPrefsDescriptionValid( $correct2 ) );
305339 }
306340
307341 //Tests for 'number' type preferences
——@@ -707,6 +741,54 @@
708742 )
709743 ) );
710744
 745+
 746+ //Tests with 'minlength' and 'maxlength' options
 747+ $wrong = $correct;
 748+ $wrong['fields'][0]['minlength'] = 4;
 749+ $wrong['fields'][0]['maxlength'] = 3; //maxlength < minlength, wrong
 750+ $this->assertFalse( GadgetPrefs::isPrefsDescriptionValid( $wrong ) );
 751+
 752+ $correct2 = $correct;
 753+ $correct2['fields'][0]['minlength'] = 2;
 754+ $correct2['fields'][0]['maxlength'] = 3;
 755+ $correct2['fields'][0]['default'] = array(
 756+ array( 'bar' => true, 'car' => '#115599' ),
 757+ array( 'bar' => false, 'car' => '#123456' )
 758+ );
 759+ $this->assertTrue( GadgetPrefs::isPrefsDescriptionValid( $correct2 ) );
 760+
 761+ $this->assertFalse( GadgetPrefs::checkPrefsAgainstDescription(
 762+ $correct2,
 763+ array( 'foo' => array( //less than minlength items
 764+ array( 'bar' => true, 'car' => '#115599' )
 765+ )
 766+ )
 767+ ) );
 768+
 769+ $this->assertFalse( GadgetPrefs::checkPrefsAgainstDescription(
 770+ $correct2,
 771+ array( 'foo' => array() ) //empty array, must fail because "required" is not false
 772+ ) );
 773+
 774+ $this->assertFalse( GadgetPrefs::checkPrefsAgainstDescription(
 775+ $correct2,
 776+ array( 'foo' => array( //more than minlength items
 777+ array( 'bar' => true, 'car' => '#115599' ),
 778+ array( 'bar' => false, 'car' => '#123456' ),
 779+ array( 'bar' => true, 'car' => '#ffffff' ),
 780+ array( 'bar' => false, 'car' => '#2357bd' )
 781+ )
 782+ )
 783+ ) );
 784+
 785+ //Test with 'required'
 786+ $correct2['fields'][0]['required'] = false;
 787+ $this->assertTrue( GadgetPrefs::checkPrefsAgainstDescription(
 788+ $correct2,
 789+ array( 'foo' => array() ) //empty array, must be accepted because "required" is false
 790+ ) );
 791+
 792+ //Tests matchPrefsWithDescription
711793 $prefs = array( 'foo' => array(
712794 array(
713795 'bar' => null,
——@@ -723,6 +805,7 @@
724806 )
725807 );
726808
 809+
727810 GadgetPrefs::matchPrefsWithDescription( $correct, $prefs );
728811 $this->assertTrue( GadgetPrefs::checkPrefsAgainstDescription( $correct, $prefs ) );
729812 }
Index: branches/salvatoreingala/Gadgets/backend/GadgetPrefs.php
——@@ -254,6 +254,18 @@
255255 ),
256256 'default' => array(
257257 'isMandatory' => true
 258+ ),
 259+ 'required' => array(
 260+ 'isMandatory' => false,
 261+ 'validator' => 'is_bool'
 262+ ),
 263+ 'minlength' => array(
 264+ 'isMandatory' => false,
 265+ 'validator' => 'is_integer'
 266+ ),
 267+ 'maxlength' => array(
 268+ 'isMandatory' => false,
 269+ 'validator' => 'is_integer'
258270 )
259271 ),
260272 'validator' => 'GadgetPrefs::validateListPrefDefinition',
——@@ -406,6 +418,13 @@
407419 return false;
408420 }
409421
 422+ //Validate minlength and maxlength
 423+ $minlength = isset( $prefDefinition['minlength'] ) ? $prefDefinition['minlength'] : 0;
 424+ $maxlength = isset( $prefDefinition['maxlength'] ) ? $prefDefinition['maxlength'] : 1024;
 425+ if ( $minlength < 0 || $maxlength <= 0 || $minlength > $maxlength ) {
 426+ return false;
 427+ }
 428+
410429 //Check if the field definition is valid, apart from missing the name
411430 $itemDescription = $prefDefinition['field'];
412431 $itemDescription['name'] = 'dummy';
——@@ -654,11 +673,13 @@
655674 $len = strlen( $value );
656675
657676 //Checks the "required" option, if present
658 - $required = isset( $prefDescription['required'] ) ? $prefDescription['required'] : true;
659 - if ( $required === true && $len == 0 ) {
660 - return false;
661 - } elseif ( $required === false && $len == 0 ) {
662 - return true; //overriding 'minlength'
 677+ if ( isset( $prefDescription['required'] ) ) {
 678+ $required = isset( $prefDescription['required'] ) ? $prefDescription['required'] : true;
 679+ if ( $required === true && $len == 0 ) {
 680+ return false;
 681+ } elseif ( $required === false && $len == 0 ) {
 682+ return true; //overriding 'minlength', if given
 683+ }
663684 }
664685
665686 //Checks the "minlength" option, if present
——@@ -803,6 +824,24 @@
804825 return false;
805826 }
806827
 828+ $nItems = count( $value );
 829+
 830+ //Checks the "required" option, if present
 831+ if ( isset( $prefDescription['required'] ) ) {
 832+ $required = $prefDescription['required'];
 833+ if ( $required === true && $nItems == 0 ) {
 834+ return false;
 835+ } elseif ( $required === false && $nItems == 0 ) {
 836+ return true; //overriding 'minlength'
 837+ }
 838+ }
 839+
 840+ $minlength = isset( $prefDescription['minlength'] ) ? $prefDescription['minlength'] : 0;
 841+ $maxlength = isset( $prefDescription['maxlength'] ) ? $prefDescription['maxlength'] : 1024;
 842+ if ( $nItems < $minlength || $nItems > $maxlength ) {
 843+ return false;
 844+ }
 845+
807846 $itemDescription = $prefDescription['field'];
808847 foreach ( $value as $item ) {
809848 if ( !self::checkSinglePref( $itemDescription, array( 'dummy' => $item ), 'dummy' ) ) {

Status & tagging log