From 93c2cc9817090c27aa08f871fe24f0514ca67b1c Mon Sep 17 00:00:00 2001 From: Xavier Carron <33637571+xav-car@users.noreply.github.com> Date: Mon, 2 Dec 2024 08:58:13 +0100 Subject: [PATCH] feat(api): add idempotent call to insert feature --- .../http-error-mapper-configuration.js | 5 ----- api/src/organizational-entities/domain/errors.js | 13 ------------- .../organization-feature-repository.js | 15 ++++++--------- .../application/api/learners-api.js | 5 +++-- .../organization/organization.admin.route.test.js | 15 --------------- .../organization-feature-repository_test.js | 12 +++--------- .../unit/application/api/learners-api_test.js | 5 +++++ 7 files changed, 17 insertions(+), 53 deletions(-) diff --git a/api/src/organizational-entities/application/http-error-mapper-configuration.js b/api/src/organizational-entities/application/http-error-mapper-configuration.js index 1c014fed6fb..1355b74cf51 100644 --- a/api/src/organizational-entities/application/http-error-mapper-configuration.js +++ b/api/src/organizational-entities/application/http-error-mapper-configuration.js @@ -1,7 +1,6 @@ import { HttpErrors } from '../../shared/application/http-errors.js'; import { DomainErrorMappingConfiguration } from '../../shared/application/models/domain-error-mapping-configuration.js'; import { - AlreadyExistingOrganizationFeatureError, DpoEmailInvalid, FeatureNotFound, FeatureParamsNotProcessable, @@ -16,10 +15,6 @@ const organizationalEntitiesDomainErrorMappingConfiguration = [ name: UnableToAttachChildOrganizationToParentOrganizationError.name, httpErrorFn: (error) => new HttpErrors.ConflictError(error.message, error.code, error.meta), }, - { - name: AlreadyExistingOrganizationFeatureError.name, - httpErrorFn: (error) => new HttpErrors.ConflictError(error.message, error.code, error.meta), - }, { name: OrganizationNotFound.name, httpErrorFn: (error) => new HttpErrors.UnprocessableEntityError(error.message, error.code, error.meta), diff --git a/api/src/organizational-entities/domain/errors.js b/api/src/organizational-entities/domain/errors.js index 64b92fd41dd..7238754c8cf 100644 --- a/api/src/organizational-entities/domain/errors.js +++ b/api/src/organizational-entities/domain/errors.js @@ -12,18 +12,6 @@ class UnableToAttachChildOrganizationToParentOrganizationError extends DomainErr } } -class AlreadyExistingOrganizationFeatureError extends DomainError { - constructor({ - code = 'ALREADY_EXISTING_ORGANIZATION_FEATURE', - message = 'Unable to add feature to organization', - meta, - } = {}) { - super(message); - this.code = code; - this.meta = meta; - } -} - class DpoEmailInvalid extends DomainError { constructor({ code = 'DPO_EMAIL_INVALID', message = 'DPO email invalid', meta } = {}) { super(message); @@ -72,7 +60,6 @@ class FeatureParamsNotProcessable extends DomainError { } export { - AlreadyExistingOrganizationFeatureError, DpoEmailInvalid, FeatureNotFound, FeatureParamsNotProcessable, diff --git a/api/src/organizational-entities/infrastructure/repositories/organization-feature-repository.js b/api/src/organizational-entities/infrastructure/repositories/organization-feature-repository.js index d9d16aefdcf..e801de0889b 100644 --- a/api/src/organizational-entities/infrastructure/repositories/organization-feature-repository.js +++ b/api/src/organizational-entities/infrastructure/repositories/organization-feature-repository.js @@ -3,7 +3,7 @@ */ import * as knexUtils from '../../../../src/shared/infrastructure/utils/knex-utils.js'; import { DomainTransaction } from '../../../shared/domain/DomainTransaction.js'; -import { AlreadyExistingOrganizationFeatureError, FeatureNotFound, OrganizationNotFound } from '../../domain/errors.js'; +import { FeatureNotFound, OrganizationNotFound } from '../../domain/errors.js'; import { OrganizationFeatureItem } from '../../domain/models/OrganizationFeatureItem.js'; /** @@ -13,21 +13,18 @@ import { OrganizationFeatureItem } from '../../domain/models/OrganizationFeature * @typedef {import('../../domain/models/OrganizationFeatureItem.js').OrganizationFeatureItem} OrganizationFeatureItem */ -const DEFAULT_BATCH_SIZE = 100; - /** ** * @param {OrganizationFeature[]} organizations */ -async function saveInBatch(organizationFeatures, batchSize = DEFAULT_BATCH_SIZE) { +async function saveInBatch(organizationFeatures) { try { const knexConn = DomainTransaction.getConnection(); - await knexConn.batchInsert('organization-features', organizationFeatures, batchSize); + await knexConn('organization-features') + .insert(organizationFeatures) + .onConflict(['featureId', 'organizationId']) + .ignore(); } catch (err) { - if (knexUtils.isUniqConstraintViolated(err)) { - throw new AlreadyExistingOrganizationFeatureError(); - } - if (knexUtils.foreignKeyConstraintViolated(err) && err.constraint.includes('featureid')) { throw new FeatureNotFound(); } diff --git a/api/src/prescription/learner-management/application/api/learners-api.js b/api/src/prescription/learner-management/application/api/learners-api.js index 3325ee97e01..9d3ea92beda 100644 --- a/api/src/prescription/learner-management/application/api/learners-api.js +++ b/api/src/prescription/learner-management/application/api/learners-api.js @@ -1,3 +1,4 @@ +import { withTransaction } from '../../../../shared/domain/DomainTransaction.js'; import { usecases } from '../../domain/usecases/index.js'; /** @@ -27,7 +28,7 @@ export const hasBeenLearner = async ({ userId }) => { * @returns {Promise} * @throws TypeError - Throw when params.userId or params.organizationId is not defined */ -export const deleteOrganizationLearnerBeforeImportFeature = async ({ userId, organizationId }) => { +export const deleteOrganizationLearnerBeforeImportFeature = withTransaction(async ({ userId, organizationId }) => { if (!userId) { throw new TypeError('userId is required'); } @@ -39,4 +40,4 @@ export const deleteOrganizationLearnerBeforeImportFeature = async ({ userId, org const organizationLearnerIds = await usecases.findOrganizationLearnersBeforeImportFeature({ organizationId }); return usecases.deleteOrganizationLearners({ userId, organizationId, organizationLearnerIds }); -}; +}); diff --git a/api/tests/organizational-entities/integration/application/organization/organization.admin.route.test.js b/api/tests/organizational-entities/integration/application/organization/organization.admin.route.test.js index f8c7fb642d6..853f33c506d 100644 --- a/api/tests/organizational-entities/integration/application/organization/organization.admin.route.test.js +++ b/api/tests/organizational-entities/integration/application/organization/organization.admin.route.test.js @@ -1,7 +1,6 @@ import { organizationAdminController } from '../../../../../src/organizational-entities/application/organization/organization.admin.controller.js'; import * as organizationAdminRoutes from '../../../../../src/organizational-entities/application/organization/organization.admin.route.js'; import { - AlreadyExistingOrganizationFeatureError, DpoEmailInvalid, FeatureNotFound, FeatureParamsNotProcessable, @@ -93,20 +92,6 @@ describe('Integration | Organizational Entities | Application | Route | Admin | expect(response.statusCode).to.equal(422); }); }); - - context('when trying to add already existing feature on organization', function () { - it('returns a 409 HTTP status code', async function () { - organizationAdminController.addOrganizationFeatureInBatch.rejects( - new AlreadyExistingOrganizationFeatureError(), - ); - - // when - const response = await httpTestServer.request(method, url, payload); - - // then - expect(response.statusCode).to.equal(409); - }); - }); }); describe('POST /api/admin/organizations/update-organizations', function () { diff --git a/api/tests/organizational-entities/integration/infrastructure/repositories/organization-feature-repository_test.js b/api/tests/organizational-entities/integration/infrastructure/repositories/organization-feature-repository_test.js index 47b659bb5c1..e48af1a928c 100644 --- a/api/tests/organizational-entities/integration/infrastructure/repositories/organization-feature-repository_test.js +++ b/api/tests/organizational-entities/integration/infrastructure/repositories/organization-feature-repository_test.js @@ -1,8 +1,4 @@ -import { - AlreadyExistingOrganizationFeatureError, - FeatureNotFound, - OrganizationNotFound, -} from '../../../../../src/organizational-entities/domain/errors.js'; +import { FeatureNotFound, OrganizationNotFound } from '../../../../../src/organizational-entities/domain/errors.js'; import { OrganizationFeature } from '../../../../../src/organizational-entities/domain/models/OrganizationFeature.js'; import { OrganizationFeatureItem } from '../../../../../src/organizational-entities/domain/models/OrganizationFeatureItem.js'; import * as organizationFeatureRepository from '../../../../../src/organizational-entities/infrastructure/repositories/organization-feature-repository.js'; @@ -54,7 +50,7 @@ describe('Integration | Repository | Organization-for-admin', function () { expect(result[1].params).to.deep.equal(organizationFeatures[1].params); }); - it('throws an error if organization feature already exists', async function () { + it('should passe even if organization feature already exists', async function () { databaseBuilder.factory.buildOrganizationFeature({ organizationId: organization.id, featureId: feature.id }); await databaseBuilder.commit(); @@ -67,9 +63,7 @@ describe('Integration | Repository | Organization-for-admin', function () { ]; // when - const error = await catchErr(organizationFeatureRepository.saveInBatch)(organizationFeatures); - - expect(error).to.be.instanceOf(AlreadyExistingOrganizationFeatureError); + expect(await organizationFeatureRepository.saveInBatch(organizationFeatures)).to.not.throws; }); it('throws an error if organization does not exists', async function () { diff --git a/api/tests/prescription/learner-management/unit/application/api/learners-api_test.js b/api/tests/prescription/learner-management/unit/application/api/learners-api_test.js index f05fc5474d3..8f3fe7d30fa 100644 --- a/api/tests/prescription/learner-management/unit/application/api/learners-api_test.js +++ b/api/tests/prescription/learner-management/unit/application/api/learners-api_test.js @@ -3,6 +3,7 @@ import { hasBeenLearner, } from '../../../../../../src/prescription/learner-management/application/api/learners-api.js'; import { usecases } from '../../../../../../src/prescription/learner-management/domain/usecases/index.js'; +import { DomainTransaction } from '../../../../../../src/shared/domain/DomainTransaction.js'; import { catchErr, expect, sinon } from '../../../../../test-helper.js'; describe('Unit | Prescription | learner management | Api | learners', function () { @@ -37,6 +38,10 @@ describe('Unit | Prescription | learner management | Api | learners', function ( let findOrganizationLearnersBeforeImportFeatureStub, deleteOrganizationLearnersStub; beforeEach(function () { + sinon.stub(DomainTransaction, 'execute').callsFake((callback) => { + return callback(); + }); + findOrganizationLearnersBeforeImportFeatureStub = sinon .stub(usecases, 'findOrganizationLearnersBeforeImportFeature') .rejects();