Skip to content

Commit

Permalink
Fix tree node check states not being updated
Browse files Browse the repository at this point in the history
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
4995e49, which optimized reactivity by
removing deep state tracking.
  • Loading branch information
undergroundwires committed Nov 6, 2023
1 parent 8ccaec7 commit 20c2873
Show file tree
Hide file tree
Showing 3 changed files with 222 additions and 32 deletions.
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -25,11 +25,21 @@ function useSelectedScripts() {

const selectedScripts = shallowRef<readonly SelectedScript[]>([]);

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 });
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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([]));
Expand All @@ -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<IScript, string>([
[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<IScript, string>([
[selectedScripts[0].script, 'expected-id-1'],
[selectedScripts[1].script, 'expected-id-2'],
]);
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<IScript, string>([
[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<IScript, string>([
[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<IScript, string>): 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<typeof useSelectedScriptNodeIds>;

Expand All @@ -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(),
},
},
});
Expand Down
11 changes: 7 additions & 4 deletions tests/unit/shared/Stubs/UserSelectionStub.ts
Original file line number Diff line number Diff line change
@@ -1,15 +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';
import { EventSourceStub } from './EventSourceStub';

export class UserSelectionStub
extends StubWithObservableMethodCalls<IUserSelection>
implements IUserSelection {
public readonly changed: IEventSource<readonly SelectedScript[]> = new EventSource<
readonly SelectedScript[]>();
public readonly changed = new EventSourceStub<readonly SelectedScript[]>();

public selectedScripts: readonly SelectedScript[] = [];

Expand All @@ -22,6 +20,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.');
}
Expand Down

0 comments on commit 20c2873

Please sign in to comment.