From f96e822de10d9d449a7037e53a7816264ebf2b24 Mon Sep 17 00:00:00 2001 From: Xavier Carron <33637571+xav-car@users.noreply.github.com> Date: Mon, 25 Nov 2024 16:00:19 +0100 Subject: [PATCH 1/8] refactor(api): use Domaintransaction.getconection() --- .../repositories/organization-feature-repository.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 b5c7fb049c4..d9d16aefdcf 100644 --- a/api/src/organizational-entities/infrastructure/repositories/organization-feature-repository.js +++ b/api/src/organizational-entities/infrastructure/repositories/organization-feature-repository.js @@ -1,8 +1,8 @@ /** * @module OrganizationFeatureRepository */ -import { knex } from '../../../../db/knex-database-connection.js'; 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 { OrganizationFeatureItem } from '../../domain/models/OrganizationFeatureItem.js'; @@ -21,7 +21,8 @@ const DEFAULT_BATCH_SIZE = 100; */ async function saveInBatch(organizationFeatures, batchSize = DEFAULT_BATCH_SIZE) { try { - await knex.batchInsert('organization-features', organizationFeatures, batchSize); + const knexConn = DomainTransaction.getConnection(); + await knexConn.batchInsert('organization-features', organizationFeatures, batchSize); } catch (err) { if (knexUtils.isUniqConstraintViolated(err)) { throw new AlreadyExistingOrganizationFeatureError(); @@ -51,7 +52,8 @@ async function saveInBatch(organizationFeatures, batchSize = DEFAULT_BATCH_SIZE) * @returns {Promise} */ async function findAllOrganizationFeaturesFromOrganizationId({ organizationId }) { - const organizationFeatures = await knex + const knexConn = DomainTransaction.getConnection(); + const organizationFeatures = await knexConn .select('key', 'params') .from('organization-features') .join('features', 'features.id', 'organization-features.featureId') From ca986b9caec8e710bfeccd0559d0d5d5f1157518 Mon Sep 17 00:00:00 2001 From: Xavier Carron <33637571+xav-car@users.noreply.github.com> Date: Mon, 25 Nov 2024 16:06:37 +0100 Subject: [PATCH 2/8] refactor(api): use domain transaction on repo --- .../organization.admin.controller.js | 5 +- .../add-organization-feature-in-batch.js | 49 ++++++++++--------- .../add-organization-feature-in-batch_test.js | 5 ++ 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/api/src/organizational-entities/application/organization/organization.admin.controller.js b/api/src/organizational-entities/application/organization/organization.admin.controller.js index f6643065bb3..ce8d632a942 100644 --- a/api/src/organizational-entities/application/organization/organization.admin.controller.js +++ b/api/src/organizational-entities/application/organization/organization.admin.controller.js @@ -20,7 +20,10 @@ const attachChildOrganization = async function (request, h) { }; const addOrganizationFeatureInBatch = async function (request, h) { - await usecases.addOrganizationFeatureInBatch({ filePath: request.payload.path }); + await usecases.addOrganizationFeatureInBatch({ + userId: request.auth.credentials.userId, + filePath: request.payload.path, + }); return h.response().code(204); }; diff --git a/api/src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js b/api/src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js index 71bd4712dab..38952dd1dca 100644 --- a/api/src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js +++ b/api/src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js @@ -6,6 +6,7 @@ import { createReadStream } from 'node:fs'; import { CsvColumn } from '../../../../lib/infrastructure/serializers/csv/csv-column.js'; import { getDataBuffer } from '../../../prescription/learner-management/infrastructure/utils/bufferize/get-data-buffer.js'; +import { withTransaction } from '../../../shared/domain/DomainTransaction.js'; import { CsvParser } from '../../../shared/infrastructure/serializers/csv/csv-parser.js'; import { FeatureParamsNotProcessable } from '../errors.js'; import { OrganizationFeature } from '../models/OrganizationFeature.js'; @@ -30,28 +31,28 @@ const organizationFeatureCsvHeader = { ], }; -/** - * @param {Object} params - A parameter object. - * @param {string} params.featureId - feature id to add. - * @param {string} params.filePath - path of csv file wich contains organizations and params. - * @param {OrganizationFeatureRepository} params.organizationFeatureRepository - organizationRepository to use. - * @param {Object} params.dependencies - * @returns {Promise} - */ -async function addOrganizationFeatureInBatch({ filePath, organizationFeatureRepository }) { - const stream = createReadStream(filePath); - const buffer = await getDataBuffer(stream); - - const csvParser = new CsvParser(buffer, organizationFeatureCsvHeader); - const csvData = csvParser.parse(); - const data = csvData.map(({ featureId, organizationId, params }) => { - try { - return new OrganizationFeature({ featureId, organizationId, params: params }); - } catch (err) { - throw new FeatureParamsNotProcessable(); - } - }); - return organizationFeatureRepository.saveInBatch(data); -} +export const addOrganizationFeatureInBatch = withTransaction( + /** + * @param {Object} params - A parameter object. + * @param {Number} params.userId - user connected performing action + * @param {string} params.filePath - path of csv file wich contains organizations and params. + * @param {OrganizationFeatureRepository} params.organizationFeatureRepository - organizationRepository to use. + * @param {Object} params.dependencies + * @returns {Promise} + */ + async ({ filePath, organizationFeatureRepository }) => { + const stream = createReadStream(filePath); + const buffer = await getDataBuffer(stream); -export { addOrganizationFeatureInBatch }; + const csvParser = new CsvParser(buffer, organizationFeatureCsvHeader); + const csvData = csvParser.parse(); + const data = csvData.map(({ featureId, organizationId, params }) => { + try { + return new OrganizationFeature({ featureId, organizationId, params: params }); + } catch (err) { + throw new FeatureParamsNotProcessable(); + } + }); + return organizationFeatureRepository.saveInBatch(data); + }, +); diff --git a/api/tests/organizational-entities/unit/domain/usecases/add-organization-feature-in-batch_test.js b/api/tests/organizational-entities/unit/domain/usecases/add-organization-feature-in-batch_test.js index b42da0ed091..8b722f2cb8c 100644 --- a/api/tests/organizational-entities/unit/domain/usecases/add-organization-feature-in-batch_test.js +++ b/api/tests/organizational-entities/unit/domain/usecases/add-organization-feature-in-batch_test.js @@ -1,12 +1,17 @@ import { FeatureParamsNotProcessable } from '../../../../../src/organizational-entities/domain/errors.js'; import { OrganizationFeature } from '../../../../../src/organizational-entities/domain/models/OrganizationFeature.js'; import { addOrganizationFeatureInBatch } from '../../../../../src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js'; +import { DomainTransaction } from '../../../../../src/shared/domain/DomainTransaction.js'; import { catchErr, createTempFile, expect, removeTempFile, sinon } from '../../../../test-helper.js'; describe('Unit | Domain | UseCases | add-organization-feature-in-batch', function () { let organizationFeatureRepository, featureId, filePath, csvData; beforeEach(function () { + sinon.stub(DomainTransaction, 'execute').callsFake((callback) => { + return callback(); + }); + featureId = 1; csvData = [ new OrganizationFeature({ featureId, organizationId: 123, params: `{ "id": 123 }` }), From 237213e287ec9b480516a79ac18afa100d7ef0b5 Mon Sep 17 00:00:00 2001 From: Xavier Carron <33637571+xav-car@users.noreply.github.com> Date: Mon, 25 Nov 2024 16:26:42 +0100 Subject: [PATCH 3/8] tech?(api): remove application transaction to domain transaction --- .../repositories/organization-learner-repository.js | 11 +++++------ .../organization-learner-repository_test.js | 7 +++---- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/api/src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js b/api/src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js index 54dbdc45083..45d5df63c4b 100644 --- a/api/src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js +++ b/api/src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js @@ -8,7 +8,6 @@ import { UserCouldNotBeReconciledError, } from '../../../../shared/domain/errors.js'; import { OrganizationLearner } from '../../../../shared/domain/models/index.js'; -import { ApplicationTransaction } from '../../../shared/infrastructure/ApplicationTransaction.js'; import { CommonOrganizationLearner } from '../../domain/models/CommonOrganizationLearner.js'; import { OrganizationLearnerForAdmin } from '../../domain/read-models/OrganizationLearnerForAdmin.js'; import * as studentRepository from './student-repository.js'; @@ -163,7 +162,7 @@ function _shouldStudentToImportBeReconciled( } const saveCommonOrganizationLearners = function (learners) { - const knex = ApplicationTransaction.getConnection(); + const knex = DomainTransaction.getConnection(); return Promise.all( learners.map((learner) => { @@ -182,7 +181,7 @@ const disableCommonOrganizationLearnersFromOrganizationId = function ({ organizationId, excludeOrganizationLearnerIds = [], }) { - const knex = ApplicationTransaction.getConnection(); + const knex = DomainTransaction.getConnection(); return knex('organization-learners') .where({ organizationId, isDisabled: false }) .whereNull('deletedAt') @@ -191,7 +190,7 @@ const disableCommonOrganizationLearnersFromOrganizationId = function ({ }; const findAllCommonLearnersFromOrganizationId = async function ({ organizationId }) { - const knex = ApplicationTransaction.getConnection(); + const knex = DomainTransaction.getConnection(); const existingLearners = await knex('view-active-organization-learners') .select(['firstName', 'id', 'lastName', 'userId', 'organizationId', 'attributes']) @@ -215,7 +214,7 @@ const findAllCommonOrganizationLearnerByReconciliationInfos = async function ({ organizationId, reconciliationInformations, }) { - const knex = ApplicationTransaction.getConnection(); + const knex = DomainTransaction.getConnection(); const query = knex('view-active-organization-learners') .select('firstName', 'lastName', 'id', 'attributes', 'userId') @@ -234,7 +233,7 @@ const findAllCommonOrganizationLearnerByReconciliationInfos = async function ({ }; const update = async function (organizationLearner) { - const knex = ApplicationTransaction.getConnection(); + const knex = DomainTransaction.getConnection(); const { id, ...attributes } = organizationLearner; const updatedRows = await knex('organization-learners').update(attributes).where({ id }); diff --git a/api/tests/prescription/learner-management/integration/infrastructure/repositories/organization-learner-repository_test.js b/api/tests/prescription/learner-management/integration/infrastructure/repositories/organization-learner-repository_test.js index 6ff0d1f0ff1..8acaa2c5602 100644 --- a/api/tests/prescription/learner-management/integration/infrastructure/repositories/organization-learner-repository_test.js +++ b/api/tests/prescription/learner-management/integration/infrastructure/repositories/organization-learner-repository_test.js @@ -1,6 +1,5 @@ import _ from 'lodash'; -import { DomainTransaction } from '../../../../../../lib/infrastructure/DomainTransaction.js'; import * as organizationLearnerRepository from '../../../../../../lib/infrastructure/repositories/organization-learner-repository.js'; import { CommonOrganizationLearner } from '../../../../../../src/prescription/learner-management/domain/models/CommonOrganizationLearner.js'; import { OrganizationLearnerForAdmin } from '../../../../../../src/prescription/learner-management/domain/read-models/OrganizationLearnerForAdmin.js'; @@ -19,7 +18,7 @@ import { saveCommonOrganizationLearners, update, } from '../../../../../../src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js'; -import { ApplicationTransaction } from '../../../../../../src/prescription/shared/infrastructure/ApplicationTransaction.js'; +import { DomainTransaction } from '../../../../../../src/shared/domain/DomainTransaction.js'; import { NotFoundError, OrganizationLearnersCouldNotBeSavedError, @@ -1087,7 +1086,7 @@ describe('Integration | Repository | Organization Learner Management | Organizat }); try { - await ApplicationTransaction.execute(async () => { + await DomainTransaction.execute(async () => { await saveCommonOrganizationLearners([learnerSacha]); throw new Error(); }); @@ -1243,7 +1242,7 @@ describe('Integration | Repository | Organization Learner Management | Organizat await databaseBuilder.commit(); try { - await ApplicationTransaction.execute(async () => { + await DomainTransaction.execute(async () => { await disableCommonOrganizationLearnersFromOrganizationId({ organizationId }); throw new Error(); }); From b9ca728ee63ea58baf93dcc5a4e3eb4e82d51560 Mon Sep 17 00:00:00 2001 From: Xavier Carron <33637571+xav-car@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:25:38 +0100 Subject: [PATCH 4/8] feat(api): add method to find learners before import feature --- .../organization-learner-repository.js | 13 +++ .../organization-learner-repository_test.js | 87 +++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/api/src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js b/api/src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js index 45d5df63c4b..ca360690b8d 100644 --- a/api/src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js +++ b/api/src/prescription/learner-management/infrastructure/repositories/organization-learner-repository.js @@ -302,6 +302,18 @@ const reconcileUserToOrganizationLearner = async function ({ userId, organizatio } }; +/** + * @function + * @name findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId + * @param {Object} params + * @param {number} params.organizationId + * @returns {Promise} + */ +const findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId = async function ({ organizationId }) { + const knexConn = DomainTransaction.getConnection(); + return knexConn('view-active-organization-learners').where({ organizationId }).whereNull('attributes').pluck('id'); +}; + export { addOrUpdateOrganizationOfOrganizationLearners, countByUserId, @@ -311,6 +323,7 @@ export { findAllCommonLearnersFromOrganizationId, findAllCommonOrganizationLearnerByReconciliationInfos, findByUserId, + findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId, findOrganizationLearnerIdsByOrganizationId, getOrganizationLearnerForAdmin, reconcileUserByNationalStudentIdAndOrganizationId, diff --git a/api/tests/prescription/learner-management/integration/infrastructure/repositories/organization-learner-repository_test.js b/api/tests/prescription/learner-management/integration/infrastructure/repositories/organization-learner-repository_test.js index 8acaa2c5602..1309ac5e5ff 100644 --- a/api/tests/prescription/learner-management/integration/infrastructure/repositories/organization-learner-repository_test.js +++ b/api/tests/prescription/learner-management/integration/infrastructure/repositories/organization-learner-repository_test.js @@ -10,6 +10,7 @@ import { disableCommonOrganizationLearnersFromOrganizationId, findAllCommonLearnersFromOrganizationId, findAllCommonOrganizationLearnerByReconciliationInfos, + findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId, findOrganizationLearnerIdsByOrganizationId, getOrganizationLearnerForAdmin, reconcileUserByNationalStudentIdAndOrganizationId, @@ -1952,4 +1953,90 @@ describe('Integration | Repository | Organization Learner Management | Organizat }); }); }); + + describe('#findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId ', function () { + let organizationId, organizationLearnerIdOfYesYes; + + beforeEach(async function () { + organizationId = databaseBuilder.factory.buildOrganization().id; + + organizationLearnerIdOfYesYes = + databaseBuilder.factory.prescription.organizationLearners.buildOrganizationLearner({ + organizationId, + firstName: 'Oui', + lastName: 'Oui', + userId: null, + attributes: null, + }).id; + + await databaseBuilder.commit(); + }); + + it('should return an array of organization learner id given organizationId', async function () { + const otherOrganizationLearnerId = + databaseBuilder.factory.prescription.organizationLearners.buildOrganizationLearner({ + organizationId, + firstName: 'Non', + lastName: 'Non', + userId: null, + attributes: null, + }).id; + + await databaseBuilder.commit(); + + const results = await findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId({ organizationId }); + + expect([organizationLearnerIdOfYesYes, otherOrganizationLearnerId]).to.be.deep.members(results); + }); + + it('should not return organization learners from other organization', async function () { + databaseBuilder.factory.prescription.organizationLearners.buildOrganizationLearner({ + organizationId: databaseBuilder.factory.buildOrganization().id, + firstName: 'Non', + lastName: 'Non', + userId: null, + attributes: null, + }).id; + + await databaseBuilder.commit(); + + const results = await findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId({ organizationId }); + + expect([organizationLearnerIdOfYesYes]).to.be.deep.members(results); + }); + + it('should not return organization learners already deleted', async function () { + databaseBuilder.factory.prescription.organizationLearners.buildOrganizationLearner({ + organizationId, + firstName: 'Non', + lastName: 'Non', + userId: null, + attributes: null, + deletedAt: new Date('2020-01-01'), + }).id; + + await databaseBuilder.commit(); + + const results = await findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId({ organizationId }); + + expect([organizationLearnerIdOfYesYes]).to.be.deep.members(results); + }); + + it('should not return organization learners with attributes', async function () { + databaseBuilder.factory.prescription.organizationLearners.buildOrganizationLearner({ + organizationId, + firstName: 'Non', + lastName: 'Non', + userId: null, + attributes: { test: 'toto' }, + deletedAt: null, + }).id; + + await databaseBuilder.commit(); + + const results = await findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId({ organizationId }); + + expect([organizationLearnerIdOfYesYes]).to.be.deep.members(results); + }); + }); }); From 53613265f9b2873d1f8b4f6c11933c07e541b43b Mon Sep 17 00:00:00 2001 From: Xavier Carron <33637571+xav-car@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:26:18 +0100 Subject: [PATCH 5/8] feat(api): add use case to find organizationLearnerIds to delete --- ...nization-learners-before-import-feature.js | 16 ++++++++++ ...ion-learners-before-import-feature_test.js | 30 +++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 api/src/prescription/learner-management/domain/usecases/find-organization-learners-before-import-feature.js create mode 100644 api/tests/prescription/learner-management/unit/domain/usecases/find-organization-learners-before-import-feature_test.js diff --git a/api/src/prescription/learner-management/domain/usecases/find-organization-learners-before-import-feature.js b/api/src/prescription/learner-management/domain/usecases/find-organization-learners-before-import-feature.js new file mode 100644 index 00000000000..f5f470a8c44 --- /dev/null +++ b/api/src/prescription/learner-management/domain/usecases/find-organization-learners-before-import-feature.js @@ -0,0 +1,16 @@ +/** + * @typedef {import('./index.js').OrganizationLearnerRepository} OrganizationLearnerRepository + */ + +/** + * @param{number} organizationId + * @param{OrganizationLearnerRepository} organizationLearnerRepository + * @returns {Promise} + */ +const findOrganizationLearnersBeforeImportFeature = async function ({ organizationId, organizationLearnerRepository }) { + return organizationLearnerRepository.findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId({ + organizationId, + }); +}; + +export { findOrganizationLearnersBeforeImportFeature }; diff --git a/api/tests/prescription/learner-management/unit/domain/usecases/find-organization-learners-before-import-feature_test.js b/api/tests/prescription/learner-management/unit/domain/usecases/find-organization-learners-before-import-feature_test.js new file mode 100644 index 00000000000..041acefe3d7 --- /dev/null +++ b/api/tests/prescription/learner-management/unit/domain/usecases/find-organization-learners-before-import-feature_test.js @@ -0,0 +1,30 @@ +import { findOrganizationLearnersBeforeImportFeature } from '../../../../../../src/prescription/learner-management/domain/usecases/find-organization-learners-before-import-feature.js'; +import { expect, sinon } from '../../../../../test-helper.js'; + +describe('Unit | UseCase | Organization Learners Management | findOrganizationLearnersBeforeImportFeature', function () { + let organizationLearnerRepository; + let organizationId; + + beforeEach(function () { + organizationId = Symbol('organizationId'); + organizationLearnerRepository = { + findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId: sinon.stub(), + }; + }); + + it('return data given from repository', async function () { + // given + const organizationLearnerIds = Symbol('organizationLearnerIds'); + organizationLearnerRepository.findOrganizationLearnerIdsBeforeImportFeatureFromOrganizationId + .withArgs({ organizationId }) + .returns(organizationLearnerIds); + + // when + const result = await findOrganizationLearnersBeforeImportFeature({ + organizationId, + organizationLearnerRepository, + }); + + expect(result).to.be.equal(organizationLearnerIds); + }); +}); From a4cc8c95ee98c8eb72a463a83d5246f7cdcf7280 Mon Sep 17 00:00:00 2001 From: Xavier Carron <33637571+xav-car@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:32:47 +0100 Subject: [PATCH 6/8] feat(api): add internal api to delete learner while activate import feature --- .../application/api/learners-api.js | 23 ++++++++ .../unit/application/api/learners-api_test.js | 59 ++++++++++++++++++- 2 files changed, 81 insertions(+), 1 deletion(-) 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 bbdae1752db..3325ee97e01 100644 --- a/api/src/prescription/learner-management/application/api/learners-api.js +++ b/api/src/prescription/learner-management/application/api/learners-api.js @@ -17,3 +17,26 @@ export const hasBeenLearner = async ({ userId }) => { return isLearner; }; + +/** + * delete organization learner before adding import feature + * + * @param {object} params + * @param {number} params.userId - The ID of the user wich request the action + * @param {number} params.organizationId - The ID of the organizationId to find learner to delete + * @returns {Promise} + * @throws TypeError - Throw when params.userId or params.organizationId is not defined + */ +export const deleteOrganizationLearnerBeforeImportFeature = async ({ userId, organizationId }) => { + if (!userId) { + throw new TypeError('userId is required'); + } + + if (!organizationId) { + throw new TypeError('organizationId is required'); + } + + const organizationLearnerIds = await usecases.findOrganizationLearnersBeforeImportFeature({ organizationId }); + + return usecases.deleteOrganizationLearners({ userId, organizationId, organizationLearnerIds }); +}; 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 35a73655c8d..f05fc5474d3 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 @@ -1,4 +1,7 @@ -import { hasBeenLearner } from '../../../../../../src/prescription/learner-management/application/api/learners-api.js'; +import { + deleteOrganizationLearnerBeforeImportFeature, + hasBeenLearner, +} from '../../../../../../src/prescription/learner-management/application/api/learners-api.js'; import { usecases } from '../../../../../../src/prescription/learner-management/domain/usecases/index.js'; import { catchErr, expect, sinon } from '../../../../../test-helper.js'; @@ -29,4 +32,58 @@ describe('Unit | Prescription | learner management | Api | learners', function ( expect(hasBeenLearnerStub).to.have.been.calledOnceWith({ userId }); }); }); + + describe('#deleteOrganizationLearnerBeforeImportFeature', function () { + let findOrganizationLearnersBeforeImportFeatureStub, deleteOrganizationLearnersStub; + + beforeEach(function () { + findOrganizationLearnersBeforeImportFeatureStub = sinon + .stub(usecases, 'findOrganizationLearnersBeforeImportFeature') + .rejects(); + deleteOrganizationLearnersStub = sinon.stub(usecases, 'deleteOrganizationLearners').rejects(); + }); + + it('should throw a "TypeError" when "userId" is not defined', async function () { + // when + const error = await catchErr(deleteOrganizationLearnerBeforeImportFeature)({ + userId: undefined, + organizationId: Symbol('organizationId'), + }); + + // then + expect(error).to.be.instanceOf(TypeError); + expect(findOrganizationLearnersBeforeImportFeatureStub).to.not.have.been.called; + expect(deleteOrganizationLearnersStub).to.not.have.been.called; + }); + + it('should throw a "TypeError" when "organizationId" is not defined', async function () { + // when + const error = await catchErr(deleteOrganizationLearnerBeforeImportFeature)({ + userId: Symbol('userId'), + organizationId: undefined, + }); + + // then + expect(error).to.be.instanceOf(TypeError); + expect(findOrganizationLearnersBeforeImportFeatureStub).to.not.have.been.called; + expect(deleteOrganizationLearnersStub).to.not.have.been.called; + }); + + it('should call usecase given parameter', async function () { + // given + const userId = Symbol('userId'); + const organizationId = Symbol('organizationId'); + const organizationLearnerIds = Symbol('organizationLearnerIds'); + + findOrganizationLearnersBeforeImportFeatureStub.withArgs({ organizationId }).resolves(organizationLearnerIds); + deleteOrganizationLearnersStub.withArgs({ userId, organizationId, organizationLearnerIds }).resolves(); + + // when + await deleteOrganizationLearnerBeforeImportFeature({ userId, organizationId }); + + // then + expect(findOrganizationLearnersBeforeImportFeatureStub).to.have.been.calledOnce; + expect(deleteOrganizationLearnersStub).to.have.been.calledOnce; + }); + }); }); From 2f8e5c10ba754d97946d34f2ba477314d4008d47 Mon Sep 17 00:00:00 2001 From: Xavier Carron <33637571+xav-car@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:26:45 +0100 Subject: [PATCH 7/8] feat(api): update batch usecase with external api --- .../domain/models/OrganizationFeature.js | 9 ++++++- .../add-organization-feature-in-batch.js | 20 +++++++++++--- .../domain/usecases/index.js | 4 ++- .../organization.admin.controller.test.js | 6 +++-- .../domain/models/OrganizationFeature_test.js | 22 ++++++++++++++- .../add-organization-feature-in-batch_test.js | 27 ++++++++++++++++--- 6 files changed, 76 insertions(+), 12 deletions(-) diff --git a/api/src/organizational-entities/domain/models/OrganizationFeature.js b/api/src/organizational-entities/domain/models/OrganizationFeature.js index bc314e7dac1..108f39eca4b 100644 --- a/api/src/organizational-entities/domain/models/OrganizationFeature.js +++ b/api/src/organizational-entities/domain/models/OrganizationFeature.js @@ -1,8 +1,15 @@ class OrganizationFeature { - constructor({ featureId, organizationId, params }) { + #deleteLearner; + constructor({ featureId, organizationId, params, deleteLearner }) { this.featureId = parseInt(featureId, 10); this.organizationId = parseInt(organizationId, 10); this.params = params ? JSON.parse(params) : null; + + this.#deleteLearner = deleteLearner === 'Y'; + } + + get deleteLearner() { + return this.#deleteLearner; } } diff --git a/api/src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js b/api/src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js index 38952dd1dca..dcb2c30424b 100644 --- a/api/src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js +++ b/api/src/organizational-entities/domain/usecases/add-organization-feature-in-batch.js @@ -28,31 +28,43 @@ const organizationFeatureCsvHeader = { name: 'Params', isRequired: false, }), + new CsvColumn({ + property: 'deleteLearner', + name: 'Delete Learner', + isRequired: false, + }), ], }; export const addOrganizationFeatureInBatch = withTransaction( /** * @param {Object} params - A parameter object. - * @param {Number} params.userId - user connected performing action + * @param {Number} params.userId - user connected performing the action * @param {string} params.filePath - path of csv file wich contains organizations and params. * @param {OrganizationFeatureRepository} params.organizationFeatureRepository - organizationRepository to use. * @param {Object} params.dependencies * @returns {Promise} */ - async ({ filePath, organizationFeatureRepository }) => { + async ({ userId, filePath, organizationFeatureRepository, learnersApi }) => { const stream = createReadStream(filePath); const buffer = await getDataBuffer(stream); const csvParser = new CsvParser(buffer, organizationFeatureCsvHeader); const csvData = csvParser.parse(); - const data = csvData.map(({ featureId, organizationId, params }) => { + const data = csvData.map(({ featureId, organizationId, params, deleteLearner }) => { try { - return new OrganizationFeature({ featureId, organizationId, params: params }); + return new OrganizationFeature({ featureId, organizationId, params, deleteLearner }); } catch (err) { throw new FeatureParamsNotProcessable(); } }); + + data.forEach(async ({ organizationId, deleteLearner }) => { + if (deleteLearner) { + await learnersApi.deleteOrganizationLearnerBeforeImportFeature({ userId, organizationId }); + } + }); + return organizationFeatureRepository.saveInBatch(data); }, ); diff --git a/api/src/organizational-entities/domain/usecases/index.js b/api/src/organizational-entities/domain/usecases/index.js index baea1f155a1..fbfab78eb89 100644 --- a/api/src/organizational-entities/domain/usecases/index.js +++ b/api/src/organizational-entities/domain/usecases/index.js @@ -2,6 +2,7 @@ import { dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; import * as organizationTagRepository from '../../../../lib/infrastructure/repositories/organization-tag-repository.js'; +import * as learnersApi from '../../../prescription/learner-management/application/api/learners-api.js'; import * as schoolRepository from '../../../school/infrastructure/repositories/school-repository.js'; import { injectDependencies } from '../../../shared/infrastructure/utils/dependency-injection.js'; import { importNamedExportsFromDirectory } from '../../../shared/infrastructure/utils/import-named-exports-from-directory.js'; @@ -12,10 +13,10 @@ import * as dataProtectionOfficerRepository from '../../infrastructure/repositor import * as organizationFeatureRepository from '../../infrastructure/repositories/organization-feature-repository.js'; import { organizationForAdminRepository } from '../../infrastructure/repositories/organization-for-admin.repository.js'; import { tagRepository } from '../../infrastructure/repositories/tag.repository.js'; - const path = dirname(fileURLToPath(import.meta.url)); /** + * @typedef {import ('../../../prescription/learner-management/application/api/learners-api.js')} learnersApi * @typedef {import ('../../infrastructure/repositories/certification-center.repository.js')} CertificationCenterRepository * @typedef {import ('../../infrastructure/repositories/certification-center-for-admin-repository.js')} CertificationCenterForAdminRepository * @typedef {import ('../../infrastructure/repositories/complementary-certification-habilitation-repository.js')} ComplementaryCertificationHabilitationRepository @@ -34,6 +35,7 @@ const repositories = { organizationForAdminRepository, organizationFeatureRepository, schoolRepository, + learnersApi, organizationTagRepository, tagRepository, }; diff --git a/api/tests/organizational-entities/unit/application/organization/organization.admin.controller.test.js b/api/tests/organizational-entities/unit/application/organization/organization.admin.controller.test.js index 4d2e830652e..62f352f40a9 100644 --- a/api/tests/organizational-entities/unit/application/organization/organization.admin.controller.test.js +++ b/api/tests/organizational-entities/unit/application/organization/organization.admin.controller.test.js @@ -5,11 +5,12 @@ import { domainBuilder, expect, hFake, sinon } from '../../../../test-helper.js' describe('Unit | Organizational Entities | Application | Controller | Admin | organization', function () { describe('#addOrganizationFeatureInBatch', function () { - let filePath, request; + let filePath, request, userId; beforeEach(function () { + userId = Symbol('userId'); filePath = Symbol('filePath'); - request = { payload: { path: filePath } }; + request = { payload: { path: filePath }, auth: { credentials: { userId } } }; sinon.stub(usecases, 'addOrganizationFeatureInBatch').resolves(); }); @@ -24,6 +25,7 @@ describe('Unit | Organizational Entities | Application | Controller | Admin | or // then expect(usecases.addOrganizationFeatureInBatch).to.have.been.calledWithExactly({ + userId, filePath, }); }); diff --git a/api/tests/organizational-entities/unit/domain/models/OrganizationFeature_test.js b/api/tests/organizational-entities/unit/domain/models/OrganizationFeature_test.js index 5de9c7f46f4..00e61fe3677 100644 --- a/api/tests/organizational-entities/unit/domain/models/OrganizationFeature_test.js +++ b/api/tests/organizational-entities/unit/domain/models/OrganizationFeature_test.js @@ -16,7 +16,27 @@ describe('Unit | Organizational Entities | Domain | Model | OrganizationFeature' //when organizationFeature = new OrganizationFeature({ featureId, organizationId, params }); // then - expect(organizationFeature).to.deep.equal({ featureId: 1, organizationId: 2, params: { id: 3 } }); + expect(organizationFeature).to.deep.equal({ + featureId: 1, + organizationId: 2, + params: { id: 3 }, + }); + }); + }); + + describe('#deleteLearner', function () { + it('should activate learner deletion given params', function () { + //when + organizationFeature = new OrganizationFeature({ featureId, organizationId, params, deleteLearner: 'Y' }); + // then + expect(organizationFeature.deleteLearner).to.be.true; + }); + + it('should deactivate learner deletion given params', function () { + //when + organizationFeature = new OrganizationFeature({ featureId, organizationId, params }); + // then + expect(organizationFeature.deleteLearner).to.be.false; }); }); }); diff --git a/api/tests/organizational-entities/unit/domain/usecases/add-organization-feature-in-batch_test.js b/api/tests/organizational-entities/unit/domain/usecases/add-organization-feature-in-batch_test.js index 8b722f2cb8c..23128fd01f0 100644 --- a/api/tests/organizational-entities/unit/domain/usecases/add-organization-feature-in-batch_test.js +++ b/api/tests/organizational-entities/unit/domain/usecases/add-organization-feature-in-batch_test.js @@ -5,19 +5,20 @@ import { DomainTransaction } from '../../../../../src/shared/domain/DomainTransa import { catchErr, createTempFile, expect, removeTempFile, sinon } from '../../../../test-helper.js'; describe('Unit | Domain | UseCases | add-organization-feature-in-batch', function () { - let organizationFeatureRepository, featureId, filePath, csvData; + let organizationFeatureRepository, learnersApi, featureId, filePath, csvData, userId; beforeEach(function () { sinon.stub(DomainTransaction, 'execute').callsFake((callback) => { return callback(); }); - + userId = Symbol('userId'); featureId = 1; csvData = [ new OrganizationFeature({ featureId, organizationId: 123, params: `{ "id": 123 }` }), new OrganizationFeature({ featureId, organizationId: 456, params: `{ "id": 123 }` }), ]; + learnersApi = { deleteOrganizationLearnerBeforeImportFeature: sinon.stub() }; organizationFeatureRepository = { saveInBatch: sinon.stub(), }; @@ -37,9 +38,29 @@ describe('Unit | Domain | UseCases | add-organization-feature-in-batch', functio `, ); // when - await addOrganizationFeatureInBatch({ filePath, organizationFeatureRepository }); + await addOrganizationFeatureInBatch({ filePath, organizationFeatureRepository, learnersApi }); + + expect(organizationFeatureRepository.saveInBatch).to.have.been.calledOnceWithExactly(csvData); + expect(learnersApi.deleteOrganizationLearnerBeforeImportFeature.called).to.be.false; + }); + + it('should call call learner api with correct paramaters', async function () { + // given + filePath = await createTempFile( + 'test.csv', + `Feature ID;Organization ID;Params;Delete Learner + ${featureId};123;{"id": 123}; + ${featureId};456;{"id": 123};Y +`, + ); + // when + await addOrganizationFeatureInBatch({ userId, filePath, organizationFeatureRepository, learnersApi }); expect(organizationFeatureRepository.saveInBatch).to.have.been.calledOnceWithExactly(csvData); + expect(learnersApi.deleteOrganizationLearnerBeforeImportFeature).to.have.been.calledOnceWithExactly({ + userId, + organizationId: 456, + }); }); it('should throw a FeatureParamsNotProcessable error', async function () { 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 8/8] 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();