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 22b2619..59c0a19 100644 (file)
--- 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 f3fee74..81b9dc3 100644 (file)
--- 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 c61c288..618afb2 100644 (file)
--- 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.'