jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/353713 )
Change subject: RCFilters: Fix getFilterRepresentation to consider '0' as false
......................................................................
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
---
M resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterGroup.js
M resources/src/mediawiki.rcfilters/mw.rcfilters.Controller.js
M tests/qunit/suites/resources/mediawiki.rcfilters/dm.FiltersViewModel.test.js
3 files changed, 71 insertions(+), 65 deletions(-)
Approvals:
Sbisson: Looks good to me, approved
jenkins-bot: Verified
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
--- 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
--- 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
---
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.'
--
To view, visit https://gerrit.wikimedia.org/r/353713
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I16d8d95be067b3e48e557ef25f8eb6a49736aa4e
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Mooeypoo <mor...@gmail.com>
Gerrit-Reviewer: Sbisson <sbis...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
