From 3b4514897fa593098e070cffd5f8ecdcfc0b7d4f Mon Sep 17 00:00:00 2001 From: undergroundwires Date: Tue, 3 Oct 2023 14:26:59 +0200 Subject: [PATCH] Fix tree node check states not being updated This commit fixes a bug where the check states of tree nodes were not correctly updated upon selecting predefined groups like "Standard", "Strict", "None" and "All". It resolves the issue by manually triggering of updates for mutated array containing selected scripts. It enhances test coverage to prevent regression and verify the correct behavior of state updates. This bug was introduced in commit 4995e49c469211404dac9fcb79b75eb121f80bce, which optimized reactivity by removing deep state tracking. the reactivity optimization reduced the deep state tracking. --- .../UseSelectedScriptNodeIds.ts | 16 +- .../UseSelectedScriptNodeIds.spec.ts | 227 ++++++++++++++++-- tests/unit/shared/Stubs/UserSelectionStub.ts | 8 +- 3 files changed, 221 insertions(+), 30 deletions(-) diff --git a/src/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds.ts b/src/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds.ts index 94eea8451..e4cc7d376 100644 --- a/src/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds.ts +++ b/src/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds.ts @@ -1,5 +1,5 @@ import { - computed, inject, shallowReadonly, shallowRef, + computed, inject, shallowReadonly, shallowRef, triggerRef, } from 'vue'; import { InjectionKeys } from '@/presentation/injectionSymbols'; import { SelectedScript } from '@/application/Context/State/Selection/SelectedScript'; @@ -25,11 +25,21 @@ function useSelectedScripts() { const selectedScripts = shallowRef([]); + function updateSelectedScripts(newReference: readonly SelectedScript[]) { + if (selectedScripts.value === newReference) { + // Manually trigger update if the array was mutated using the same reference. + // Array might have been mutated without changing the reference + triggerRef(selectedScripts); + } else { + selectedScripts.value = newReference; + } + } + onStateChange((state) => { - selectedScripts.value = state.selection.selectedScripts; + updateSelectedScripts(state.selection.selectedScripts); events.unsubscribeAllAndRegister([ state.selection.changed.on((scripts) => { - selectedScripts.value = scripts; + updateSelectedScripts(scripts); }), ]); }, { immediate: true }); diff --git a/tests/unit/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds.spec.ts b/tests/unit/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds.spec.ts index 4d45cbbe5..cfa52ce3b 100644 --- a/tests/unit/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds.spec.ts +++ b/tests/unit/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds.spec.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; import { shallowMount } from '@vue/test-utils'; +import { nextTick, watch } from 'vue'; import { useSelectedScriptNodeIds } from '@/presentation/components/Scripts/View/Tree/TreeViewAdapter/UseSelectedScriptNodeIds'; import { InjectionKeys } from '@/presentation/injectionSymbols'; import { UseAutoUnsubscribedEventsStub } from '@tests/unit/shared/Stubs/UseAutoUnsubscribedEventsStub'; @@ -8,9 +9,10 @@ import { CategoryCollectionStateStub } from '@tests/unit/shared/Stubs/CategoryCo import { SelectedScriptStub } from '@tests/unit/shared/Stubs/SelectedScriptStub'; import { getScriptNodeId } from '@/presentation/components/Scripts/View/Tree/TreeViewAdapter/CategoryNodeMetadataConverter'; import { IScript } from '@/domain/IScript'; +import { UserSelectionStub } from '@tests/unit/shared/Stubs/UserSelectionStub'; describe('useSelectedScriptNodeIds', () => { - it('returns empty array when no scripts are selected', () => { + it('returns an empty array when no scripts are selected', () => { // arrange const { useStateStub, returnObject } = mountWrapperComponent(); useStateStub.withState(new CategoryCollectionStateStub().withSelectedScripts([])); @@ -19,38 +21,213 @@ describe('useSelectedScriptNodeIds', () => { // assert expect(actualIds).to.have.lengthOf(0); }); - - it('returns correct node IDs for selected scripts', () => { + it('initially registers the unsubscribe callback', () => { // arrange - const selectedScripts = [ - new SelectedScriptStub('id-1'), - new SelectedScriptStub('id-2'), - ]; - const parsedNodeIds = new Map([ - [selectedScripts[0].script, 'expected-id-1'], - [selectedScripts[1].script, 'expected-id-1'], - ]); - const { useStateStub, returnObject } = mountWrapperComponent({ - scriptNodeIdParser: (script) => parsedNodeIds.get(script), - }); - useStateStub.triggerOnStateChange({ - newState: new CategoryCollectionStateStub().withSelectedScripts(selectedScripts), - immediateOnly: true, - }); + const eventsStub = new UseAutoUnsubscribedEventsStub(); // act - const actualIds = returnObject.selectedScriptNodeIds.value; + mountWrapperComponent({ + useAutoUnsubscribedEvents: eventsStub, + }); // assert - const expectedNodeIds = [...parsedNodeIds.values()]; - expect(actualIds).to.have.lengthOf(expectedNodeIds.length); - expect(actualIds).to.include.members(expectedNodeIds); + const calls = eventsStub.events.callHistory; + expect(eventsStub.events.callHistory).has.lengthOf(1); + const call = calls.find((c) => c.methodName === 'unsubscribeAllAndRegister'); + expect(call).toBeDefined(); + }); + describe('returns correct node IDs for selected scripts', () => { + it('immediately', () => { + // arrange + const selectedScripts = [ + new SelectedScriptStub('id-1'), + new SelectedScriptStub('id-2'), + ]; + const parsedNodeIds = new Map([ + [selectedScripts[0].script, 'expected-id-1'], + [selectedScripts[1].script, 'expected-id-1'], + ]); + const { useStateStub, returnObject } = mountWrapperComponent({ + scriptNodeIdParser: createNodeIdParserFromMap(parsedNodeIds), + }); + useStateStub.triggerOnStateChange({ + newState: new CategoryCollectionStateStub().withSelectedScripts(selectedScripts), + immediateOnly: true, + }); + // act + const actualIds = returnObject.selectedScriptNodeIds.value; + // assert + const expectedNodeIds = [...parsedNodeIds.values()]; + expect(actualIds).to.have.lengthOf(expectedNodeIds.length); + expect(actualIds).to.include.members(expectedNodeIds); + }); + it('when the collection state changes', () => { + // arrange + const initialScripts = []; + const changedScripts = [ + new SelectedScriptStub('id-1'), + new SelectedScriptStub('id-2'), + ]; + const parsedNodeIds = new Map([ + [changedScripts[0].script, 'expected-id-1'], + [changedScripts[1].script, 'expected-id-2'], + ]); + const { useStateStub, returnObject } = mountWrapperComponent({ + scriptNodeIdParser: createNodeIdParserFromMap(parsedNodeIds), + }); + useStateStub.triggerOnStateChange({ + newState: new CategoryCollectionStateStub().withSelectedScripts(initialScripts), + immediateOnly: true, + }); + // act + useStateStub.triggerOnStateChange({ + newState: new CategoryCollectionStateStub().withSelectedScripts(changedScripts), + immediateOnly: false, + }); + const actualIds = returnObject.selectedScriptNodeIds.value; + // assert + const expectedNodeIds = [...parsedNodeIds.values()]; + expect(actualIds).to.have.lengthOf(expectedNodeIds.length); + expect(actualIds).to.include.members(expectedNodeIds); + }); + it('when the selection state changes', () => { + // arrange + const initialScripts = []; + const changedScripts = [ + new SelectedScriptStub('id-1'), + new SelectedScriptStub('id-2'), + ]; + const parsedNodeIds = new Map([ + [changedScripts[0].script, 'expected-id-1'], + [changedScripts[1].script, 'expected-id-2'], + ]); + const { useStateStub, returnObject } = mountWrapperComponent({ + scriptNodeIdParser: createNodeIdParserFromMap(parsedNodeIds), + }); + const userSelection = new UserSelectionStub([]) + .withSelectedScripts(initialScripts); + useStateStub.triggerOnStateChange({ + newState: new CategoryCollectionStateStub() + .withSelection(userSelection), + immediateOnly: true, + }); + // act + userSelection.triggerSelectionChangedEvent(changedScripts); + const actualIds = returnObject.selectedScriptNodeIds.value; + // assert + const expectedNodeIds = [...parsedNodeIds.values()]; + expect(actualIds).to.have.lengthOf(expectedNodeIds.length); + expect(actualIds).to.include.members(expectedNodeIds); + }); + }); + describe('reactivity to state changes', () => { + describe('when the collection state changes', () => { + it('with new array references', async () => { + // arrange + const { useStateStub, returnObject } = mountWrapperComponent(); + let isChangeTriggered = false; + watch(() => returnObject.selectedScriptNodeIds.value, () => { + isChangeTriggered = true; + }); + // act + useStateStub.triggerOnStateChange({ + newState: new CategoryCollectionStateStub(), + immediateOnly: false, + }); + await nextTick(); + // assert + expect(isChangeTriggered).to.equal(true); + }); + it('with the same array reference', async () => { + // arrange + const sharedSelectedScriptsReference = []; + const initialCollectionState = new CategoryCollectionStateStub() + .withSelectedScripts(sharedSelectedScriptsReference); + const changedCollectionState = new CategoryCollectionStateStub() + .withSelectedScripts(sharedSelectedScriptsReference); + const { useStateStub, returnObject } = mountWrapperComponent(); + useStateStub.triggerOnStateChange({ + newState: initialCollectionState, + immediateOnly: true, + }); + let isChangeTriggered = false; + watch(() => returnObject.selectedScriptNodeIds.value, () => { + isChangeTriggered = true; + }); + // act + sharedSelectedScriptsReference.push(new SelectedScriptStub('new')); // mutate array using same reference + useStateStub.triggerOnStateChange({ + newState: changedCollectionState, + immediateOnly: false, + }); + await nextTick(); + // assert + expect(isChangeTriggered).to.equal(true); + }); + }); + describe('when the selection state changes', () => { + it('with new array references', async () => { + // arrange + const { useStateStub, returnObject } = mountWrapperComponent(); + const userSelection = new UserSelectionStub([]) + .withSelectedScripts([]); + useStateStub.triggerOnStateChange({ + newState: new CategoryCollectionStateStub() + .withSelection(userSelection), + immediateOnly: true, + }); + let isChangeTriggered = false; + watch(() => returnObject.selectedScriptNodeIds.value, () => { + isChangeTriggered = true; + }); + // act + userSelection.triggerSelectionChangedEvent([]); + await nextTick(); + // assert + expect(isChangeTriggered).to.equal(true); + }); + it('with the same array reference', async () => { + // arrange + const { useStateStub, returnObject } = mountWrapperComponent(); + const sharedSelectedScriptsReference = []; + const userSelection = new UserSelectionStub([]) + .withSelectedScripts(sharedSelectedScriptsReference); + useStateStub.triggerOnStateChange({ + newState: new CategoryCollectionStateStub() + .withSelection(userSelection), + immediateOnly: true, + }); + let isChangeTriggered = false; + watch(() => returnObject.selectedScriptNodeIds.value, () => { + isChangeTriggered = true; + }); + // act + sharedSelectedScriptsReference.push(new SelectedScriptStub('new')); // mutate array using same reference + userSelection.triggerSelectionChangedEvent(sharedSelectedScriptsReference); + await nextTick(); + // assert + expect(isChangeTriggered).to.equal(true); + }); + }); }); }); +type ScriptNodeIdParser = typeof getScriptNodeId; + +function createNodeIdParserFromMap(scriptToIdMap: Map): ScriptNodeIdParser { + return (script) => { + const expectedId = scriptToIdMap.get(script); + if (!expectedId) { + throw new Error(`No mapped ID for script: ${JSON.stringify(script)}`); + } + return expectedId; + }; +} + function mountWrapperComponent(scenario?: { - readonly scriptNodeIdParser?: typeof getScriptNodeId, + readonly scriptNodeIdParser?: ScriptNodeIdParser, + readonly useAutoUnsubscribedEvents?: UseAutoUnsubscribedEventsStub, }) { const useStateStub = new UseCollectionStateStub(); - const nodeIdParser: typeof getScriptNodeId = scenario?.scriptNodeIdParser + const nodeIdParser: ScriptNodeIdParser = scenario?.scriptNodeIdParser ?? ((script) => script.id); let returnObject: ReturnType; @@ -65,7 +242,7 @@ function mountWrapperComponent(scenario?: { [InjectionKeys.useCollectionState as symbol]: () => useStateStub.get(), [InjectionKeys.useAutoUnsubscribedEvents as symbol]: - () => new UseAutoUnsubscribedEventsStub().get(), + () => (scenario?.useAutoUnsubscribedEvents ?? new UseAutoUnsubscribedEventsStub()).get(), }, }, }); diff --git a/tests/unit/shared/Stubs/UserSelectionStub.ts b/tests/unit/shared/Stubs/UserSelectionStub.ts index 8e38057bf..2a64fa550 100644 --- a/tests/unit/shared/Stubs/UserSelectionStub.ts +++ b/tests/unit/shared/Stubs/UserSelectionStub.ts @@ -1,14 +1,13 @@ import { IUserSelection } from '@/application/Context/State/Selection/IUserSelection'; import { SelectedScript } from '@/application/Context/State/Selection/SelectedScript'; import { IScript } from '@/domain/IScript'; -import { IEventSource } from '@/infrastructure/Events/IEventSource'; import { EventSource } from '@/infrastructure/Events/EventSource'; import { StubWithObservableMethodCalls } from './StubWithObservableMethodCalls'; export class UserSelectionStub extends StubWithObservableMethodCalls implements IUserSelection { - public readonly changed: IEventSource = new EventSource< + public readonly changed: EventSource = new EventSource< readonly SelectedScript[]>(); public selectedScripts: readonly SelectedScript[] = []; @@ -22,6 +21,11 @@ export class UserSelectionStub return this; } + public triggerSelectionChangedEvent(scripts: readonly SelectedScript[]): this { + this.changed.notify(scripts); + return this; + } + public areAllSelected(): boolean { throw new Error('Method not implemented.'); }