From e27da9d8a54450df766da5611b1ce7b9d95f5b71 Mon Sep 17 00:00:00 2001 From: Moriel Schottlender Date: Fri, 10 Mar 2017 15:22:12 -0800 Subject: [PATCH] RCFilters UI: Rework conflicts to be objects in filter or group context Allow conflicts to be defined in either the filter or the group context and represent a whole object rather than an array of filter names. Bug: T160453 Bug: T152754 Bug: T156427 Change-Id: I2423eb2618aa64bf30395b1a1912589e0c71f283 --- .../dm/mw.rcfilters.dm.FilterGroup.js | 74 +++++++- .../dm/mw.rcfilters.dm.FilterItem.js | 30 +++- .../dm/mw.rcfilters.dm.FiltersViewModel.js | 76 +++++--- .../mediawiki.rcfilters/dm.FilterItem.test.js | 17 +- .../dm.FiltersViewModel.test.js | 163 +++++++++++++++--- 5 files changed, 295 insertions(+), 65 deletions(-) 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 14a610b71e..22323e8498 100644 --- a/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterGroup.js +++ b/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterGroup.js @@ -13,6 +13,7 @@ * @cfg {string} [separator='|'] Value separator for 'string_options' groups * @cfg {boolean} [active] Group is active * @cfg {boolean} [fullCoverage] This filters in this group collectively cover all results + * @cfg {Object} [conflicts] Defines the conflicts for this filter group */ mw.rcfilters.dm.FilterGroup = function MwRcfiltersDmFilterGroup( name, config ) { config = config || {}; @@ -29,6 +30,8 @@ this.active = !!config.active; this.fullCoverage = !!config.fullCoverage; + this.conflicts = config.conflicts || {}; + this.aggregate( { update: 'filterItemUpdate' } ); this.connect( this, { filterItemUpdate: 'onFilterItemUpdate' } ); }; @@ -81,6 +84,54 @@ return this.name; }; + /** + * Get the conflicts associated with the entire group. + * Conflict object is set up by filter name keys and conflict + * definition. For example: + * [ + * { + * filterName: { + * filter: filterName, + * group: group1 + * } + * }, + * { + * filterName2: { + * filter: filterName2, + * group: group2 + * } + * } + * ] + * @return {Object} Conflict definition + */ + mw.rcfilters.dm.FilterGroup.prototype.getConflicts = function () { + return this.conflicts; + }; + + /** + * Set conflicts for this group. See #getConflicts for the expected + * structure of the definition. + * + * @param {Object} conflicts Conflicts for this group + */ + mw.rcfilters.dm.FilterGroup.prototype.setConflicts = function ( conflicts ) { + this.conflicts = conflicts; + }; + + /** + * Check whether this item has a potential conflict with the given item + * + * This checks whether the given item is in the list of conflicts of + * the current item, but makes no judgment about whether the conflict + * is currently at play (either one of the items may not be selected) + * + * @param {mw.rcfilters.dm.FilterItem} filterItem Filter item + * @return {boolean} This item has a conflict with the given item + */ + mw.rcfilters.dm.FilterGroup.prototype.existsInConflicts = function ( filterItem ) { + return Object.prototype.hasOwnProperty.call( this.getConflicts(), filterItem.getName() ); + }; + /** * Check whether there are any items selected * @@ -126,9 +177,15 @@ mw.rcfilters.dm.FilterGroup.prototype.areAllSelectedInConflictWith = function ( filterItem ) { var selectedItems = this.getSelectedItems( filterItem ); - return selectedItems.length > 0 && selectedItems.every( function ( selectedFilter ) { - return selectedFilter.existsInConflicts( filterItem ); - } ); + return selectedItems.length > 0 && + ( + // The group as a whole is in conflict with this item + this.existsInConflicts( filterItem ) || + // All selected items are in conflict individually + selectedItems.every( function ( selectedFilter ) { + return selectedFilter.existsInConflicts( filterItem ); + } ) + ); }; /** @@ -140,9 +197,14 @@ mw.rcfilters.dm.FilterGroup.prototype.areAnySelectedInConflictWith = function ( filterItem ) { var selectedItems = this.getSelectedItems( filterItem ); - return selectedItems.length > 0 && selectedItems.some( function ( selectedFilter ) { - return selectedFilter.existsInConflicts( filterItem ); - } ); + return selectedItems.length > 0 && ( + // The group as a whole is in conflict with this item + this.existsInConflicts( filterItem ) || + // Any selected items are in conflict individually + selectedItems.some( function ( selectedFilter ) { + return selectedFilter.existsInConflicts( filterItem ); + } ) + ); }; /** diff --git a/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterItem.js b/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterItem.js index 0df34f8f16..f162528587 100644 --- a/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterItem.js +++ b/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FilterItem.js @@ -16,7 +16,7 @@ * selected, makes inactive. * @cfg {boolean} [selected] The item is selected * @cfg {string[]} [subset] Defining the names of filters that are a subset of this filter - * @cfg {string[]} [conflictsWith] Defining the names of filters that conflict with this item + * @cfg {Object} [conflicts] Defines the conflicts for this filter * @cfg {string} [cssClass] The class identifying the results that match this filter */ mw.rcfilters.dm.FilterItem = function MwRcfiltersDmFilterItem( name, groupModel, config ) { @@ -34,7 +34,7 @@ // Interaction definitions this.subset = config.subset || []; - this.conflicts = config.conflicts || []; + this.conflicts = config.conflicts || {}; this.superset = []; // Interaction states @@ -192,19 +192,33 @@ /** * Get filter conflicts * - * @return {string[]} Filter conflicts + * Conflict object is set up by filter name keys and conflict + * definition. For example: + * { + * filterName: { + * filter: filterName, + * group: group1 + * } + * filterName2: { + * filter: filterName2, + * group: group2 + * } + * } + * + * @return {Object} Filter conflicts */ mw.rcfilters.dm.FilterItem.prototype.getConflicts = function () { - return this.conflicts; + return $.extend( {}, this.conflicts, this.getGroupModel().getConflicts() ); }; /** - * Set filter conflicts + * Set conflicts for this filter. See #getConflicts for the expected + * structure of the definition. * - * @param {string[]} conflicts Filter conflicts + * @param {Object} conflicts Conflicts for this filter */ mw.rcfilters.dm.FilterItem.prototype.setConflicts = function ( conflicts ) { - this.conflicts = conflicts || []; + this.conflicts = conflicts || {}; }; /** @@ -237,7 +251,7 @@ * @return {boolean} This item has a conflict with the given item */ mw.rcfilters.dm.FilterItem.prototype.existsInConflicts = function ( filterItem ) { - return this.conflicts.indexOf( filterItem.getName() ) > -1; + return Object.prototype.hasOwnProperty.call( this.getConflicts(), filterItem.getName() ); }; /** diff --git a/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FiltersViewModel.js b/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FiltersViewModel.js index 3bb7716107..36fc4a71be 100644 --- a/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FiltersViewModel.js +++ b/resources/src/mediawiki.rcfilters/dm/mw.rcfilters.dm.FiltersViewModel.js @@ -162,9 +162,12 @@ * @param {Array} filters Filter group definition */ mw.rcfilters.dm.FiltersViewModel.prototype.initializeFilters = function ( filters ) { - var i, filterItem, selectedFilterNames, + var i, filterItem, selectedFilterNames, filterConflictResult, groupConflictResult, model = this, items = [], + supersetMap = {}, + groupConflictMap = {}, + filterConflictMap = {}, addArrayElementsUnique = function ( arr, elements ) { elements = Array.isArray( elements ) ? elements : [ elements ]; @@ -176,8 +179,31 @@ return arr; }, - conflictMap = {}, - supersetMap = {}; + expandConflictDefinitions = function ( obj ) { + var result = {}; + + $.each( obj, function ( group, conflicts ) { + var adjustedConflicts = {}; + conflicts.forEach( function ( conflict ) { + if ( conflict.filter ) { + adjustedConflicts[ conflict.filter ] = conflict; + } else { + // This conflict is for an entire group. Split it up to + // represent each filter + + // Get the relevant group items + model.groups[ conflict.group ].getItems().forEach( function ( groupItem ) { + // Rebuild the conflict + adjustedConflicts[ groupItem.getName() ] = $.extend( {}, conflict, { filter: groupItem.getName() } ); + } ); + } + } ); + + result[ group ] = adjustedConflicts; + } ); + + return result; + }; // Reset this.clearItems(); @@ -195,6 +221,10 @@ } ); } + if ( data.conflicts ) { + groupConflictMap[ group ] = data.conflicts; + } + selectedFilterNames = []; for ( i = 0; i < data.filters.length; i++ ) { data.filters[ i ].subset = data.filters[ i ].subset || []; @@ -224,26 +254,9 @@ } ); } - // Conflicts are bi-directional, which means FilterA can define having - // a conflict with FilterB, and this conflict should appear in **both** - // filter definitions. - // We need to remap all the 'conflicts' so they reflect the entire state - // in either direction regardless of which filter defined the other as conflicting. + // Store conflicts if ( data.filters[ i ].conflicts ) { - conflictMap[ filterItem.getName() ] = conflictMap[ filterItem.getName() ] || []; - addArrayElementsUnique( - conflictMap[ filterItem.getName() ], - data.filters[ i ].conflicts - ); - - data.filters[ i ].conflicts.forEach( function ( conflictingFilterName ) { // eslint-disable-line no-loop-func - // Add this filter to the conflicts of each of the filters in its list - conflictMap[ conflictingFilterName ] = conflictMap[ conflictingFilterName ] || []; - addArrayElementsUnique( - conflictMap[ conflictingFilterName ], - filterItem.getName() - ); - } ); + filterConflictMap[ data.filters[ i ].name ] = data.filters[ i ].conflicts; } if ( data.type === 'send_unselected_if_any' ) { @@ -269,14 +282,23 @@ } } ); - items.forEach( function ( filterItem ) { - // Apply conflict map to the items - // Now that we mapped all items and conflicts bi-directionally - // we need to apply the definition to each filter again - filterItem.setConflicts( conflictMap[ filterItem.getName() ] ); + // Expand conflicts + groupConflictResult = expandConflictDefinitions( groupConflictMap ); + filterConflictResult = expandConflictDefinitions( filterConflictMap ); + // Set conflicts for groups + $.each( groupConflictResult, function ( group, conflicts ) { + model.groups[ group ].setConflicts( conflicts ); + } ); + + items.forEach( function ( filterItem ) { // Apply the superset map filterItem.setSuperset( supersetMap[ filterItem.getName() ] ); + + // set conflicts for item + if ( filterConflictResult[ filterItem.getName() ] ) { + filterItem.setConflicts( filterConflictResult[ filterItem.getName() ] ); + } } ); // Add items to the model diff --git a/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FilterItem.test.js b/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FilterItem.test.js index 25ea98812c..3232d08a68 100644 --- a/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FilterItem.test.js +++ b/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FilterItem.test.js @@ -3,7 +3,8 @@ QUnit.test( 'Initializing filter item', function ( assert ) { var item, - group1 = new mw.rcfilters.dm.FilterGroup( 'group1' ); + group1 = new mw.rcfilters.dm.FilterGroup( 'group1' ), + group2 = new mw.rcfilters.dm.FilterGroup( 'group2' ); item = new mw.rcfilters.dm.FilterItem( 'filter1', group1 ); assert.equal( @@ -96,18 +97,26 @@ 'filter1', group1, { - conflicts: [ 'conflict1', 'conflict2', 'conflict3' ] + conflicts: { + conflict1: { group: 'group2', filter: 'conflict1' }, + conflict2: { group: 'group2', filter: 'conflict2' }, + conflict3: { group: 'group2', filter: 'conflict3' } + } } ); assert.deepEqual( item.getConflicts(), - [ 'conflict1', 'conflict2', 'conflict3' ], + { + conflict1: { group: 'group2', filter: 'conflict1' }, + conflict2: { group: 'group2', filter: 'conflict2' }, + conflict3: { group: 'group2', filter: 'conflict3' } + }, 'Conflict information is retained.' ); assert.equal( // TODO: Consider allowing for either a FilterItem or a filter name // in this method, so it is consistent with the subset one - item.existsInConflicts( new mw.rcfilters.dm.FilterItem( 'conflict1', group1 ) ), + item.existsInConflicts( new mw.rcfilters.dm.FilterItem( 'conflict1', group2 ) ), true, 'Specific item exists in conflicts.' ); 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 52ba3601d7..60a32c3c4c 100644 --- a/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FiltersViewModel.test.js +++ b/tests/qunit/suites/resources/mediawiki.rcfilters/dm.FiltersViewModel.test.js @@ -1052,13 +1052,13 @@ name: 'filter1', label: '1', description: '1', - conflicts: [ 'filter2', 'filter4' ] + conflicts: [ { group: 'group2' } ] }, { name: 'filter2', label: '2', description: '2', - conflicts: [ 'filter6' ] + conflicts: [ { group: 'group2', filter: 'filter6' } ] }, { name: 'filter3', @@ -1070,6 +1070,7 @@ name: 'group2', title: 'Group 2', type: 'send_unselected_if_any', + conflicts: [ { group: 'group1', filter: 'filter1' } ], filters: [ { name: 'filter4', @@ -1079,13 +1080,13 @@ { name: 'filter5', label: '5', - description: '5', - conflicts: [ 'filter3' ] + description: '5' }, { name: 'filter6', label: '6', - description: '6' + description: '6', + conflicts: [ { group: 'group1', filter: 'filter2' } ] } ] } ], @@ -1107,9 +1108,9 @@ 'Initial state: no conflicts because no selections.' ); - // Select a filter that has a conflict with another + // Select a filter that has a conflict with an entire group model.toggleFiltersSelected( { - filter1: true // conflicts: filter2, filter4 + filter1: true // conflicts: entire of group 2 ( filter4, filter5, filter6) } ); model.reassessFilterInteractions( model.getItemByName( 'filter1' ) ); @@ -1118,10 +1119,11 @@ model.getFullState(), $.extend( true, {}, baseFullState, { filter1: { selected: true }, - filter2: { conflicted: true }, - filter4: { conflicted: true } + filter4: { conflicted: true }, + filter5: { conflicted: true }, + filter6: { conflicted: true } } ), - 'Selecting a filter set its conflicts list as "conflicted".' + 'Selecting a filter that conflicts with a group sets all the conflicted group items as "conflicted".' ); // Select one of the conflicts (both filters are now conflicted and selected) @@ -1134,29 +1136,150 @@ model.getFullState(), $.extend( true, {}, baseFullState, { filter1: { selected: true, conflicted: true }, - filter2: { conflicted: true }, - filter4: { selected: true, conflicted: true } + filter4: { selected: true, conflicted: true }, + filter5: { conflicted: true }, + filter6: { conflicted: true } } ), - 'Selecting a conflicting filter sets both sides to conflicted and selected.' + 'Selecting a conflicting filter inside a group, sets both sides to conflicted and selected.' ); - // Select another filter from filter4 group, meaning: - // now filter1 no longer conflicts with filter4 + // Reset + model = new mw.rcfilters.dm.FiltersViewModel(); + model.initializeFilters( definition ); + + // Select a filter that has a conflict with a specific filter + model.toggleFiltersSelected( { + filter2: true // conflicts: filter6 + } ); + + model.reassessFilterInteractions( model.getItemByName( 'filter2' ) ); + + assert.deepEqual( + model.getFullState(), + $.extend( true, {}, baseFullState, { + filter2: { selected: true }, + filter6: { conflicted: true } + } ), + 'Selecting a filter that conflicts with another filter sets the other as "conflicted".' + ); + + // Select the conflicting filter model.toggleFiltersSelected( { filter6: true // conflicts: filter2 } ); + model.reassessFilterInteractions( model.getItemByName( 'filter6' ) ); assert.deepEqual( model.getFullState(), $.extend( true, {}, baseFullState, { - filter1: { selected: true, conflicted: false }, // No longer conflicts (filter4 is not the only in the group) - filter2: { conflicted: true }, // While not selected, still in conflict with filter1, which is selected - filter4: { selected: true, conflicted: false }, // No longer conflicts with filter1 - filter6: { selected: true, conflicted: false } + filter2: { selected: true, conflicted: true }, + filter6: { selected: true, conflicted: true }, + // This is added to the conflicts because filter6 is part of group2, + // who is in conflict with filter1; note that filter2 also conflicts + // with filter6 which means that filter1 conflicts with filter6 (because it's in group2) + // and also because its **own sibling** (filter2) is **also** in conflict with the + // selected items in group2 (filter6) + filter1: { conflicted: true } + } ), + 'Selecting a conflicting filter with an individual filter, sets both sides to conflicted and selected.' + ); + + // Now choose a non-conflicting filter from the group + model.toggleFiltersSelected( { + filter5: true + } ); + + model.reassessFilterInteractions( model.getItemByName( 'filter5' ) ); + + assert.deepEqual( + model.getFullState(), + $.extend( true, {}, baseFullState, { + filter2: { selected: true }, + filter6: { selected: true }, + filter5: { selected: true } + // Filter6 and filter1 are no longer in conflict because + // filter5, while it is in conflict with filter1, it is + // not in conflict with filter2 - and since filter2 is + // selected, it removes the conflict bidirectionally + } ), + 'Selecting a non-conflicting filter within the group of a conflicting filter removes the conflicts.' + ); + + // Followup on the previous test, unselect filter2 so filter1 + // is now the only one selected in its own group, and since + // it is in conflict with the entire of group2, it means + // filter1 is once again conflicted + model.toggleFiltersSelected( { + filter2: false + } ); + + model.reassessFilterInteractions( model.getItemByName( 'filter2' ) ); + + assert.deepEqual( + model.getFullState(), + $.extend( true, {}, baseFullState, { + filter1: { conflicted: true }, + filter6: { selected: true }, + filter5: { selected: true } } ), - 'Selecting a non-conflicting filter from a conflicting group removes the conflict' + 'Unselecting an item that did not conflict returns the conflict state.' ); + + // Followup #2: Now actually select filter1, and make everything conflicted + model.toggleFiltersSelected( { + filter1: true + } ); + + model.reassessFilterInteractions( model.getItemByName( 'filter1' ) ); + + assert.deepEqual( + model.getFullState(), + $.extend( true, {}, baseFullState, { + filter1: { selected: true, conflicted: true }, + filter6: { selected: true, conflicted: true }, + filter5: { selected: true, conflicted: true }, + filter4: { conflicted: true } // Not selected but conflicted because it's in group2 + } ), + 'Selecting an item that conflicts with a whole group makes all selections in that group conflicted.' + ); + + /* Simple case */ + // Reset + model = new mw.rcfilters.dm.FiltersViewModel(); + model.initializeFilters( definition ); + + // Select a filter that has a conflict with a specific filter + model.toggleFiltersSelected( { + filter2: true // conflicts: filter6 + } ); + + model.reassessFilterInteractions( model.getItemByName( 'filter2' ) ); + + assert.deepEqual( + model.getFullState(), + $.extend( true, {}, baseFullState, { + filter2: { selected: true }, + filter6: { conflicted: true } + } ), + 'Simple case: Selecting a filter that conflicts with another filter sets the other as "conflicted".' + ); + + model.toggleFiltersSelected( { + filter3: true // conflicts: filter6 + } ); + + model.reassessFilterInteractions( model.getItemByName( 'filter3' ) ); + + assert.deepEqual( + model.getFullState(), + $.extend( true, {}, baseFullState, { + filter2: { selected: true }, + filter3: { selected: true } + } ), + 'Simple case: Selecting a filter that is not in conflict removes the conflict.' + ); + } ); QUnit.test( 'Filter highlights', function ( assert ) { -- 2.20.1