From a90f804f4c79212f91ab2a132c4723ad4c3ed966 Mon Sep 17 00:00:00 2001 From: Kevin Date: Wed, 17 May 2023 17:16:39 +0200 Subject: [PATCH] fix(Features): Tweaks and fixes for the udpateFeatureName Some smaller fixes for the updateFeatureName feature, as well as a new Error check for name uniqueness on the given feature-project id pair --- .../dto/update-feature-name.dto.ts | 3 +- .../geo-features/geo-features.service.ts | 16 +++++++- .../modules/projects/projects.controller.ts | 5 +++ .../update-project-feature-name.e2e-spec.ts | 39 +++++++++++++++---- 4 files changed, 53 insertions(+), 10 deletions(-) diff --git a/api/apps/api/src/modules/geo-features/dto/update-feature-name.dto.ts b/api/apps/api/src/modules/geo-features/dto/update-feature-name.dto.ts index bbd0725a2e..08d5c6dfdf 100644 --- a/api/apps/api/src/modules/geo-features/dto/update-feature-name.dto.ts +++ b/api/apps/api/src/modules/geo-features/dto/update-feature-name.dto.ts @@ -1,8 +1,9 @@ import { ApiProperty } from '@nestjs/swagger'; -import { IsString } from 'class-validator'; +import { IsNotEmpty, IsString } from 'class-validator'; export class UpdateFeatureNameDto { @ApiProperty() @IsString() + @IsNotEmpty() featureClassName!: string; } diff --git a/api/apps/api/src/modules/geo-features/geo-features.service.ts b/api/apps/api/src/modules/geo-features/geo-features.service.ts index 8827dd431c..c763615927 100644 --- a/api/apps/api/src/modules/geo-features/geo-features.service.ts +++ b/api/apps/api/src/modules/geo-features/geo-features.service.ts @@ -50,6 +50,7 @@ type GeoFeatureFilters = Record; export const featureNotFound = Symbol('feature not found'); export const featureNotEditable = Symbol('feature cannot be edited'); +export const featureNameAlreadyInUse = Symbol('feature name already in use'); export type FindResult = { data: (Partial | undefined)[]; @@ -403,8 +404,9 @@ export class GeoFeaturesService extends AppBaseService< Either< | typeof featureNotFound | typeof featureNotEditable - | typeof projectNotFound, - any + | typeof projectNotFound + | typeof featureNameAlreadyInUse, + true > > { const project = await this.projectRepository.findOne({ @@ -429,6 +431,16 @@ export class GeoFeaturesService extends AppBaseService< return left(featureNotEditable); } + const projectsWithSameName = await this.geoFeaturesRepository.count({ + where: { + featureClassName: updateFeatureNameDto.featureClassName, + projectId, + }, + }); + if (projectsWithSameName > 0) { + return left(featureNameAlreadyInUse); + } + await this.geoFeaturesRepository.update(featureId, { featureClassName: updateFeatureNameDto.featureClassName, }); diff --git a/api/apps/api/src/modules/projects/projects.controller.ts b/api/apps/api/src/modules/projects/projects.controller.ts index bfb5db1f5f..bc7abc09b2 100644 --- a/api/apps/api/src/modules/projects/projects.controller.ts +++ b/api/apps/api/src/modules/projects/projects.controller.ts @@ -140,6 +140,7 @@ import { updateSolutionsAreLockFailed } from '../legacy-project-import/applicati import { blmCreationFailure } from '../scenarios/blm-calibration/create-initial-scenario-blm.command'; import { UpdateFeatureNameDto } from '@marxan-api/modules/geo-features/dto/update-feature-name.dto'; import { + featureNameAlreadyInUse, featureNotEditable, featureNotFound, } from '@marxan-api/modules/geo-features/geo-features.service'; @@ -784,6 +785,10 @@ export class ProjectsController { throw new ForbiddenException( `Feature with id ${featureId}, for project with id ${projectId}, not editable`, ); + case featureNameAlreadyInUse: + throw new ForbiddenException( + `Feature with id ${featureId}, for project with id ${projectId}, cannot be updated: name is already in use`, + ); } } diff --git a/api/apps/api/test/project/update-project-feature-name.e2e-spec.ts b/api/apps/api/test/project/update-project-feature-name.e2e-spec.ts index e324bd9093..d334a253f6 100644 --- a/api/apps/api/test/project/update-project-feature-name.e2e-spec.ts +++ b/api/apps/api/test/project/update-project-feature-name.e2e-spec.ts @@ -59,9 +59,9 @@ describe('Project - update feature Name', () => { ); await fixtures.ThenUpdateWasForbidden( result, - projectId, featureId, originalName, + `Feature with id ${featureId}, for project with id ${projectId}, not editable`, ); }); @@ -76,9 +76,9 @@ describe('Project - update feature Name', () => { ); await fixtures.ThenUpdateWasForbidden( result, - projectId, featureId, originalName, + `Feature with id ${featureId}, for project with id ${projectId}, not editable`, ); }); @@ -96,9 +96,29 @@ describe('Project - update feature Name', () => { ); await fixtures.ThenUpdateWasForbidden( result, - viewOnlyProject, featureId, originalName, + `Feature with id ${featureId}, for project with id ${viewOnlyProject}, not editable`, + ); + }); + + test('should not permit updating a given feature, when another feature with the same name already exists for the same project', async () => { + const originalName = 'originalFeatureName'; + const sameName = 'sameName'; + const projectId = fixtures.projectId; + const featureId = await fixtures.GivenBaseFeature(originalName, projectId); + await fixtures.GivenBaseFeature(sameName, projectId); + + const result = await fixtures.WhenUpdatingFeatureForProject( + projectId, + featureId, + sameName, + ); + await fixtures.ThenUpdateWasForbidden( + result, + featureId, + originalName, + `Feature with id ${featureId}, for project with id ${projectId}, cannot be updated: name is already in use`, ); }); @@ -171,6 +191,13 @@ const getFixtures = async () => { projectId: project.projectId, anotherProjectId: anotherProject.projectId, cleanup: async () => { + //Restore the owner role to anotherProject to avoid errors when cleaning up + await userProjectsRepo.save({ + projectId: anotherProject.projectId, + userId: userId, + roleName: ProjectRoles.project_owner, + }); + await geoFeaturesApiRepo.clear(); await project.cleanup(); await anotherProject.cleanup(); @@ -228,15 +255,13 @@ const getFixtures = async () => { }, ThenUpdateWasForbidden: async ( response: request.Response, - projectId: string, featureId: string, originalName: string, + errorMessage: string, ) => { expect(response.status).toEqual(403); const error: any = response.body.errors[0]; - expect(error.title).toEqual( - `Feature with id ${featureId}, for project with id ${projectId}, not editable`, - ); + expect(error.title).toEqual(errorMessage); const features = await geoFeaturesApiRepo.findOne({ where: {