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

Thomas/backend env actions attached to reportings #887

Conversation

thoomasbro
Copy link
Collaborator

No description provided.

@@ -1,4 +1,4 @@
package fr.gouv.cacem.monitorenv.domain.entities.mission
package fr.gouv.cacem.monitorenv.domain.entities.mission.envAction
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
package fr.gouv.cacem.monitorenv.domain.entities.mission.envAction
package fr.gouv.cacem.monitorenv.domain.entities.mission.action

J'avoue que depuis le début je ne suis pas super fan de ce nommage, une action dans ce repo est pas définition une action Env, car il n'y en a aucune autre.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Il me semble qu'au début on n'était pas sûrs de ce qu'allait stocker la base monitorEnv, et j'imaginais qu'il pouvait y avoir d'autres types d'actions. Ok pour renommer en envAction en action. Est ce qu'on renomme aussi la table en db ? (je dirais plutot non)

@@ -0,0 +1,7 @@
package fr.gouv.cacem.monitorenv.domain.entities.mission.envAction.envActionControl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ici, le envActionControl fait déjà partie du package envAction donc ça me semble une répétition de naming de package

reportingRepository.attachReportingsToMission(
attachedReportingIds ?: listOf(),
savedMission.mission.id,
)

return missionRepository.findById(savedMission.mission.id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pourquoi ce findBy avec que save renvoie la mission ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

c'est une confusion de ma part entre le useCase et la couche JPA.

private val departmentRepository: IDepartmentAreaRepository,
private val facadeRepository: IFacadeAreasRepository,
private val missionRepository: IMissionRepository,
private val reportingRepository: IReportingRepository,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
private val reportingRepository: IReportingRepository,
private val reportingRepository: IReportingRepository,
private val createOrUpdateMission: CreateOrUpdateMission,

Je pense que CreateOrUpdateMissionWithAttachedReporting devrait utiliser le use-case CreateOrUpdateMission en dépendance injectée, pour éviter de répéter du code et pour faciliter le refacto par la suite.
Ce use-case deviendra plus simple, avec seulement les l.96 à l.106.

@@ -290,11 +298,14 @@ class MissionsControllerITests {
isGeometryComputedFromControls = false,
),
)
val envAction = EnvActionControlEntity(
val envAction = MissionEnvActionDataInput(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dans ce test j'aurais bien testé le when de MissionEnvActionDataInput (avec un Mockito.verify() ici ou alors un test unitaire de MissionEnvActionDataInput), c'est fréquent d'oublier des propriétés dans ce type de mapper

…porting in form when user attach a reporting to a control
@claire2212 claire2212 force-pushed the claire/attach-reporting-to-mission branch from 580ed2f to f147a0a Compare October 20, 2023 08:42
@claire2212 claire2212 force-pushed the thomas/backend-envActionsAttachedToReportings branch from 97e6e7d to e352bbc Compare October 20, 2023 09:39
@claire2212 claire2212 force-pushed the thomas/backend-envActionsAttachedToReportings branch from ee5f2b2 to 414fc52 Compare October 20, 2023 12:02
@claire2212 claire2212 force-pushed the claire/attach-reporting-to-mission branch 2 times, most recently from 4695f31 to c127f7a Compare October 30, 2023 07:48
@thoomasbro thoomasbro force-pushed the claire/attach-reporting-to-mission branch 3 times, most recently from b1593ea to 7e314a8 Compare November 2, 2023 22:19
@thoomasbro
Copy link
Collaborator Author

Cherrypicked in claire/attach-reporting-to-mission

@thoomasbro thoomasbro closed this Nov 3, 2023
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