From 00c156a8e90ec4c067e8fd7380f7f71357354cbd Mon Sep 17 00:00:00 2001 From: alexeh Date: Fri, 18 Nov 2022 11:30:47 +0300 Subject: [PATCH] Avoid NaN impact values, order interventions desc --- .../indicator-record.entity.ts | 3 + .../services/impact-calculator.service.ts | 31 +++----- .../scenario-intervention.repository.ts | 1 + api/test/e2e/scenarios/scenarios.spec.ts | 28 ++++++- .../indicator-records.service.spec.ts | 76 +++++++++---------- .../indicator-record.validations.spec.ts | 53 +++++++++++++ 6 files changed, 130 insertions(+), 62 deletions(-) rename api/test/integration/indicator-record/{ => calculations}/indicator-records.service.spec.ts (93%) create mode 100644 api/test/integration/indicator-record/validations/indicator-record.validations.spec.ts diff --git a/api/src/modules/indicator-records/indicator-record.entity.ts b/api/src/modules/indicator-records/indicator-record.entity.ts index 15d140821..bf5ed4e5a 100644 --- a/api/src/modules/indicator-records/indicator-record.entity.ts +++ b/api/src/modules/indicator-records/indicator-record.entity.ts @@ -1,4 +1,5 @@ import { + Check, Column, Entity, getManager, @@ -39,6 +40,8 @@ export enum INDICATOR_RECORD_STATUS { } @Entity() +@Check(`value <> 'NaN'`) +@Check(`scaler <> 'NaN'`) export class IndicatorRecord extends TimestampedBaseEntity { @ApiProperty() @PrimaryGeneratedColumn('uuid') diff --git a/api/src/modules/indicator-records/services/impact-calculator.service.ts b/api/src/modules/indicator-records/services/impact-calculator.service.ts index 13fb7ac19..1ce885e3b 100644 --- a/api/src/modules/indicator-records/services/impact-calculator.service.ts +++ b/api/src/modules/indicator-records/services/impact-calculator.service.ts @@ -1,7 +1,6 @@ import { Injectable, Logger, - NotFoundException, ServiceUnavailableException, } from '@nestjs/common'; import { IndicatorRecordRepository } from 'modules/indicator-records/indicator-record.repository'; @@ -19,17 +18,12 @@ import { IndicatorRecord, } from 'modules/indicator-records/indicator-record.entity'; import { IndicatorCoefficientsDtoV2 } from 'modules/indicator-coefficients/dto/indicator-coefficients.dto'; -import { - MaterialToH3, - MATERIAL_TO_H3_TYPE, -} from 'modules/materials/material-to-h3.entity'; +import { MaterialToH3 } from 'modules/materials/material-to-h3.entity'; import { MissingH3DataError } from 'modules/indicator-records/errors/missing-h3-data.error'; import { IndicatorRecordCalculatedValuesDtoV2 } from 'modules/indicator-records/dto/indicator-record-calculated-values.dto'; import { MaterialsToH3sService } from 'modules/materials/materials-to-h3s.service'; import { IndicatorsService } from 'modules/indicators/indicators.service'; import { SourcingRecord } from 'modules/sourcing-records/sourcing-record.entity'; -import { H3Data } from 'modules/h3-data/h3-data.entity'; -import { H3DataService } from 'modules/h3-data/h3-data.service'; import { IndicatorDependencyManager } from 'modules/indicator-records/services/indicator-dependency-manager.service'; /** @@ -46,7 +40,6 @@ export class ImpactCalculator { private readonly materialToH3: MaterialsToH3sService, private readonly indicatorService: IndicatorsService, private readonly dependencyManager: IndicatorDependencyManager, - private readonly h3DataService: H3DataService, ) {} async calculateImpactForAllSourcingRecords( @@ -58,9 +51,9 @@ export class ImpactCalculator { const newImpactToBeSaved: IndicatorRecord[] = []; rawData.forEach((data: SourcingRecordsWithIndicatorRawDataDtoV2) => { - const landPerTon: number = data.harvestedArea ?? 0 / data.production ?? 0; + const landPerTon: number = data.harvestedArea / data.production || 0; const weightedTotalCropLandArea: number = - data.weightedAllHarvest ?? 0 / data.production ?? 0; + data.weightedAllHarvest / data.production || 0; const deforestationPerHarvestLandUse: number = weightedTotalCropLandArea > 0 ? data.rawDeforestation / weightedTotalCropLandArea @@ -194,9 +187,9 @@ export class ImpactCalculator { >(); const landPerTon: number = - rawData.harvestedArea ?? 0 / rawData.production ?? 0; + rawData.harvestedArea / rawData.production || 0; const weightedTotalCropLandArea: number = - rawData.weightedAllHarvest ?? 0 / rawData.production ?? 0; + rawData.weightedAllHarvest / rawData.production || 0; const deforestationPerHarvestLandUse: number = weightedTotalCropLandArea > 0 ? rawData.rawDeforestation / weightedTotalCropLandArea @@ -209,8 +202,7 @@ export class ImpactCalculator { const deforestation: number = deforestationPerHarvestLandUse * landUse; const carbonLoss: number = carbonPerHarvestLandUse * landUse; const waterUse: number = rawData.rawWater * sourcingData.tonnage; - const unsustainableWaterUse: number = - waterUse * rawData.waterStressPerct ? rawData.waterStressPerct : 0; + const unsustainableWaterUse: number = waterUse * rawData.waterStressPerct; calculatedIndicatorRecordValues.values.set( INDICATOR_TYPES_NEW.CLIMATE_RISK, @@ -275,9 +267,8 @@ export class ImpactCalculator { ]); return res[0]; } catch (error: any) { - console.error(error); throw new ServiceUnavailableException( - `Could not calculate Raw Indicator values for new Scenario`, + `Could not calculate Raw Indicator values for new Scenario ` + error, ); } } @@ -316,22 +307,22 @@ export class ImpactCalculator { calculatedIndicatorValues.values.set( INDICATOR_TYPES_NEW.LAND_USE, newIndicatorCoefficients[INDICATOR_TYPES_NEW.LAND_USE] * - sourcingData.tonnage, + sourcingData.tonnage || 0, ); calculatedIndicatorValues.values.set( INDICATOR_TYPES_NEW.DEFORESTATION_RISK, newIndicatorCoefficients[INDICATOR_TYPES_NEW.DEFORESTATION_RISK] * - sourcingData.tonnage, + sourcingData.tonnage || 0, ); calculatedIndicatorValues.values.set( INDICATOR_TYPES_NEW.CLIMATE_RISK, newIndicatorCoefficients[INDICATOR_TYPES_NEW.CLIMATE_RISK] * - sourcingData.tonnage, + sourcingData.tonnage || 0, ); calculatedIndicatorValues.values.set( INDICATOR_TYPES_NEW.WATER_USE, newIndicatorCoefficients[INDICATOR_TYPES_NEW.WATER_USE] * - sourcingData.tonnage, + sourcingData.tonnage || 0, ); // TODO: We need to ignore satelligence indicators from being affected by a coefficient that a user can send diff --git a/api/src/modules/scenario-interventions/scenario-intervention.repository.ts b/api/src/modules/scenario-interventions/scenario-intervention.repository.ts index c52c88f72..ef8c1633f 100644 --- a/api/src/modules/scenario-interventions/scenario-intervention.repository.ts +++ b/api/src/modules/scenario-interventions/scenario-intervention.repository.ts @@ -71,6 +71,7 @@ export class ScenarioInterventionRepository extends Repository { expect(response).toHaveJSONAPIAttributes(expectedJSONAPIAttributes); }); test( - 'When I filter a Scenario by Id and I include its interventions in the query + ' + + 'When I filter Interventions by Scenario Id + ' + 'Then I should receive said Interventions in the response' + 'And they should include the replaced entity information', async () => { @@ -511,4 +511,30 @@ describe('ScenariosModule (e2e)', () => { }, ); }); + test( + 'When I filter Interventions by Scenario Id + ' + + 'Then I should receive said Interventions in the response' + + 'And they should be ordered by creation date in a DESC order', + async () => { + const interventions: ScenarioIntervention[] = []; + + const scenario: Scenario = await createScenario(); + + for (const n of [1, 2, 3, 4, 5]) { + const intervention = await createScenarioIntervention({ + scenario, + title: `inter ${n}`, + }); + interventions.push(intervention); + } + const response = await request(app.getHttpServer()) + .get(`/api/v1/scenarios/${scenario.id}/interventions`) + .set('Authorization', `Bearer ${jwtToken}`) + .send(); + + expect( + interventions.map((i: ScenarioIntervention) => i.id).reverse(), + ).toEqual(response.body.data.map((i: ScenarioIntervention) => i.id)); + }, + ); }); diff --git a/api/test/integration/indicator-record/indicator-records.service.spec.ts b/api/test/integration/indicator-record/calculations/indicator-records.service.spec.ts similarity index 93% rename from api/test/integration/indicator-record/indicator-records.service.spec.ts rename to api/test/integration/indicator-record/calculations/indicator-records.service.spec.ts index 2f69d1fa9..4b2e84ecd 100644 --- a/api/test/integration/indicator-record/indicator-records.service.spec.ts +++ b/api/test/integration/indicator-record/calculations/indicator-records.service.spec.ts @@ -1,10 +1,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { AppModule } from 'app.module'; import { IndicatorRecordRepository } from 'modules/indicator-records/indicator-record.repository'; -import { - CachedRawValue, - IndicatorRecordsService, -} from 'modules/indicator-records/indicator-records.service'; +import { CachedRawValue } from 'modules/indicator-records/indicator-records.service'; import { createAdminRegion, createBusinessUnit, @@ -16,21 +13,19 @@ import { createSourcingLocation, createSourcingRecord, createSupplier, -} from '../../entity-mocks'; +} from '../../../entity-mocks'; import { Indicator, INDICATOR_TYPES, INDICATOR_TYPES_NEW, } from 'modules/indicators/indicator.entity'; + import { INDICATOR_RECORD_STATUS, IndicatorRecord, } from 'modules/indicator-records/indicator-record.entity'; import { MaterialsToH3sService } from 'modules/materials/materials-to-h3s.service'; -import { - IndicatorCoefficientsDto, - IndicatorCoefficientsDtoV2, -} from 'modules/indicator-coefficients/dto/indicator-coefficients.dto'; +import { IndicatorCoefficientsDtoV2 } from 'modules/indicator-coefficients/dto/indicator-coefficients.dto'; import { MissingH3DataError } from 'modules/indicator-records/errors/missing-h3-data.error'; import { H3DataRepository } from 'modules/h3-data/h3-data.repository'; import { Material } from 'modules/materials/material.entity'; @@ -40,33 +35,33 @@ import { BusinessUnit } from 'modules/business-units/business-unit.entity'; import { SourcingLocation } from 'modules/sourcing-locations/sourcing-location.entity'; import { GeoRegion } from 'modules/geo-regions/geo-region.entity'; import { IndicatorRecordsModule } from 'modules/indicator-records/indicator-records.module'; -import { createWorldToCalculateIndicatorRecords } from '../../utils/indicator-records-preconditions'; +import { createWorldToCalculateIndicatorRecords } from '../../../utils/indicator-records-preconditions'; import { v4 as UUIDv4 } from 'uuid'; import { H3Data } from 'modules/h3-data/h3-data.entity'; -import { dropH3GridTables } from '../../utils/database-test-helper'; +import { dropH3GridTables } from '../../../utils/database-test-helper'; import { MATERIAL_TO_H3_TYPE, MaterialToH3, -} from '../../../src/modules/materials/material-to-h3.entity'; -import { h3MaterialExampleDataFixture } from '../../e2e/h3-data/mocks/h3-fixtures'; +} from '../../../../src/modules/materials/material-to-h3.entity'; +import { h3MaterialExampleDataFixture } from '../../../e2e/h3-data/mocks/h3-fixtures'; import { dropH3DataMock, h3DataMock, -} from '../../e2e/h3-data/mocks/h3-data.mock'; +} from '../../../e2e/h3-data/mocks/h3-data.mock'; import { NotFoundException, ServiceUnavailableException } from '@nestjs/common'; -import { CachedDataService } from '../../../src/modules/cached-data/cached-data.service'; +import { CachedDataService } from '../../../../src/modules/cached-data/cached-data.service'; import { CACHED_DATA_TYPE, CachedData, -} from '../../../src/modules/cached-data/cached.data.entity'; -import { IndicatorRepository } from '../../../src/modules/indicators/indicator.repository'; -import { SourcingRecordRepository } from '../../../src/modules/sourcing-records/sourcing-record.repository'; -import { AdminRegionRepository } from '../../../src/modules/admin-regions/admin-region.repository'; -import { BusinessUnitRepository } from '../../../src/modules/business-units/business-unit.repository'; -import { SupplierRepository } from '../../../src/modules/suppliers/supplier.repository'; -import { GeoRegionRepository } from '../../../src/modules/geo-regions/geo-region.repository'; -import { MaterialRepository } from '../../../src/modules/materials/material.repository'; -import { CachedDataRepository } from '../../../src/modules/cached-data/cached-data.repository'; +} from '../../../../src/modules/cached-data/cached.data.entity'; +import { IndicatorRepository } from '../../../../src/modules/indicators/indicator.repository'; +import { SourcingRecordRepository } from '../../../../src/modules/sourcing-records/sourcing-record.repository'; +import { AdminRegionRepository } from '../../../../src/modules/admin-regions/admin-region.repository'; +import { BusinessUnitRepository } from '../../../../src/modules/business-units/business-unit.repository'; +import { SupplierRepository } from '../../../../src/modules/suppliers/supplier.repository'; +import { GeoRegionRepository } from '../../../../src/modules/geo-regions/geo-region.repository'; +import { MaterialRepository } from '../../../../src/modules/materials/material.repository'; +import { CachedDataRepository } from '../../../../src/modules/cached-data/cached-data.repository'; import { SourcingRecord } from 'modules/sourcing-records/sourcing-record.entity'; import { ImpactCalculator } from 'modules/indicator-records/services/impact-calculator.service'; @@ -82,7 +77,7 @@ describe('Indicator Records Service', () => { let materialRepository: MaterialRepository; let cachedDataRepository: CachedDataRepository; - let indicatorRecordService: ImpactCalculator; + let impactCalculator: ImpactCalculator; let materialsToH3sService: MaterialsToH3sService; let cachedDataService: CachedDataService; @@ -115,8 +110,7 @@ describe('Indicator Records Service', () => { cachedDataRepository = testingModule.get(CachedDataRepository); - indicatorRecordService = - testingModule.get(ImpactCalculator); + impactCalculator = testingModule.get(ImpactCalculator); materialsToH3sService = testingModule.get( MaterialsToH3sService, ); @@ -186,7 +180,7 @@ describe('Indicator Records Service', () => { //ACT const testStatement = async (): Promise => { - await indicatorRecordService.createIndicatorRecordsBySourcingRecords( + await impactCalculator.createIndicatorRecordsBySourcingRecords( sourcingData, providedCoefficients, ); @@ -242,7 +236,7 @@ describe('Indicator Records Service', () => { //ACT const calculatedIndicators = - await indicatorRecordService.createIndicatorRecordsBySourcingRecords( + await impactCalculator.createIndicatorRecordsBySourcingRecords( sourcingData, providedCoefficients, ); @@ -319,7 +313,7 @@ describe('Indicator Records Service', () => { .mockResolvedValueOnce(undefined); const testStatement = async (): Promise => { - await indicatorRecordService.createIndicatorRecordsBySourcingRecords( + await impactCalculator.createIndicatorRecordsBySourcingRecords( sourcingData, ); }; @@ -359,7 +353,7 @@ describe('Indicator Records Service', () => { //ACT const testStatement = async (): Promise => { - await indicatorRecordService.createIndicatorRecordsBySourcingRecords( + await impactCalculator.createIndicatorRecordsBySourcingRecords( sourcingData, ); }; @@ -414,7 +408,7 @@ describe('Indicator Records Service', () => { //ACT const testStatement = async (): Promise => { - await indicatorRecordService.createIndicatorRecordsBySourcingRecords( + await impactCalculator.createIndicatorRecordsBySourcingRecords( sourcingData, ); }; @@ -458,7 +452,7 @@ describe('Indicator Records Service', () => { ); const calculatedRecords = - await indicatorRecordService.createIndicatorRecordsBySourcingRecords( + await impactCalculator.createIndicatorRecordsBySourcingRecords( sourcingData, ); @@ -489,7 +483,7 @@ describe('Indicator Records Service', () => { indicatorPreconditions.landUseIndicator, materialH3DataProducer, sourcingData.sourcingRecordId, - 805000, + 500, 1610, calculatedRecords, ); @@ -543,7 +537,7 @@ describe('Indicator Records Service', () => { //ACT - await indicatorRecordService.calculateImpactForAllSourcingRecords([ + await impactCalculator.calculateImpactForAllSourcingRecords([ indicatorPreconditions.waterUseIndicator, indicatorPreconditions.unsustWaterUseIndicator, indicatorPreconditions.climateRiskIndicator, @@ -568,7 +562,7 @@ describe('Indicator Records Service', () => { indicatorPreconditions.landUseIndicator, materialH3DataProducer1, indicatorPreconditions.sourcingRecord1.id, - 1610000, + 1000, 1610, ); await checkCreatedIndicatorRecord( @@ -601,7 +595,7 @@ describe('Indicator Records Service', () => { indicatorPreconditions.landUseIndicator, materialH3DataProducer2, indicatorPreconditions.sourcingRecord2.id, - 805000, + 500, 1610, ); await checkCreatedIndicatorRecord( @@ -660,7 +654,7 @@ describe('Indicator Records Service', () => { ); //Small "hack" to access the method to simplify part of the cache key - const indicatorRecordServiceAny: any = indicatorRecordService; + const indicatorRecordServiceAny: any = impactCalculator; const generateIndicatorCacheKey: any = indicatorRecordServiceAny.generateIndicatorCalculationCacheKey; const generateMaterialCacheKey: any = @@ -712,7 +706,7 @@ describe('Indicator Records Service', () => { //ACT const calculatedRecords = - await indicatorRecordService.createIndicatorRecordsBySourcingRecords( + await impactCalculator.createIndicatorRecordsBySourcingRecords( sourcingData, ); @@ -803,7 +797,7 @@ describe('Indicator Records Service', () => { materialsMap.set(MATERIAL_TO_H3_TYPE.PRODUCER, h3MaterialProducer); //Small "hack" to access the method to simplify part of the cache key - const indicatorRecordServiceAny: any = indicatorRecordService; + const indicatorRecordServiceAny: any = impactCalculator; const generateIndicatorCacheKey: any = indicatorRecordServiceAny.generateIndicatorCalculationCacheKey; const generateMaterialCacheKey: any = @@ -844,7 +838,7 @@ describe('Indicator Records Service', () => { //ACT const calculatedRecords = - await indicatorRecordService.createIndicatorRecordsBySourcingRecords( + await impactCalculator.createIndicatorRecordsBySourcingRecords( sourcingData, ); diff --git a/api/test/integration/indicator-record/validations/indicator-record.validations.spec.ts b/api/test/integration/indicator-record/validations/indicator-record.validations.spec.ts new file mode 100644 index 000000000..a2fdcf742 --- /dev/null +++ b/api/test/integration/indicator-record/validations/indicator-record.validations.spec.ts @@ -0,0 +1,53 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { AppModule } from 'app.module'; + +import { createIndicatorRecord } from '../../../entity-mocks'; + +import { IndicatorRecord } from 'modules/indicator-records/indicator-record.entity'; + +import { Material } from 'modules/materials/material.entity'; + +import { IndicatorRecordsModule } from 'modules/indicator-records/indicator-records.module'; + +import { H3Data } from 'modules/h3-data/h3-data.entity'; +import { clearEntityTables } from '../../../utils/database-test-helper'; +import { MaterialToH3 } from '../../../../src/modules/materials/material-to-h3.entity'; + +import { SourcingRecord } from 'modules/sourcing-records/sourcing-record.entity'; +import { getConnection } from 'typeorm'; + +describe('Indicator Records Service', () => { + let testingModule: TestingModule; + + beforeAll(async () => { + testingModule = await Test.createTestingModule({ + imports: [AppModule, IndicatorRecordsModule], + }).compile(); + }); + + afterEach(async () => { + await clearEntityTables([ + IndicatorRecord, + SourcingRecord, + MaterialToH3, + Material, + H3Data, + ]); + }); + + afterAll(() => testingModule.close()); + + test('When I want to save a calculated impact, But this impact has a NaN value, Then I should not be able to persist it in the database', async () => { + const indicatorRecord = await createIndicatorRecord({ + value: NaN, + }).catch((e: any) => e); + + const indicatorRecords: IndicatorRecord[] = await getConnection() + .getRepository(IndicatorRecord) + .find(); + + expect(indicatorRecord.value).toBeUndefined(); + expect(indicatorRecord.scaler).toBeUndefined(); + expect(indicatorRecords).toHaveLength(0); + }); +});