From: Moriel Schottlender Date: Sat, 13 May 2017 19:28:38 +0000 (-0700) Subject: RCFilters: Fix getFilterRepresentation to consider '0' as false X-Git-Tag: 1.31.0-rc.0~3253^2 X-Git-Url: https://git.cyclocoop.org/%27.WWW_URL.%27admin/?a=commitdiff_plain;h=ee5397eaa025908ac9dfaa3d4688f027c09c8a7d;p=lhc%2Fweb%2Fwiklou.git RCFilters: Fix getFilterRepresentation to consider '0' as false Because '0' is a string, it's true, but for our purposes, it's supposed to be false. Thanks JavaScript. This bug was actually pretty horrific, it meant that when you refresh the representation is all wrong (all items in the group were considered true if the group was 'send_unselected_if_any' which meant that most of those (that are full coverage) 'corrected themselves' to be all-false which meant you lost filters when refreshing, even though the parameters appeared in the URL (the url helpfully corrects itself based on the model but the model was wrong.) How did this pass unit tests, one might ask. Well, the unit tests were treating parameter values as numbers, rather than strings, a fact that is promptly fixed in this commit. Also, for consistency and proper data validation, all parameters are now always stored and handled as strings, in the model. Bug: T165230 Change-Id: I16d8d95be067b3e48e557ef25f8eb6a49736aa4e --- diff --git a/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterGroup.js b/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterGroup.js index 22b2619c1e..59c0a19e6c 100644 --- a/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterGroup.js +++ b/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterGroup.js @@ -118,7 +118,8 @@ if ( model.getType() === 'send_unselected_if_any' ) { // Store the default parameter state // For this group type, parameter values are direct - model.defaultParams[ filter.name ] = Number( !!filter.default ); + // We need to convert from a boolean to a string ('1' and '0') + model.defaultParams[ filter.name ] = String( Number( !!filter.default ) ); } } ); @@ -412,7 +413,9 @@ // Go over the items and define the correct values $.each( filterRepresentation, function ( name, value ) { result[ filterParamNames[ name ] ] = areAnySelected ? - Number( !value ) : 0; + // We must store all parameter values as strings '0' or '1' + String( Number( !value ) ) : + '0'; } ); } else if ( this.getType() === 'string_options' ) { values = []; @@ -451,10 +454,12 @@ paramRepresentation = paramRepresentation || {}; // Expand param representation to include all filters in the group this.getItems().forEach( function ( filterItem ) { - paramRepresentation[ filterItem.getParamName() ] = !!paramRepresentation[ filterItem.getParamName() ]; - paramToFilterMap[ filterItem.getParamName() ] = filterItem; + var paramName = filterItem.getParamName(); - if ( paramRepresentation[ filterItem.getParamName() ] ) { + paramRepresentation[ paramName ] = paramRepresentation[ paramName ] || '0'; + paramToFilterMap[ paramName ] = filterItem; + + if ( Number( paramRepresentation[ filterItem.getParamName() ] ) ) { areAnySelected = true; } } ); diff --git a/resources/src/mediawiki.rcfilters/mw.rcfilters.Controller.js b/resources/src/mediawiki.rcfilters/mw.rcfilters.Controller.js index f3fee74515..81b9dc36c9 100644 --- a/resources/src/mediawiki.rcfilters/mw.rcfilters.Controller.js +++ b/resources/src/mediawiki.rcfilters/mw.rcfilters.Controller.js @@ -44,6 +44,7 @@ parsedSavedQueries, this._getBaseState() ); + this.updateStateBasedOnUrl(); // Update the changes list with the existing data diff --git a/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FiltersViewModel.test.js b/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FiltersViewModel.test.js index c61c2883d8..618afb2df8 100644 --- a/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FiltersViewModel.test.js +++ b/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FiltersViewModel.test.js @@ -188,12 +188,12 @@ assert.deepEqual( model.getDefaultParams(), { - hidefilter1: 1, - hidefilter2: 0, - hidefilter3: 1, - hidefilter4: 0, - hidefilter5: 1, - hidefilter6: 0, + hidefilter1: '1', + hidefilter2: '0', + hidefilter3: '1', + hidefilter4: '0', + hidefilter5: '1', + hidefilter6: '0', group3: 'filter8' }, 'Default parameters are stored properly per filter and group' @@ -364,12 +364,12 @@ assert.deepEqual( model.getParametersFromFilters(), { - hidefilter1: 0, - hidefilter2: 0, - hidefilter3: 0, - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter1: '0', + hidefilter2: '0', + hidefilter3: '0', + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: '' }, 'Unselected filters return all parameters falsey or \'\'.' @@ -389,13 +389,13 @@ model.getParametersFromFilters(), { // Group 1 (one selected, the others are true) - hidefilter1: 0, - hidefilter2: 1, - hidefilter3: 1, + hidefilter1: '0', + hidefilter2: '1', + hidefilter3: '1', // Group 2 (nothing is selected, all false) - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: '' }, 'One filters in one "send_unselected_if_any" group returns the other parameters truthy.' @@ -415,13 +415,13 @@ model.getParametersFromFilters(), { // Group 1 (two selected, the others are true) - hidefilter1: 0, - hidefilter2: 0, - hidefilter3: 1, + hidefilter1: '0', + hidefilter2: '0', + hidefilter3: '1', // Group 2 (nothing is selected, all false) - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: '' }, 'Two filters in one "send_unselected_if_any" group returns the other parameters truthy.' @@ -441,13 +441,13 @@ model.getParametersFromFilters(), { // Group 1 (all selected, all false) - hidefilter1: 0, - hidefilter2: 0, - hidefilter3: 0, + hidefilter1: '0', + hidefilter2: '0', + hidefilter3: '0', // Group 2 (nothing is selected, all false) - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: '' }, 'All filters selected in one "send_unselected_if_any" group returns all parameters falsy.' @@ -464,13 +464,13 @@ model.getParametersFromFilters(), { // Group 1 (all selected, all) - hidefilter1: 0, - hidefilter2: 0, - hidefilter3: 0, + hidefilter1: '0', + hidefilter2: '0', + hidefilter3: '0', // Group 2 (nothing is selected, all false) - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: 'filter7' }, 'One filter selected in "string_option" group returns that filter in the value.' @@ -487,13 +487,13 @@ model.getParametersFromFilters(), { // Group 1 (all selected, all) - hidefilter1: 0, - hidefilter2: 0, - hidefilter3: 0, + hidefilter1: '0', + hidefilter2: '0', + hidefilter3: '0', // Group 2 (nothing is selected, all false) - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: 'filter7,filter8' }, 'Two filters selected in "string_option" group returns those filters in the value.' @@ -510,13 +510,13 @@ model.getParametersFromFilters(), { // Group 1 (all selected, all) - hidefilter1: 0, - hidefilter2: 0, - hidefilter3: 0, + hidefilter1: '0', + hidefilter2: '0', + hidefilter3: '0', // Group 2 (nothing is selected, all false) - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: 'all' }, 'All filters selected in "string_option" group returns \'all\'.' @@ -574,13 +574,13 @@ }, expected: { // Group 1 (two selected, the others are true) - hidefilter1: 0, - hidefilter2: 0, - hidefilter3: 1, + hidefilter1: '0', + hidefilter2: '0', + hidefilter3: '1', // Group 2 (nothing is selected, all false) - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: 'filter7,filter8' }, msg: 'Given an explicit (complete) filter state object, the result is the same as if the object given represented the model state.' @@ -593,13 +593,13 @@ }, expected: { // Group 1 (one selected, the others are true) - hidefilter1: 0, - hidefilter2: 1, - hidefilter3: 1, + hidefilter1: '0', + hidefilter2: '1', + hidefilter3: '1', // Group 2 (nothing is selected, all false) - hidefilter4: 0, - hidefilter5: 0, - hidefilter6: 0, + hidefilter4: '0', + hidefilter5: '0', + hidefilter6: '0', group3: '' }, msg: 'Given an explicit (incomplete) filter state object, the result is the same as if the object give represented the model state.'