From d8823a33591db5c5dc9a0af753e007167218a3e3 Mon Sep 17 00:00:00 2001 From: alex-krasn <64093224+alex-krasn@users.noreply.github.com> Date: Wed, 6 May 2020 14:18:26 -0700 Subject: [PATCH] fix: we're not allow to change tagtype if it is a checkbox (#224) * fix: we're not allowing change tagtype if it is a checkbox * refactoring: deletes console.log * refactoring: delets comment * + not allow to change tag type to "checkbox" * refactoring: deletes extra brackets * fix: let -> const * fix prev review issues + added warning toast * fix: failing pipeline test * refactoring * refactor: spelling "service" * refactor: switching to is empty(temporary), disabling options instead of removing, updates state on change * refactor: renaming fix test on pipelines * refactor: now with documentCount * style: deletes empty line * refactor: delete 'isEmpty' * style: fix variable to camelcase * refactor * refactor: switching tag types to enum * refactor: switch statement * Stew ro/check tag type and region catagory compatibility (#241) * refactor: check tag type and region catagory compatibility * refactor and spelling Co-authored-by: alex-krasn * feature: no more than one checkbox per tag * refactor: add compatibilty method and refactor * refactor * spelling * style: missing trailing comma * refactor: isCompatible Co-authored-by: stew-ro <60453211+stew-ro@users.noreply.github.com> Co-authored-by: kunzheng <58841788+kunzms@users.noreply.github.com> --- src/common/localization/en-us.ts | 2 + src/common/localization/es-cl.ts | 2 + src/common/mockFactory.ts | 6 +-- src/common/strings.ts | 2 + .../components/common/tagInput/tagInput.tsx | 51 +++++++++++++++---- src/services/projectService.test.ts | 26 +++++----- src/services/projectService.ts | 21 ++++---- 7 files changed, 75 insertions(+), 35 deletions(-) diff --git a/src/common/localization/en-us.ts b/src/common/localization/en-us.ts index 2d31b402a..a56c8202f 100644 --- a/src/common/localization/en-us.ts +++ b/src/common/localization/en-us.ts @@ -172,6 +172,8 @@ export const english: IAppStrings = { existingName: "Tag name already exists. Choose another name", emptyName: "Cannot have an empty tag name", unknownTagName: "Unknown", + notCompatibleTagType: "Tag type is not compatible with this feature", + checkboxPerTagLimit: "Cannot assign more than one checkbox per tag", }, toolbar: { add: "Add new tag", diff --git a/src/common/localization/es-cl.ts b/src/common/localization/es-cl.ts index 42cca1aeb..404c5d24d 100644 --- a/src/common/localization/es-cl.ts +++ b/src/common/localization/es-cl.ts @@ -173,6 +173,8 @@ export const spanish: IAppStrings = { existingName: "Nombre de etiqueta ya existe. Elige otro nombre", emptyName: "El nombre de etiqueta no puede ser vacío", unknownTagName: "Desconocido", + notCompatibleTagType: "El tipo de etiqueta no es compatible con esta función", + checkboxPerTagLimit: "No se puede asignar más de una casilla de verificación por etiqueta", }, toolbar: { add: "Agregar nueva etiqueta", diff --git a/src/common/mockFactory.ts b/src/common/mockFactory.ts index ae773c107..a2aaea342 100644 --- a/src/common/mockFactory.ts +++ b/src/common/mockFactory.ts @@ -400,7 +400,7 @@ export default class MockFactory { /** * Creates an array of test regions - * @param count The number of regions to create (deafult: 5) + * @param count The number of regions to create (default: 5) */ public static createTestRegions(count: number = 5) { const regions: IRegion[] = []; @@ -561,7 +561,7 @@ export default class MockFactory { } /** - * Runs and waits for a condidtion to be met and resolves a promise + * Runs and waits for a condition to be met and resolves a promise * @param predicate The predicate to evaluate the condition * @param interval The interval to check the value */ @@ -729,7 +729,7 @@ export default class MockFactory { /** * Gets StorageType for asset providers - * @param providerType Asset Providet type + * @param providerType Asset Provider type */ private static getStorageType(providerType: string): StorageType { switch (providerType) { diff --git a/src/common/strings.ts b/src/common/strings.ts index 16c8a91aa..920245ea8 100644 --- a/src/common/strings.ts +++ b/src/common/strings.ts @@ -186,6 +186,8 @@ export interface IAppStrings { existingName: string; emptyName: string; unknownTagName: string; + notCompatibleTagType: string; + checkboxPerTagLimit: string; } }; connections: { diff --git a/src/react/components/common/tagInput/tagInput.tsx b/src/react/components/common/tagInput/tagInput.tsx index f72b54713..75549c18a 100644 --- a/src/react/components/common/tagInput/tagInput.tsx +++ b/src/react/components/common/tagInput/tagInput.tsx @@ -463,19 +463,41 @@ export class TagInput extends React.Component { let deselect = selected && oldTagOperation === TagOperationMode.None; // Only fire click event if a region is selected - if (this.props.selectedRegions && - this.props.selectedRegions.length > 0 && - this.props.onTagClick) { - deselect = false; - this.props.onTagClick(tag); + const { selectedRegions, onTagClick, labels } = this.props; + if (selectedRegions && selectedRegions.length && onTagClick) { + const { category } = selectedRegions[0]; + const { format, type, documentCount, name } = tag; + const tagCategory = this.getTagCategory(type); + if (tagCategory === category || + (documentCount === 0 && type === FieldType.String && format === FieldFormat.NotSpecified)) { + if (category === "checkbox" && this.labelAssigned(labels, name)) { + toast.warn(strings.tags.warnings.checkboxPerTagLimit); + return; + } + onTagClick(tag); + deselect = false; + } else { + toast.warn(strings.tags.warnings.notCompatibleTagType); + } } - this.setState({ selectedTag: deselect ? null : tag, tagOperation, }); + } + } - } + private labelAssigned = (labels, name): boolean => { + return labels.find((label) => label.label === name ? true : false); + } + + private getTagCategory = (tagType: string) => { + switch (tagType) { + case FieldType.SelectionMark: + return "checkbox"; + default: + return "text"; + } } private onSearchKeyDown = (event: KeyboardEvent): void => { @@ -611,17 +633,28 @@ export class TagInput extends React.Component { return menuItems; } + private isTypeCompatibleWithTag = (tag, type) => { + // If free tag we can assign any type + if (tag && tag.documentCount <= 0) { + return true; + } + const tagType = this.getTagCategory(tag.type); + const menuItemType = this.getTagCategory(type); + return tagType === menuItemType; + } + private getTypeSubMenuItems = (): IContextualMenuItem[] => { const tag = this.state.selectedTag; const types = Object.values(FieldType); - return types.map((type) => { + const isCompatible = this.isTypeCompatibleWithTag(tag, type); return { key: type, text: type, - canCheck: true, + canCheck: isCompatible, isChecked: type === tag.type, onClick: this.onTypeSelect, + disabled: !isCompatible, } as IContextualMenuItem; }); } diff --git a/src/services/projectService.test.ts b/src/services/projectService.test.ts index 0ffaba53a..60caa7355 100644 --- a/src/services/projectService.test.ts +++ b/src/services/projectService.test.ts @@ -11,7 +11,7 @@ import { generateKey } from "../common/crypto"; import { encryptProject, decryptProject } from "../common/utils"; describe("Project Service", () => { - let projectSerivce: IProjectService = null; + let projectService: IProjectService = null; let testProject: IProject = null; let projectList: IProject[] = null; let securityToken: ISecurityToken = null; @@ -29,7 +29,7 @@ describe("Project Service", () => { key: generateKey(), }; testProject = MockFactory.createTestProject("TestProject"); - projectSerivce = new ProjectService(); + projectService = new ProjectService(); storageProviderMock.writeText.mockClear(); storageProviderMock.deleteFile.mockClear(); @@ -37,13 +37,13 @@ describe("Project Service", () => { it("Load decrypts any project settings using the specified key", async () => { const encryptedProject = await encryptProject(testProject, securityToken); - const decryptedProject = await projectSerivce.load(encryptedProject, securityToken); + const decryptedProject = await projectService.load(encryptedProject, securityToken); expect(decryptedProject).toEqual(testProject); }); it("Saves calls project storage provider to write project", async () => { - const result = await projectSerivce.save(testProject, securityToken); + const result = await projectService.save(testProject, securityToken); const encryptedProject: IProject = { ...testProject, @@ -69,7 +69,7 @@ describe("Project Service", () => { it("initializes tags to empty array if not defined", async () => { testProject.tags = null; - const result = await projectSerivce.save(testProject, securityToken); + const result = await projectService.save(testProject, securityToken); expect(result.tags).toEqual([]); }); @@ -77,7 +77,7 @@ describe("Project Service", () => { it("Save throws error if writing to storage provider fails", async () => { const expectedError = "Error writing to storage provider"; storageProviderMock.writeText.mockImplementationOnce(() => Promise.reject(expectedError)); - await expect(projectSerivce.save(testProject, securityToken)).rejects.toEqual(expectedError); + await expect(projectService.save(testProject, securityToken)).rejects.toEqual(expectedError); }); it("Save throws error if storage provider cannot be created", async () => { @@ -85,11 +85,11 @@ describe("Project Service", () => { const createMock = StorageProviderFactory.create as jest.Mock; createMock.mockImplementationOnce(() => { throw expectedError; }); - await expect(projectSerivce.save(testProject, securityToken)).rejects.toEqual(expectedError); + await expect(projectService.save(testProject, securityToken)).rejects.toEqual(expectedError); }); it("Delete calls project storage provider to delete project", async () => { - await projectSerivce.delete(testProject); + await projectService.delete(testProject); expect(StorageProviderFactory.create).toBeCalledWith( testProject.sourceConnection.providerType, @@ -104,7 +104,7 @@ describe("Project Service", () => { storageProviderMock.deleteFile .mockImplementationOnce(() => Promise.reject(expectedError)); - await expect(projectSerivce.delete(testProject)).rejects.toEqual(expectedError); + await expect(projectService.delete(testProject)).rejects.toEqual(expectedError); }); it("Delete call fails if storage provider cannot be created", async () => { @@ -112,20 +112,20 @@ describe("Project Service", () => { const createMock = StorageProviderFactory.create as jest.Mock; createMock.mockImplementationOnce(() => { throw expectedError; }); - await expect(projectSerivce.delete(testProject)).rejects.toEqual(expectedError); + await expect(projectService.delete(testProject)).rejects.toEqual(expectedError); }); it("isDuplicate returns false when called with a unique project", async () => { testProject = MockFactory.createTestProject("TestProject"); projectList = MockFactory.createTestProjects(); - expect(projectSerivce.isDuplicate(testProject, projectList)).toEqual(false); + expect(projectService.isDuplicate(testProject, projectList)).toEqual(false); }); it("isDuplicate returns true when called with a duplicate project", async () => { testProject = MockFactory.createTestProject("1"); testProject.id = undefined; projectList = MockFactory.createTestProjects(); - expect(projectSerivce.isDuplicate(testProject, projectList)).toEqual(true); + expect(projectService.isDuplicate(testProject, projectList)).toEqual(true); }); it("deletes all asset metadata files when project is deleted", async () => { @@ -136,7 +136,7 @@ describe("Project Service", () => { testProject.assets = _.keyBy(assets, (asset) => asset.id); - await projectSerivce.delete(testProject); + await projectService.delete(testProject); expect(storageProviderMock.deleteFile.mock.calls).toHaveLength(assets.length + 1); }); }); diff --git a/src/services/projectService.ts b/src/services/projectService.ts index a8613db9c..5780d635c 100644 --- a/src/services/projectService.ts +++ b/src/services/projectService.ts @@ -211,7 +211,7 @@ export default class ProjectService implements IProjectService { asset?: string) { const tags: ITag[] = []; const tagNameSet = new Set(); - const tagdocumentCount = {}; + const tagDocumentCount = {}; try { const blobs = new Set(await storageProvider.listFiles(project.folderPath)); const assetLabel = asset ? asset + constants.labelFileExtension : undefined; @@ -225,10 +225,10 @@ export default class ProjectService implements IProjectService { const content = JSON.parse(await storageProvider.readText(blob)); content.labels.forEach((label) => { tagNameSet.add(label.label); - if (tagdocumentCount[label.label]) { - tagdocumentCount[label.label] += 1; + if (tagDocumentCount[label.label]) { + tagDocumentCount[label.label] += 1; } else { - tagdocumentCount[label.label] = 1; + tagDocumentCount[label.label] = 1; } }); } @@ -255,11 +255,11 @@ export default class ProjectService implements IProjectService { // use default type type: FieldType.String, format: FieldFormat.NotSpecified, - documentCount: tagdocumentCount[name], + documentCount: tagDocumentCount[name], } as ITag); }); if (project.tags) { - await this.addMissingTagsAndUpdatedocumentCount(project, tags, tagdocumentCount); + await this.addMissingTagsAndUpdateDocumentCount(project, tags, tagDocumentCount); } else { project.tags = tags; } @@ -293,7 +293,7 @@ export default class ProjectService implements IProjectService { }); if (project.tags) { project.tags = patch(project.tags, tags, "name", ["type", "format"]); - await this.addMissingTagsAndUpdatedocumentCount(project, tags); + await this.addMissingTagsAndUpdateDocumentCount(project, tags); } else { project.tags = tags; } @@ -328,14 +328,14 @@ export default class ProjectService implements IProjectService { updatedProject.tags = existingTags; } - private async addMissingTagsAndUpdatedocumentCount(project: IProject, tags: ITag[], tagdocumentCount?: any) { + private async addMissingTagsAndUpdateDocumentCount(project: IProject, tags: ITag[], tagDocumentCount?: any) { const missingTags = tags.filter((fileTag) => { const foundExistingTag = project.tags.find((tag) => fileTag.name === tag.name ); if (!foundExistingTag) { return true; } else { - if (tagdocumentCount) { - foundExistingTag.documentCount = tagdocumentCount[foundExistingTag.name]; + if (tagDocumentCount) { + foundExistingTag.documentCount = tagDocumentCount[foundExistingTag.name]; } return false; } @@ -343,6 +343,7 @@ export default class ProjectService implements IProjectService { project.tags = [...project.tags, ...missingTags]; } + // private async getAllTagsInProjectCount(project: IProject, tags: ITag[]) {} /** * Save fields.json * @param project the project we're trying to create