From 5e936846ba37348c59ae03d214dc42c46ab43b40 Mon Sep 17 00:00:00 2001 From: andrea rota Date: Thu, 12 Oct 2023 14:40:03 +0100 Subject: [PATCH 1/3] add test: hide projects with malformed grid data Both when listing projects and when trying to get such projects by id. I think we can ignore the case of published projects, as users should not even get to being able to publish projects with malformed grid data in any case. --- .../projects/crud/project-get.e2e-spec.ts | 12 +++++++ .../projects/crud/update-project.e2e-spec.ts | 2 +- .../projects/crud/update-project.fixtures.ts | 4 +++ .../api/test/projects/projects.fixtures.ts | 34 +++++++++++++++++++ 4 files changed, 51 insertions(+), 1 deletion(-) diff --git a/api/apps/api/test/projects/crud/project-get.e2e-spec.ts b/api/apps/api/test/projects/crud/project-get.e2e-spec.ts index ad30adb5db..31f5569446 100644 --- a/api/apps/api/test/projects/crud/project-get.e2e-spec.ts +++ b/api/apps/api/test/projects/crud/project-get.e2e-spec.ts @@ -32,3 +32,15 @@ test(`getting a project where the user is not in project`, async () => { ); fixtures.ThenForbiddenIsReturned(response); }); + +test(`if a project was created with malformed grid data and not GC'ed yet, it should not be included in project listings`, async () => { + const projectId = await fixtures.GivenPrivateProjectWithMalformedGridDataWasCreated(); + const response = await fixtures.WhenGettingUserProjects(); + fixtures.ThenProjectIsNotIncludedInProjectsList(projectId, response); +}); + +test(`if a project was created with malformed grid data and not GC'ed yet, a request to get its information should return 404`, async () => { + const projectId = await fixtures.GivenPrivateProjectWithMalformedGridDataWasCreated(); + const response = await fixtures.WhenGettingProject(projectId); + fixtures.ThenNotFoundIsReturned(response); +}); diff --git a/api/apps/api/test/projects/crud/update-project.e2e-spec.ts b/api/apps/api/test/projects/crud/update-project.e2e-spec.ts index 7869618246..5360012453 100644 --- a/api/apps/api/test/projects/crud/update-project.e2e-spec.ts +++ b/api/apps/api/test/projects/crud/update-project.e2e-spec.ts @@ -28,5 +28,5 @@ test(`updating a project does not work if user is not in project`, async () => { const request = await fixtures.WhenProjectIsUpdatedAsNotIncludedUser(); - await fixtures.ThenForbiddenIsReturned(request); + fixtures.ThenForbiddenIsReturned(request); }); diff --git a/api/apps/api/test/projects/crud/update-project.fixtures.ts b/api/apps/api/test/projects/crud/update-project.fixtures.ts index 2091300734..9fc7295290 100644 --- a/api/apps/api/test/projects/crud/update-project.fixtures.ts +++ b/api/apps/api/test/projects/crud/update-project.fixtures.ts @@ -7,6 +7,7 @@ import * as request from 'supertest'; import { ProjectChecker } from '@marxan-api/modules/projects/project-checker/project-checker.service'; import { ProjectCheckerFake } from '../../utils/project-checker.service-fake'; import { HttpStatus } from '@nestjs/common'; +import { PlanningUnitGridShape } from '@marxan/scenarios-planning-unit'; export const getFixtures = async () => { const app = await bootstrapApplication(); @@ -30,6 +31,9 @@ export const getFixtures = async () => { name: originalName, organizationId, metadata: {}, + countryId: 'RWA', + planningUnitGridShape: PlanningUnitGridShape.Hexagon, + planningUnitAreakm2: 10000, }) ).data.id; }, diff --git a/api/apps/api/test/projects/projects.fixtures.ts b/api/apps/api/test/projects/projects.fixtures.ts index 968f53ceed..b6a3a0fffd 100644 --- a/api/apps/api/test/projects/projects.fixtures.ts +++ b/api/apps/api/test/projects/projects.fixtures.ts @@ -25,6 +25,7 @@ import { bootstrapApplication } from '../utils/api-application'; import { EventBusTestUtils } from '../utils/event-bus.test.utils'; import { ScenariosTestUtils } from '../utils/scenarios.test.utils'; import { ScenarioType } from '@marxan-api/modules/scenarios/scenario.api.entity'; +import { Project } from '@marxan-api/modules/projects/project.api.entity'; export const getFixtures = async () => { const app = await bootstrapApplication([CqrsModule], [EventBusTestUtils]); @@ -35,6 +36,9 @@ export const getFixtures = async () => { const publishedProjectsRepo: Repository = app.get( getRepositoryToken(PublishedProject), ); + const projectsRepo: Repository = app.get( + getRepositoryToken(Project), + ); const cleanups: (() => Promise)[] = []; const apiEvents = app.get(ApiEventsService); @@ -100,6 +104,23 @@ export const getFixtures = async () => { cleanups.push(cleanup); return projectId; }, + GivenPrivateProjectWithMalformedGridDataWasCreated: async () => { + const { cleanup, projectId } = await GivenProjectExists( + app, + randomUserToken, + ); + cleanups.push(cleanup); + await projectsRepo.update( + { id: projectId }, + { + countryId: undefined, + adminAreaLevel1Id: undefined, + adminAreaLevel2Id: undefined, + planningAreaId: undefined, + }, + ); + return projectId; + }, GivenScenarioWasCreated: async (projectId: string, name?: string) => { const result = await ScenariosTestUtils.createScenario( app, @@ -336,10 +357,23 @@ export const getFixtures = async () => { ThenForbiddenIsReturned: (response: request.Response) => { expect(response.status).toEqual(403); }, + ThenProjectIsNotIncludedInProjectsList: ( + projectId: string, + response: request.Response, + ) => { + const allIdsOfProjectsInResponse = response.body.data.map( + (project: { id: string }) => project.id, + ); + expect(allIdsOfProjectsInResponse).not.toContain(projectId); + }, WhenGettingProject: async (projectId: string) => await request(app.getHttpServer()) .get(`/api/v1/projects/${projectId}`) .set('Authorization', `Bearer ${randomUserToken}`), + WhenGettingUserProjects: async () => + await request(app.getHttpServer()) + .get(`/api/v1/projects`) + .set('Authorization', `Bearer ${randomUserToken}`), WhenGettingProjectAsNotIncludedUser: async (projectId: string) => await request(app.getHttpServer()) .get(`/api/v1/projects/${projectId}`) From b8a4a7b403bf75c32e3d4d29af477d50096b15e2 Mon Sep 17 00:00:00 2001 From: andrea rota Date: Thu, 12 Oct 2023 14:58:56 +0100 Subject: [PATCH 2/3] hide projects with malformed grid data --- .../modules/projects/projects-crud.service.ts | 39 ++++++++++++++++--- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/api/apps/api/src/modules/projects/projects-crud.service.ts b/api/apps/api/src/modules/projects/projects-crud.service.ts index 8aa697b871..d9f6be4aec 100644 --- a/api/apps/api/src/modules/projects/projects-crud.service.ts +++ b/api/apps/api/src/modules/projects/projects-crud.service.ts @@ -28,7 +28,6 @@ import { ProjectId, SetProjectGridFromShapefile } from './planning-unit-grid'; import { ProjectRoles } from '@marxan-api/modules/access-control/projects-acl/dto/user-role-project.dto'; import { Roles } from '@marxan-api/modules/access-control/role.api.entity'; import { PlanningUnitGridShape } from '@marxan/scenarios-planning-unit'; -import { PublishedProject } from '../published-project/entities/published-project.api.entity'; import { CostSurfaceId } from '@marxan-api/modules/projects/planning-unit-grid/project.id'; import { getDefaultCostSurfaceIdFromProject } from '@marxan-api/modules/projects/get-default-project-cost-surface'; @@ -61,8 +60,6 @@ export class ProjectsCrudService extends AppBaseService< private readonly userProjects: Repository, @InjectRepository(ProtectedArea, DbConnections.geoprocessingDB) private readonly protectedAreas: Repository, - @InjectRepository(PublishedProject) - private readonly publishedProjects: Repository, private readonly commandBus: CommandBus, ) { super(repository, 'project', 'projects', { @@ -128,6 +125,34 @@ export class ProjectsCrudService extends AppBaseService< return query; } + /** + * In some cases, newly-created projects may not have any of the grid-related + * data: either a GADM id (if the project was created using a GADM area as + * planning area) or a planning area geometry id (if the user uploaded a + * shapefile with the project's planning area or planning grid). + * + * This could either be a temporary state (while the planning grid is being + * created in geodb) or an error situation. Either way, allowing users to + * "discover" such projects (e.g. when requesting a list of projects they + * have access to) is not desirable, as no meaningful work can be done when + * a project is in a state like this. Therefore, we hide such projects from + * the results of `findAll()` and `getById()` queries. + */ + private async hideProjectsWithIncompleteGridData( + query: SelectQueryBuilder, + ): Promise> { + query.andWhere(`NOT( + ${this.alias}.country_id IS NULL + AND + ${this.alias}.admin_area_l1_id IS NULL + AND + ${this.alias}.admin_area_l2_id IS NULL + AND + ${this.alias}.planning_area_geometry_id IS NULL + )`); + return query; + } + async setDataCreate( create: CreateProjectDTO, info?: ProjectsRequest, @@ -264,8 +289,8 @@ export class ProjectsCrudService extends AppBaseService< async extendGetByIdQuery( query: SelectQueryBuilder, - fetchSpecification?: FetchSpecification, - info?: ProjectsRequest, + _fetchSpecification?: FetchSpecification, + _info?: ProjectsRequest, ): Promise> { /** * Bring in publicMetadata (if the project has been made public). This is @@ -274,6 +299,8 @@ export class ProjectsCrudService extends AppBaseService< */ query.leftJoinAndSelect('project.publicMetadata', 'publicMetadata'); + this.hideProjectsWithIncompleteGridData(query); + return query; } @@ -318,6 +345,8 @@ export class ProjectsCrudService extends AppBaseService< ); } + this.hideProjectsWithIncompleteGridData(query); + if (loggedUser) { query .andWhere(`acl.user_id = :userId`, { From 2a2c753684996fd9ac10c3d790a2331691c9d4ce Mon Sep 17 00:00:00 2001 From: andrea rota Date: Fri, 13 Oct 2023 12:52:43 +0100 Subject: [PATCH 3/3] no need to bother with fine-grained grids in tests --- api/apps/api/test/e2e.config.ts | 4 ++-- .../api/test/projects/user-projects/user-projects.fixtures.ts | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/api/apps/api/test/e2e.config.ts b/api/apps/api/test/e2e.config.ts index 8201d429b0..5561ac676d 100644 --- a/api/apps/api/test/e2e.config.ts +++ b/api/apps/api/test/e2e.config.ts @@ -120,7 +120,7 @@ export const E2E_CONFIG: { name: faker.random.words(5), organizationId: faker.random.uuid(), planningUnitGridShape: PlanningUnitGridShape.Hexagon, - planningUnitAreakm2: 10, + planningUnitAreakm2: 10000, countryId: 'NAM', }), minimalInGivenAdminArea: (options?: { @@ -135,7 +135,7 @@ export const E2E_CONFIG: { adminAreaLevel1Id: options?.adminAreaLevel1Id, adminAreaLevel2Id: options?.adminAreaLevel2Id, planningUnitGridShape: PlanningUnitGridShape.Hexagon, - planningUnitAreakm2: 10, + planningUnitAreakm2: 10000, }), complete: (options: CountryCodeInput): CreateProjectDTO => ({ name: faker.random.words(5), diff --git a/api/apps/api/test/projects/user-projects/user-projects.fixtures.ts b/api/apps/api/test/projects/user-projects/user-projects.fixtures.ts index 448689595d..8d980caa7b 100644 --- a/api/apps/api/test/projects/user-projects/user-projects.fixtures.ts +++ b/api/apps/api/test/projects/user-projects/user-projects.fixtures.ts @@ -3,6 +3,7 @@ import { GivenUserIsLoggedIn } from '../../steps/given-user-is-logged-in'; import { OrganizationsTestUtils } from '../../utils/organizations.test.utils'; import { ProjectsTestUtils } from '../../utils/projects.test.utils'; import * as request from 'supertest'; +import { PlanningUnitGridShape } from '@marxan/scenarios-planning-unit'; export const getFixtures = async () => { const app = await bootstrapApplication(); @@ -16,6 +17,9 @@ export const getFixtures = async () => { await ProjectsTestUtils.createProject(app, authToken, { name: `User Project ${Date.now()}`, organizationId: org.data.id, + countryId: 'RWA', + planningUnitGridShape: PlanningUnitGridShape.Hexagon, + planningUnitAreakm2: 10000, }) ).data.id; };