Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Rafraichir le cache LCMS en asynchrone (pgboss) #9422

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions api/lib/application/cache/cache-controller.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import _ from 'lodash';

import * as learningContentDatasource from '../../../src/shared/infrastructure/datasources/learning-content/datasource.js';
import { sharedUsecases as usecases } from '../../../src/shared/domain/usecases/index.js';
import * as LearningContentDatasources from '../../../src/shared/infrastructure/datasources/learning-content/index.js';
import { logger } from '../../../src/shared/infrastructure/utils/logger.js';

const refreshCacheEntries = function (_, h, dependencies = { learningContentDatasource }) {
dependencies.learningContentDatasource
.refreshLearningContentCacheRecords()
.catch((e) => logger.error('Error while reloading cache', e));
const refreshCacheEntries = async function (_, h) {
await usecases.refreshLearningContentCache();
Comment on lines +6 to +7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pourquoi on fait pas de la "vraie" inversion de dépendance ici ? (en déclarant les usecases en argument du controller)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dans tous les controllers de Pix API, on importe les usecases en dépendance node il me semble. Aujourd'hui il n'y a que les serializers injecté en argument des controlleurs.

Copy link
Contributor

@dlahaye dlahaye Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

il y a des exemples dans le code où les usecases sont injectés

https://github.com/1024pix/pix/blob/dev/api/src/devcomp/application/modules/controller.js
https://github.com/1024pix/pix/blob/dev/api/src/devcomp/application/passages/controller.js

mais oui après avoir checké j'ai l'impression qu'on fait ça seulement chez devcomp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ^^ Injecter les usecases comme le reste me semble bien vis-à-vis de notre archi, peut-être qu'on devrait avoir une guideline globale là-dessus.

Copy link
Contributor

@dlahaye dlahaye Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En fait l'ADR 46 dit bien que c'est une volonté de ne pas injecter les usecases dans les controller mais ce n'est pas dit pourquoi (@frinyvonnick ?)

return h.response({}).code(202);
};

Expand Down
9 changes: 9 additions & 0 deletions api/lib/infrastructure/jobs/lcms/LcmsRefreshCacheJob.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { JobPgBoss } from '../JobPgBoss.js';

class LcmsRefreshCacheJob extends JobPgBoss {
constructor(queryBuilder) {
super({ name: 'LcmsRefreshCacheJob', retryLimit: 0 }, queryBuilder);
}
}

export { LcmsRefreshCacheJob };
18 changes: 18 additions & 0 deletions api/lib/infrastructure/jobs/lcms/LcmsRefreshCacheJobHandler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { LcmsRefreshCacheJob } from './LcmsRefreshCacheJob.js';

class LcmsRefreshCacheJobHandler {
constructor({ learningContentDatasource, logger }) {
this.logger = logger;
this.learningContentDatasource = learningContentDatasource;
}

async handle() {
await this.learningContentDatasource.refreshLearningContentCacheRecords();
}

get name() {
return LcmsRefreshCacheJob.name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Il n'y a pas de propriété statique name sur LcmsRefreshCacheJob, ça fonctionne quand même ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est JobPgBoss qui stocke la variable, dans le super de LcmsRefreshCacheJob

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nlepage c'est pas une propriété de JobPgBoss ?

Copy link
Contributor Author

@bpetetot bpetetot Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En Javascript, la propriété statique name d'une classe ou fonction retourne son nom:

class LcmsRefreshCacheJob {}
console.log(LcmsRefreshCacheJob.name) // returns 'LcmsRefreshCacheJob'

Ce qui n'est pas top c'est qu'on initialise également une propriété name du job dans le constructeur (qui peut potentiellement différer). Et il est probable que si elle est différente, le job ne se lance pas. C'est le cas pour tous les jobs. 🤔

class LcmsRefreshCacheJob extends JobPgBoss {
  constructor(queryBuilder) {
    super({ name: 'LcmsRefreshCacheJob', retryLimit: 0 }, queryBuilder);
  }
}

Ça serait potentiellement un point à améliorer pour l'équipe "Évènementiel" des tech days.

}
}

export { LcmsRefreshCacheJobHandler };
3 changes: 3 additions & 0 deletions api/src/shared/domain/usecases/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { dirname, join } from 'node:path';
import { fileURLToPath } from 'node:url';

import { knex } from '../../../../db/knex-database-connection.js';
import { LcmsRefreshCacheJob } from '../../../../lib/infrastructure/jobs/lcms/LcmsRefreshCacheJob.js';
import * as complementaryCertificationBadgeRepository from '../../../certification/complementary-certification/infrastructure/repositories/complementary-certification-badge-repository.js';
import * as badgeRepository from '../../../evaluation/infrastructure/repositories/badge-repository.js';
import { injectDependencies } from '../../../shared/infrastructure/utils/dependency-injection.js';
Expand All @@ -15,6 +17,7 @@ const usecasesWithoutInjectedDependencies = {
const dependencies = {
complementaryCertificationBadgeRepository,
badgeRepository,
lcmsRefreshCacheJob: new LcmsRefreshCacheJob(knex),
};

const sharedUsecases = injectDependencies(usecasesWithoutInjectedDependencies, dependencies);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const refreshLearningContentCache = async function ({ lcmsRefreshCacheJob }) {
await lcmsRefreshCacheJob.schedule();
};
export { refreshLearningContentCache };
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { refreshLearningContentCache } from '../../../../../src/shared/domain/usecases/refresh-learning-content-cache.js';
import { expect, sinon } from '../../../../test-helper.js';

describe('Unit | Domain | Usecases | Refresh Learning Content Cache', function () {
it('should use repository to schedule reefresh job', async function () {
// given
const lcmsRefreshCacheJob = { schedule: sinon.stub() };

// when
await refreshLearningContentCache({ lcmsRefreshCacheJob });

// then
expect(lcmsRefreshCacheJob.schedule).to.have.been.calledOnce;
});
});
43 changes: 4 additions & 39 deletions api/tests/unit/application/cache/cache-controller_test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { cacheController } from '../../../../lib/application/cache/cache-controller.js';
import { sharedUsecases as usecases } from '../../../../src/shared/domain/usecases/index.js';
import * as learningContentDatasources from '../../../../src/shared/infrastructure/datasources/learning-content/index.js';
import { logger } from '../../../../src/shared/infrastructure/utils/logger.js';
import { expect, hFake, sinon } from '../../../test-helper.js';

describe('Unit | Controller | cache-controller', function () {
Expand Down Expand Up @@ -79,53 +79,18 @@ describe('Unit | Controller | cache-controller', function () {
});

describe('#refreshCacheEntries', function () {
const request = {};
let learningContentDatasourceStub;

beforeEach(function () {
learningContentDatasourceStub = { refreshLearningContentCacheRecords: sinon.stub() };
});

context('nominal case', function () {
it('should reply with http status 202', async function () {
// given
const numberOfDeletedKeys = 0;
learningContentDatasourceStub.refreshLearningContentCacheRecords.resolves(numberOfDeletedKeys);

// when
const response = await cacheController.refreshCacheEntries(request, hFake, {
learningContentDatasource: learningContentDatasourceStub,
});

// then
expect(learningContentDatasourceStub.refreshLearningContentCacheRecords).to.have.been.calledOnce;
expect(response.statusCode).to.equal(202);
});
});

context('error case', function () {
let response;

beforeEach(async function () {
// given
sinon.stub(logger, 'error');
learningContentDatasourceStub.refreshLearningContentCacheRecords.rejects();
sinon.stub(usecases, 'refreshLearningContentCache').resolves();

// when
response = await cacheController.refreshCacheEntries(request, hFake, {
learningContentDatasource: learningContentDatasourceStub,
});
});
const response = await cacheController.refreshCacheEntries({}, hFake);

it('should reply with http status 202', async function () {
// then
expect(usecases.refreshLearningContentCache).to.have.been.calledOnce;
expect(response.statusCode).to.equal(202);
});

it('should call log errors', async function () {
// then
expect(logger.error).to.have.been.calledOnce;
});
});
});
});
7 changes: 7 additions & 0 deletions api/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { SendSharedParticipationResultsToPoleEmploiHandler } from './lib/infrast
import { SendSharedParticipationResultsToPoleEmploiJob } from './lib/infrastructure/jobs/campaign-result/SendSharedParticipationResultsToPoleEmploiJob.js';
import { scheduleCpfJobs } from './lib/infrastructure/jobs/cpf-export/schedule-cpf-jobs.js';
import { JobQueue } from './lib/infrastructure/jobs/JobQueue.js';
import { LcmsRefreshCacheJob } from './lib/infrastructure/jobs/lcms/LcmsRefreshCacheJob.js';
import { LcmsRefreshCacheJobHandler } from './lib/infrastructure/jobs/lcms/LcmsRefreshCacheJobHandler.js';
import { MonitoredJobQueue } from './lib/infrastructure/jobs/monitoring/MonitoredJobQueue.js';
import { ComputeCertificabilityJob } from './lib/infrastructure/jobs/organization-learner/ComputeCertificabilityJob.js';
import { ComputeCertificabilityJobHandler } from './lib/infrastructure/jobs/organization-learner/ComputeCertificabilityJobHandler.js';
Expand All @@ -27,6 +29,7 @@ import { ImportOrganizationLearnersJob } from './src/prescription/learner-manage
import { ImportOrganizationLearnersJobHandler } from './src/prescription/learner-management/infrastructure/jobs/ImportOrganizationLearnersJobHandler.js';
import { ValidateOrganizationImportFileJob } from './src/prescription/learner-management/infrastructure/jobs/ValidateOrganizationImportFileJob.js';
import { ValidateOrganizationImportFileJobHandler } from './src/prescription/learner-management/infrastructure/jobs/ValidateOrganizationImportFileJobHandler.js';
import * as learningContentDatasource from './src/shared/infrastructure/datasources/learning-content/datasource.js';
import { logger } from './src/shared/infrastructure/utils/logger.js';

async function startPgBoss() {
Expand Down Expand Up @@ -73,6 +76,10 @@ export async function runJobs(dependencies = { startPgBoss, createMonitoredJobQu
const pgBoss = await dependencies.startPgBoss();
const monitoredJobQueue = dependencies.createMonitoredJobQueue(pgBoss);

monitoredJobQueue.performJob(LcmsRefreshCacheJob.name, LcmsRefreshCacheJobHandler, {
learningContentDatasource,
logger,
});
monitoredJobQueue.performJob(
ScheduleComputeOrganizationLearnersCertificabilityJob.name,
ScheduleComputeOrganizationLearnersCertificabilityJobHandler,
Expand Down