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

[TECH] Regrouper la gestion des centres (PIX-12462). #8892

Closed
wants to merge 7 commits into from

Conversation

mcampourcy
Copy link
Contributor

@mcampourcy mcampourcy commented May 13, 2024

🦄 Problème

On répète à de nombreux endroits les mêmes instructions dans le cadre de la Séparation Pix / Pix+, notamment de récupérer pour un centre donné ses features (CAN_REGISTER_FOR_A_COMPLEMENTARY_CERTIFICATION_ALONE)

On constate déjà à ce jour

Un objet Center.js dans api/src/certification/enrolment/domain/models/Center.js qui représente un centre de certif de manière générique

Un objet CertificationCenterForAdmin dans api/lib/domain/models/CertificationCenterForAdmin.js qui est un aggregat entre les informations d’un centre + le DPO officer

Un objet AllowedCertificationCenterAccess dans api/lib/domain/read-models/AllowedCertificationCenterAccess.js qui est un aggregat entre un centre, des tags et des informations supplémentaires lié au SCO

On voudrait mettre en cohérence ce code pour éviter de refaire un nombre identique de fois les mêmes opérations qui sont faites dans les repositories.

🤖 Proposition

🌈 Remarques

💯 Pour tester

@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@mcampourcy mcampourcy force-pushed the pix-12462-group-centers-management branch 6 times, most recently from 923911f to c133f70 Compare May 14, 2024 09:47
@AndreiaPena AndreiaPena changed the title WIP - Group center management [TECH] Regrouper la gestion des centres (PIX-12462). May 15, 2024
@mcampourcy mcampourcy force-pushed the pix-12462-group-centers-management branch from c133f70 to 21ba9a2 Compare May 16, 2024 08:05
@mcampourcy mcampourcy force-pushed the pix-12462-group-centers-management branch 2 times, most recently from 92e6535 to 7893745 Compare May 16, 2024 13:57
@mcampourcy mcampourcy marked this pull request as ready for review May 16, 2024 14:54
@mcampourcy mcampourcy force-pushed the pix-12462-group-centers-management branch 2 times, most recently from 67bbf96 to ac779d2 Compare May 21, 2024 14:21
Copy link
Contributor

@aceol aceol left a comment

Choose a reason for hiding this comment

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

Je suis pas certain que mettre "center" en dehors du scope certif soit une bonne chose (lib + dans les message d'erreur).
Sinon OK :D

createdAt: 'certification-centers.createdAt',
updatedAt: 'certification-centers.updatedAt',
isV3Pilot: 'certification-centers.isV3Pilot',
isComplementaryAlonePilot: knex.raw(
Copy link
Contributor

Choose a reason for hiding this comment

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

On peut en avoir plusieurs? je ne comprends pas pourquoi le count

* @param {Array} centerList - List of certification centers.
* @returns {Promise<Array>} - List of allowed center accesses.
*/
const getAllowedCenterAccesses = async function (centerList) {
Copy link
Contributor

Choose a reason for hiding this comment

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

On ne se sert que des ids, peut etre qu'on ne devrait prendre en parametre que cela

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Faire un centerIdList ? Le problème c'est qu'on se sert des autres informations + bas, dans le toDomain

@mcampourcy mcampourcy force-pushed the pix-12462-group-centers-management branch 2 times, most recently from 2d8e25e to ff55052 Compare May 23, 2024 09:32
@mcampourcy mcampourcy requested review from Steph0 and aceol May 23, 2024 12:40
@mcampourcy mcampourcy force-pushed the pix-12462-group-centers-management branch from ff55052 to e72d286 Compare May 23, 2024 12:44
@@ -110,7 +110,7 @@ describe('Integration | Repository | CertificationPointOfContact', function () {
type: certificationCenter.type,
isV3Pilot: certificationCenter.isV3Pilot,
habilitations: [],
isComplementaryAlonePilot: false,
// isComplementaryAlonePilot: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

On dé-commente ou on supprime ? :)

@Steph0 Steph0 force-pushed the pix-12462-group-centers-management branch from e72d286 to ac5bf5b Compare May 28, 2024 08:05
@mcampourcy mcampourcy force-pushed the pix-12462-group-centers-management branch from ac5bf5b to 3e6b408 Compare May 28, 2024 12:21
@Steph0
Copy link
Contributor

Steph0 commented May 28, 2024

J'ai cassé la RA, vu avec @mcampourcy je ferme et j'ouvre une autre PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants