From 3b303e50a957cd58597c1439cf653a0bdbcb9e1b Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Fri, 9 Feb 2018 11:43:59 -0500 Subject: [PATCH] Cmatch: Fix combined show/hide of same field, multi-index This is something that managed to slip by (but not unnoticed) for almost exactly one year to this day (028606242a2f8ebbd558cf4752309054fdee427e). It can only be reproduced by changing classes that result in visibility changes differing on the same field by index. The issue hides itself on first load (because all fields are shown by default) and on refresh. The problem is that, when one index shows a field but another hides it, the hide overrode the show indexes, so only the hide took place. * src/client/Cmatch.js (markShowHide): Make virtual. New implementation to support concurrent show/hide. (_handleClassMatch): Use it. * test/client/CmatchTest.js: New test. * npm-shrinkwrap.json: ease.js v0.2.{8=>9}. --- npm-shrinkwrap.json | 2 +- src/client/Cmatch.js | 32 ++++++++------ test/client/CmatchTest.js | 87 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 13 deletions(-) create mode 100644 test/client/CmatchTest.js diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 6b583ba..b93cc56 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -38,7 +38,7 @@ "version": "3.2.0" }, "easejs": { - "version": "0.2.8" + "version": "0.2.9" }, "escape-string-regexp": { "version": "1.0.5" diff --git a/src/client/Cmatch.js b/src/client/Cmatch.js index 62913c6..4705e0c 100644 --- a/src/client/Cmatch.js +++ b/src/client/Cmatch.js @@ -238,7 +238,7 @@ module.exports = Class( 'Cmatch', .getExclusiveFieldNames(); - var visq = []; + var visq = {}; for ( var field in cmatch ) { // ignore fields that are not on the current step @@ -316,11 +316,16 @@ module.exports = Class( 'Cmatch', { Object.keys( visq ).forEach( field => { - const { event_id, name, indexes } = visq[ field ]; + const field_vis = visq[ field ]; - this._client.handleEvent( event_id, { - elementName: name, - indexes: indexes, + Object.keys( field_vis ).forEach( event_id => + { + const indexes = field_vis[ event_id ]; + + this._client.handleEvent( event_id, { + elementName: field, + indexes: indexes, + } ); } ); this._dapiTrigger( name ); @@ -334,33 +339,36 @@ module.exports = Class( 'Cmatch', * * This also updates the cached visibility of field FIELD. * - * XXX: This method makes very obvious a nasty bug where hides override - * shows if both are set. - * * @param {string} field field name * @param {Array} show indexes to show * @param {Array} hide indexes to hide * * @return {undefined} */ - 'protected markShowHide'( field, visq, show, hide ) + 'virtual protected markShowHide'( field, visq, show, hide ) { if ( !( show.length || hide.length ) ) { - return; + return visq; } + const { [field]: result = {} } = visq; + if ( show.length ) { this._mergeCmatchHidden( field, show, false ); - visq[ field ] = { event_id: 'show', name: field, indexes: show }; + result.show = show; } if ( hide.length ) { this._mergeCmatchHidden( field, hide, true ); - visq[ field ] = { event_id: 'hide', name: field, indexes: hide }; + result.hide = hide; } + + visq[ field ] = result; + + return visq; }, diff --git a/test/client/CmatchTest.js b/test/client/CmatchTest.js new file mode 100644 index 0000000..cfbfb14 --- /dev/null +++ b/test/client/CmatchTest.js @@ -0,0 +1,87 @@ +/** + * Test case for Cmatch + * + * Copyright (C) 2018 R-T Specialty, LLC. + * + * This file is part of the Liza Data Collection Framework + * + * Liza is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +const { event } = require( '../../' ).client; +const { expect } = require( 'chai' ); + +const Sut = require( '../../src/client/Cmatch' ) + .extend( +{ + 'override constructor'( _, __, ___ ) {}, + + // make public + 'override public markShowHide'( field, visq, show, hide ) + { + return this.__super( field, visq, show, hide ); + } +} ); + + +// these tests aren't terribly effective right now +describe( "Cmatch", () => +{ + it( "marks hidden fields on class change to show", () => + { + expect( + Sut().markShowHide( 'foo', {}, [ 1, 2 ], [] ) + ).to.deep.equal( { foo: { show: [ 1, 2 ] } } ); + } ); + + + it( "marks shown fields on class change to hide", () => + { + expect( + Sut().markShowHide( 'foo', {}, [], [ 3, 4, 5 ] ) + ).to.deep.equal( { foo: { hide: [ 3, 4, 5 ] } } ); + } ); + + + it( "marks combination show/hide on class change", () => + { + expect( + Sut().markShowHide( 'foo', {}, [ 2, 3 ], [ 4, 5, 6 ] ) + ).to.deep.equal( { + foo: { + show: [ 2, 3 ], + hide: [ 4, 5, 6 ], + } + } ); + } ); + + + it( "marks no fields with no show or hide", () => + { + expect( + Sut().markShowHide( 'foo', {}, [], [] ) + ).to.deep.equal( {} ); + } ); + + + it( "does not affect marking of other fields", () => + { + const barval = {}; + const visq = { bar: barval }; + + Sut().markShowHide( 'foo', {}, [ 1 ], [ 0 ] ); + + expect( visq.bar ).to.equal( barval ); + } ); +} );