Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 'select' method to allow deselecting selection #129

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 6.1.2
* Fix 'select' method to allow deselecting selection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix 'select' method in MockISelectionManager to allow deselecting selection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* Check if 'callback' is defined in MockISelectionManager
* Add tests for MockISelectionManager

## 6.1.1
* powerbi-visuals-api update to 5.9.0
Expand Down
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "powerbi-visuals-utils-testutils",
"version": "6.1.1",
"version": "6.1.2",
"description": "powerbi-visuals-utils-testutils",
"main": "lib/index.js",
"module": "lib/index.js",
Expand Down
59 changes: 41 additions & 18 deletions src/mocks/mockISelectionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,25 +55,46 @@ export class MockISelectionManager implements ISelectionManager {
}

public select(selectionId: ISelectionId | ISelectionId[], multiSelect?: boolean): IPromise<ISelectionId[]> {
const selectionIds: ISelectionId[] = [].concat(selectionId);
const selectionIds: ISelectionId[] = Array.isArray(selectionId) ? selectionId : [selectionId];

// if no multiselect reset current selection and save new passed selections;
if (!multiSelect) {
this.selectionIds = selectionIds;
if (selectionIds.length < 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selectionIds.length === 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return new Promise((resolve, reject) => {
resolve(this.selectionIds);
}) as any;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a lot of If else blocks and all of them have the same this.selectionIds = selectionIds

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is right, you can make it much easier:

if(multiSelect){
...all your ifs here
} else {
   this.selectionIds = selectionIds
}

Copy link
Contributor Author

@adiletelf adiletelf Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored the code to make it more readable.

if (selectionIds.length > 1) {
// the new selection is a set of points
if (multiSelect) {
// if multiSelect is truthy, toggle the selection state of each selectionId
selectionIds.forEach(id => {
const index = this.selectionIds.findIndex(selectedId => selectedId.equals(id));
if (index > -1) {
this.selectionIds.splice(index, 1);
} else {
this.selectionIds.push(id);
}
});
} else {
// if an array of selectionIds are passed in, assume multiSelect and set the selection to be the new set that is selected
this.selectionIds = selectionIds;
}
} else if (this.containsSelection(selectionIds[0])) {
// the selectionId that was selected is a subset of what is already selected
if (multiSelect) {
// if multiSelect is on, deselect the selected id
this.selectionIds = this.selectionIds.filter(x => !selectionIds[0].equals(x));
} else {
// if multiSelect is off, the selected item is the new selectedId, else deselect the selection
this.selectionIds = selectionIds.length > 1 ? selectionIds : [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here would be always an empty array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it's the expected behavior too. Fixed.

}
} else {
// if multiselect then check all passed selections
selectionIds.forEach( (id: ISelectionId) => {
// if selectionManager has passed selection in list of current selections
if (this.containsSelection(id)) {
// need to exclude from selection (selection of selected element should deselect element)
this.selectionIds = this.selectionIds.filter((selectedId: ISelectionId) => {
return !selectedId.equals(id);
});
} else {
// otherwise include the new selection into current selections
this.selectionIds.push(id);
}
});
// the selectionId that was selected is not a subset of what is already selected
if (multiSelect) {
this.selectionIds.push(selectionIds[0]);
} else {
this.selectionIds = selectionIds;
}
}

return new Promise((resolve, reject) => {
Expand Down Expand Up @@ -112,6 +133,8 @@ export class MockISelectionManager implements ISelectionManager {
}

public simutateSelection(selections: ISelectionId[]): void {
this.callback(selections);
if (this.callback && typeof this.callback === "function") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type guard is enough here, it also checks for undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

this.callback(selections);
}
}
}
250 changes: 250 additions & 0 deletions test/mocks/mockISelectionManagerTest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,250 @@
/*
* Power BI Visualizations
*
* Copyright (c) Microsoft Corporation
* All rights reserved.
* MIT License
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the ""Software""), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

import powerbi from "powerbi-visuals-api";
import { createSelectionId } from "../../src/mocks/mocks";
import { MockISelectionManager } from "../../src/mocks/mockISelectionManager";

import ISelectionId = powerbi.visuals.ISelectionId;

describe("MockISelectionManager", () => {
let selectionManager: MockISelectionManager;

beforeEach(() => {
selectionManager = new MockISelectionManager();
});

describe("clear", () => {
it("should have an empty selection", () => {
expect(selectionManager.hasSelection()).toBeFalsy();
});

it("should clear the selection", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);
selectionManager.clear();

expect(selectionManager.getSelectionIds()).toHaveSize(0);
expect(selectionManager.hasSelection()).toBeFalsy();
})
});

describe("hasSelection", () => {
it("should return false when there is no selection", () => {
expect(selectionManager.hasSelection()).toBeFalsy();
});

it("should return true when there is a selection", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);

expect(selectionManager.hasSelection()).toBeTruthy();
});

it("should return false when the selection is cleared", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);
selectionManager.clear();

expect(selectionManager.hasSelection()).toBeFalsy();
});

it("should return false when selection is toggled", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);
selectionManager.select(selectionId);

expect(selectionManager.hasSelection()).toBeFalsy();
});
});

describe("getSelectionIds", () => {
it("should return an empty array when there is no selection", () => {
expect(selectionManager.getSelectionIds()).toHaveSize(0);
});

it("should return an array with a selection", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(1);
});
});

describe("containsSelection", () => {
it("should return false when there is no selection", () => {
const selectionId: ISelectionId = createSelectionId("1");

expect(selectionManager.containsSelection(selectionId)).toBeFalsy();
});

it("should return true when the selection is present", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);

expect(selectionManager.containsSelection(selectionId)).toBeTruthy();
});

it("should return false when the selection is not present", () => {
const selectionId: ISelectionId = createSelectionId("1");
const otherSelectionId: ISelectionId = createSelectionId("2");

selectionManager.select(selectionId);

expect(selectionManager.containsSelection(otherSelectionId)).toBeFalsy();
});
});

describe("select", () => {
it("should add a selection", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(1);
expect(selectedIds[0].equals(selectionId)).toBeTruthy();
});

it("should toggle a selection", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);
selectionManager.select(selectionId);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(0);
});

it("should replace a selection", () => {
const selectionId0: ISelectionId = createSelectionId("0");
const selectionId1: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId0);
selectionManager.select(selectionId1);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(1);
expect(selectedIds[0].equals(selectionId1)).toBeTruthy();
});

it("should not update the selection when selection array is empty", () => {
const selectionId: ISelectionId = createSelectionId("1");

selectionManager.select(selectionId);
selectionManager.select([]);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(1);
expect(selectedIds[0].equals(selectionId)).toBeTruthy();
})

it("should add multiple selections at once", () => {
const selectionId0: ISelectionId = createSelectionId("0");
const selectionId1: ISelectionId = createSelectionId("1");
const selectionId2: ISelectionId = createSelectionId("2");
Comment on lines +173 to +175
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create a variable with these selectionIds and then compare it with the result using toEqual

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


selectionManager.select([selectionId0, selectionId1, selectionId2]);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(3);
expect(selectedIds[0].equals(selectionId0)).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(selectedIds[0]).toEqual(selectionId0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

expect(selectedIds[1].equals(selectionId1)).toBeTruthy();
expect(selectedIds[2].equals(selectionId2)).toBeTruthy();
});

it("should add multiple selections with multiSelection", () => {
const selectionId0: ISelectionId = createSelectionId("0");
const selectionId1: ISelectionId = createSelectionId("1");
const selectionId2: ISelectionId = createSelectionId("2");

selectionManager.select(selectionId0);
selectionManager.select(selectionId1, true);
selectionManager.select(selectionId2, true);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(3);
expect(selectedIds[0].equals(selectionId0)).toBeTruthy();
expect(selectedIds[1].equals(selectionId1)).toBeTruthy();
expect(selectedIds[2].equals(selectionId2)).toBeTruthy();
});

it("should toggle a selection with multiSelection", () => {
const selectionId0: ISelectionId = createSelectionId("0");
const selectionId1: ISelectionId = createSelectionId("1");
const selectionId2: ISelectionId = createSelectionId("2");

selectionManager.select([selectionId0, selectionId1, selectionId2]);
selectionManager.select(selectionId2, true);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(2);
expect(selectedIds[0].equals(selectionId0)).toBeTruthy();
expect(selectedIds[1].equals(selectionId1)).toBeTruthy();
});

it("should replace a selection with passed array of selectionIds", () => {
const selectionId0: ISelectionId = createSelectionId("0");
const selectionId1: ISelectionId = createSelectionId("1");
const selectionId2: ISelectionId = createSelectionId("2");
const selectionId3: ISelectionId = createSelectionId("3");

selectionManager.select([selectionId0, selectionId1]);
selectionManager.select([selectionId2, selectionId3]);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(2);
expect(selectedIds[0].equals(selectionId0)).toBeFalsy();
expect(selectedIds[1].equals(selectionId1)).toBeFalsy();
expect(selectedIds[0].equals(selectionId2)).toBeTruthy();
expect(selectedIds[1].equals(selectionId3)).toBeTruthy();
});

it("should merge a selection with passed array of selectionIds", () => {
const selectionId0: ISelectionId = createSelectionId("0");
const selectionId1: ISelectionId = createSelectionId("1");
const selectionId2: ISelectionId = createSelectionId("2");
const selectionId3: ISelectionId = createSelectionId("3");

selectionManager.select([selectionId0, selectionId1]);
selectionManager.select([selectionId2, selectionId3], true);

const selectedIds: ISelectionId[] = selectionManager.getSelectionIds();
expect(selectedIds).toHaveSize(4);
expect(selectedIds[0].equals(selectionId0)).toBeTruthy();
expect(selectedIds[1].equals(selectionId1)).toBeTruthy();
expect(selectedIds[2].equals(selectionId2)).toBeTruthy();
expect(selectedIds[3].equals(selectionId3)).toBeTruthy();
});
});
});
Loading