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

Rattacher les Animations Collectives aux structures Milo #1319

Closed
wants to merge 4 commits into from

Conversation

Mzem
Copy link
Contributor

@Mzem Mzem commented Dec 7, 2023

No description provided.

@Mzem Mzem marked this pull request as draft December 7, 2023 16:10
@Mzem Mzem force-pushed the migration-structures-milo branch from 5ba8400 to a4c8897 Compare December 7, 2023 20:47
@Mzem Mzem changed the title Migration structures milo WIP Migration structures milo Dec 7, 2023
@Mzem Mzem marked this pull request as ready for review December 7, 2023 20:47
@Mzem Mzem force-pushed the migration-structures-milo branch 4 times, most recently from be94c1f to 6ae8900 Compare December 10, 2023 17:38
@Mzem Mzem force-pushed the migration-structures-milo branch from 6ae8900 to adf51f6 Compare December 10, 2023 17:59
@Mzem Mzem changed the title WIP Migration structures milo Rattacher les Animations Collectives aux structures Milo Dec 11, 2023
Copy link
Contributor

@arthurlbrjc arthurlbrjc left a comment

Choose a reason for hiding this comment

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

Il manque l'ajout de la structure milo au moment de la création d'une animation collective par un conseiller, nan ?

package.json Show resolved Hide resolved
scripts/data-migrations/structures-milo-ac.js Show resolved Hide resolved
scripts/data-migrations/structures-milo-ac.js Show resolved Hide resolved
scripts/data-migrations/structures-milo-ac.js Show resolved Hide resolved
src/domain/rendez-vous/rendez-vous.ts Show resolved Hide resolved
@Mzem Mzem force-pushed the migration-structures-milo branch from 1272729 to dda9def Compare December 11, 2023 13:24
@Mzem Mzem force-pushed the migration-structures-milo branch 2 times, most recently from 896ca73 to 701d110 Compare December 12, 2023 09:55
@Mzem Mzem force-pushed the migration-structures-milo branch 3 times, most recently from f14736e to 296c7ae Compare December 12, 2023 21:52
@Mzem Mzem force-pushed the migration-structures-milo branch from 296c7ae to 208547b Compare December 12, 2023 22:07
Copy link

sonarqubecloud bot commented Dec 12, 2023

Comment on lines +84 to +87
if (RendezVous.estUnTypeAnimationCollective(command.type)) {
const conseillerResult = await this.conseillerMiloRepository.get(
command.idConseiller
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Du coup le conseiller Pass emploi ne peut plus créer d'animation collective.
Peut-être qu'on peut récupérer le conseiller avant le if et checker s'il est MILO ?

const conseiller = await this.conseillerRepository.get(command.idConseiller)
if (!conseiller) { // non trouvé }
if (conseiller.structure === Structure.MILO && RDV.estAnimationCollective()) { // recupérer jeunes milo }
else { // récupérer jeunes }

On peut éventuellement rajouter une méthode Conseiller.Milo.Repository.assertAUneStructureMilo avant de récupérer les jeunes milo pour garder le check qui est fait dans le Conseiller.Milo.Repository sur la présence de la structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oui le conseiller PASS EMPLOI ne devrait pas pouvoir créer une AC, sauf si on lui rajoute à la main une structure Milo. Mais pas envie qu'on fasse du code pour autoriser les ACs aux PASS EMPLOI.

Ta proposition de code ne tient pas compte du fait qu'il y conseillerRepository (lorsque c'est non AC) et conseillerMiloRepository (lorsque c'est AC)
conseillerMiloRepository.get(idConseiller) vérifie que c'est un Milo + qu'il a une structure sinon failure

Comment on lines +88 to +90
if (RendezVous.estUnTypeAnimationCollective(rendezVous.type)) {
jeunes = await this.jeuneMiloRepository.findAll(command.idsJeunes)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Même commentaire, y'a un soucis avec les conseiller Pass emploi. Faut trouver un moyen explicite de tester si c'est une AC créée par un conseiller Milo 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

le script qui va être joué va supprimer toutes les ACs des PASS EMPLOI
AC = MILO

export class JeuneNonLieALAgenceError implements DomainError {
static CODE = 'JEUNE_NON_LIE_A_L_AGENCE'
readonly code: string = JeuneNonLieALAgenceError.CODE
export class JeuneNonLieALaStructureMiloError implements DomainError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Faut garder JeuneNonLieALAgenceError pour les AC des conseillers Pass emploi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pareil pas envie de garder du code pour les PASS EMPLOI, ça ne fait que créer du bruit alors qu'on peut tout tester pour des jeunes et conseillers Milo en recette
YAGNI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

les ACs n'avaient du sens que pour simuler les sessions Milo, le jour ou l'on voudra des ACs spécifiquement pour le PASS EMPLOI on verra

function verifierAgenceJeunes(
jeunes: JeuneDuRendezVous[],
idAgence: string
function verifierStructureMiloJeunes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Faut garder verifierAgenceJeunes pour les ACs des conseillers Pass emploi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same

Copy link
Contributor

@oni-Sk oni-Sk left a comment

Choose a reason for hiding this comment

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

Le premier commit c'est pas un refacto ? je vois feat mais clean up dans la description. D'ailleurs le tout m'a l'air d'être un refacto (certes structurant) mais on change pas le fonctionnel en prod juste la structure interne.

Sinon les impacts conséquents pour des refactos là-dessus et la lisibilité du code, je vote pour travailler sur des modules et de se créer un point dédié entre nous. Puis de le faire baffer avec les autres devs, le métier. Mais dans tous les cas on aura des gros bénéfices rien que pour la manipulation du backend et étaler ça à des optims d'infra spécifiques aux modules qu'on aura découpé.

@@ -27,7 +27,7 @@ import { CreateActionCommandHandler } from './application/commands/action/create
import { CreateEvenementCommandHandler } from './application/commands/create-evenement.command.handler'
import { CreateRechercheCommandHandler } from './application/commands/create-recherche.command.handler'
import { CreateRendezVousCommandHandler } from './application/commands/create-rendez-vous.command.handler'
import { CreerJeuneMiloCommandHandler } from './application/commands/creer-jeune-milo.command.handler'
import { CreerJeuneMiloCommandHandler } from './application/commands/milo/creer-jeune-milo.command.handler'
Copy link
Contributor

Choose a reason for hiding this comment

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

Chaud pour commencer à travailler un découpage en modules plutôt qu'ajouter un autre layer. On pourrait faire un point tech back dédié

Copy link
Contributor Author

Choose a reason for hiding this comment

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

que veux-tu dire par modules ?
là c'est juste une petite organisation pour qu'on se retrouve plus facilement dans les commands/queries

@Mzem Mzem closed this Dec 15, 2023
@Mzem Mzem deleted the migration-structures-milo branch December 15, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants