From 344d34cbfdac0c04585ce43416117b8730800ffd Mon Sep 17 00:00:00 2001 From: Guillaume Date: Mon, 2 Dec 2024 16:27:02 +0100 Subject: [PATCH] fix(api): prevent calling reward user usecase if user id is not provided --- .../application/answers/answer-controller.js | 6 ++- ...ganization-learners-with-participations.js | 10 +++++ api/src/quest/domain/usecases/reward-user.js | 4 ++ .../answers/answer-controller_test.js | 37 ++++++++++++++--- ...ation-learners-with-participations_test.js | 25 +++++++++++- .../unit/domain/usecases/reward-user_test.js | 40 +++++++++++++------ 6 files changed, 102 insertions(+), 20 deletions(-) diff --git a/api/src/evaluation/application/answers/answer-controller.js b/api/src/evaluation/application/answers/answer-controller.js index 51d0fdcc679..0a8f0d2001d 100644 --- a/api/src/evaluation/application/answers/answer-controller.js +++ b/api/src/evaluation/application/answers/answer-controller.js @@ -11,7 +11,11 @@ const save = async function (request, h, dependencies = { answerSerializer, requ const userId = dependencies.requestResponseUtils.extractUserIdFromRequest(request); const locale = dependencies.requestResponseUtils.extractLocaleFromRequest(request); const createdAnswer = await usecases.correctAnswerThenUpdateAssessment({ answer, userId, locale }); - if (!config.featureToggles.isAsyncQuestRewardingCalculationEnabled && config.featureToggles.isQuestEnabled) { + if ( + userId && + !config.featureToggles.isAsyncQuestRewardingCalculationEnabled && + config.featureToggles.isQuestEnabled + ) { await questUsecases.rewardUser({ userId }); } diff --git a/api/src/prescription/organization-learner/domain/usecases/find-organization-learners-with-participations.js b/api/src/prescription/organization-learner/domain/usecases/find-organization-learners-with-participations.js index a4147b66235..d4e292243f0 100644 --- a/api/src/prescription/organization-learner/domain/usecases/find-organization-learners-with-participations.js +++ b/api/src/prescription/organization-learner/domain/usecases/find-organization-learners-with-participations.js @@ -1,5 +1,9 @@ +import Joi from 'joi'; + import { withTransaction } from '../../../../shared/domain/DomainTransaction.js'; +const userIdsSchema = Joi.array().items(Joi.number()); + const findOrganizationLearnersWithParticipations = withTransaction(async function ({ userIds, campaignParticipationOverviewRepository, @@ -7,6 +11,12 @@ const findOrganizationLearnersWithParticipations = withTransaction(async functio libOrganizationLearnerRepository, tagRepository, }) { + const validationResult = userIdsSchema.validate(userIds); + + if (validationResult.error) { + return []; + } + const organizationLearners = ( await Promise.all( userIds.map((userId) => { diff --git a/api/src/quest/domain/usecases/reward-user.js b/api/src/quest/domain/usecases/reward-user.js index 49cb1bdf32c..dd12aa7d84e 100644 --- a/api/src/quest/domain/usecases/reward-user.js +++ b/api/src/quest/domain/usecases/reward-user.js @@ -5,6 +5,10 @@ export const rewardUser = async ({ successRepository, rewardRepository, }) => { + if (!userId) { + return; + } + const quests = await questRepository.findAll(); if (quests.length === 0) { diff --git a/api/tests/evaluation/unit/application/answers/answer-controller_test.js b/api/tests/evaluation/unit/application/answers/answer-controller_test.js index f6cc72f47a5..1e25e4d9f21 100644 --- a/api/tests/evaluation/unit/application/answers/answer-controller_test.js +++ b/api/tests/evaluation/unit/application/answers/answer-controller_test.js @@ -18,6 +18,7 @@ describe('Unit | Controller | answer-controller', function () { extractLocaleFromRequest: sinon.stub(), }; sinon.stub(usecases, 'correctAnswerThenUpdateAssessment'); + sinon.stub(questUsecases, 'rewardUser'); }); describe('#save', function () { @@ -120,15 +121,15 @@ describe('Unit | Controller | answer-controller', function () { usecases.correctAnswerThenUpdateAssessment.resolves(createdAnswer); requestResponseUtilsStub.extractUserIdFromRequest.withArgs(request).returns(userId); requestResponseUtilsStub.extractLocaleFromRequest.withArgs(request).returns(locale); + }); + it('should call the usecase to save the answer', async function () { // when response = await answerController.save(request, hFake, { answerSerializer: answerSerializerStub, requestResponseUtils: requestResponseUtilsStub, }); - }); - it('should call the usecase to save the answer', function () { // then expect(usecases.correctAnswerThenUpdateAssessment).to.have.been.calledWithExactly({ answer: deserializedAnswer, @@ -136,12 +137,23 @@ describe('Unit | Controller | answer-controller', function () { locale, }); }); + it('should serialize the answer', async function () { + // when + response = await answerController.save(request, hFake, { + answerSerializer: answerSerializerStub, + requestResponseUtils: requestResponseUtilsStub, + }); - it('should serialize the answer', function () { // then expect(answerSerializerStub.serialize).to.have.been.calledWithExactly(createdAnswer); }); - it('should return the serialized answer', function () { + it('should return the serialized answer', async function () { + // when + response = await answerController.save(request, hFake, { + answerSerializer: answerSerializerStub, + requestResponseUtils: requestResponseUtilsStub, + }); + // then expect(response.source).to.deep.equal(serializedAnswer); expect(response.statusCode).to.equal(201); @@ -150,7 +162,6 @@ describe('Unit | Controller | answer-controller', function () { context('quests', function () { beforeEach(function () { sinon.stub(config, 'featureToggles'); - sinon.stub(questUsecases, 'rewardUser'); }); context('when quest feature is not enabled', function () { @@ -199,6 +210,22 @@ describe('Unit | Controller | answer-controller', function () { // then expect(questUsecases.rewardUser).to.have.been.calledWith({ userId }); }); + + it('should not call the reward user usecase if userId is not provided', async function () { + // given + config.featureToggles.isQuestEnabled = true; + config.featureToggles.isAsyncQuestRewardingCalculationEnabled = false; + requestResponseUtilsStub.extractUserIdFromRequest.withArgs(request).returns(null); + + // when + await answerController.save(request, hFake, { + answerSerializer: answerSerializerStub, + requestResponseUtils: requestResponseUtilsStub, + }); + + // then + expect(questUsecases.rewardUser).to.not.have.been.called; + }); }); }); }); diff --git a/api/tests/prescription/organization-learner/unit/domain/usecases/find-organization-learners-with-participations_test.js b/api/tests/prescription/organization-learner/unit/domain/usecases/find-organization-learners-with-participations_test.js index 88a69a631ef..86415384e5e 100644 --- a/api/tests/prescription/organization-learner/unit/domain/usecases/find-organization-learners-with-participations_test.js +++ b/api/tests/prescription/organization-learner/unit/domain/usecases/find-organization-learners-with-participations_test.js @@ -1,7 +1,30 @@ -import { _getCampaignParticipationOverviewsWithoutPagination } from '../../../../../../src/prescription/organization-learner/domain/usecases/find-organization-learners-with-participations.js'; +import { + _getCampaignParticipationOverviewsWithoutPagination, + findOrganizationLearnersWithParticipations, +} from '../../../../../../src/prescription/organization-learner/domain/usecases/find-organization-learners-with-participations.js'; +import { DomainTransaction } from '../../../../../../src/shared/domain/DomainTransaction.js'; import { expect, sinon } from '../../../../../test-helper.js'; describe('Unit | UseCase | find-organization-learners-with-participations', function () { + context('#findOrganizationLearnersWithParticipations', function () { + it('should not call libOrganizationLearnerRepository if userIds does not have the expected format', async function () { + // given + sinon.stub(DomainTransaction, 'execute').callsFake((lambda) => lambda()); + const libOrganizationLearnerRepository = { + findByUserId: sinon.stub(), + }; + + // when + await findOrganizationLearnersWithParticipations({ + userIds: [null], + libOrganizationLearnerRepository, + }); + + // then + expect(libOrganizationLearnerRepository.findByUserId).to.not.have.been.called; + }); + }); + context('#_getCampaignParticipationOverviewsWithoutPagination', function () { it('should return all campaign participations', async function () { // given diff --git a/api/tests/quest/unit/domain/usecases/reward-user_test.js b/api/tests/quest/unit/domain/usecases/reward-user_test.js index 5e696be05e8..ae3170b5457 100644 --- a/api/tests/quest/unit/domain/usecases/reward-user_test.js +++ b/api/tests/quest/unit/domain/usecases/reward-user_test.js @@ -8,6 +8,8 @@ describe('Quest | Unit | Domain | Usecases | RewardUser', function () { let successRepository; let rewardRepository; + let dependencies; + beforeEach(function () { userId = 1; @@ -22,6 +24,13 @@ describe('Quest | Unit | Domain | Usecases | RewardUser', function () { successRepository = { find: sinon.stub() }; rewardRepository = { save: sinon.stub(), getByUserId: sinon.stub() }; + + dependencies = { + questRepository, + eligibilityRepository, + successRepository, + rewardRepository, + }; }); context('when there are no quests available', function () { @@ -30,7 +39,21 @@ describe('Quest | Unit | Domain | Usecases | RewardUser', function () { questRepository.findAll.resolves([]); // when - await rewardUser({ userId, questRepository, eligibilityRepository }); + await rewardUser({ userId, ...dependencies }); + expect(eligibilityRepository.find).to.not.have.been.called; + }); + }); + + context('when user id is not provided', function () { + it('should not call eligibility repository', async function () { + // given + const quest = { isEligible: () => false }; + questRepository.findAll.resolves([quest]); + rewardRepository.getByUserId.resolves([]); + eligibilityRepository.find.resolves([]); + + // when + await rewardUser({ userId: null, ...dependencies }); expect(eligibilityRepository.find).to.not.have.been.called; }); }); @@ -46,10 +69,7 @@ describe('Quest | Unit | Domain | Usecases | RewardUser', function () { // when await rewardUser({ userId, - questRepository, - eligibilityRepository, - successRepository, - rewardRepository, + ...dependencies, }); expect(successRepository.find).to.not.have.been.called; }); @@ -72,10 +92,7 @@ describe('Quest | Unit | Domain | Usecases | RewardUser', function () { // when await rewardUser({ userId, - questRepository, - eligibilityRepository, - successRepository, - rewardRepository, + ...dependencies, }); // then @@ -99,10 +116,7 @@ describe('Quest | Unit | Domain | Usecases | RewardUser', function () { // when await rewardUser({ userId, - questRepository, - eligibilityRepository, - successRepository, - rewardRepository, + ...dependencies, }); expect(successRepository.find).to.not.have.been.called; });