-
Notifications
You must be signed in to change notification settings - Fork 7
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
Paramétrage diffusion préavis – Créer la liste des unités recevant des préavis #3794
base: master
Are you sure you want to change the base?
Paramétrage diffusion préavis – Créer la liste des unités recevant des préavis #3794
Conversation
|
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
9429425 | Triggered | Generic Password | 64ebd64 | infra/docker/.env.local.defaults | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quelques commentaires!
val missionsUrl = "${monitorenvProperties.url}/api/v1/control_units" | ||
|
||
try { | ||
apiClient.httpClient.get(missionsUrl).body() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: En fait ici je pense qu'on devrait avoir un objet LegacyControlUnit
(une copie) qui fait partie de l'infra de /monitorenv
et copie dans l'entité métier LegacyControlUnit
, ça permet d'assurer le découplage en mode clean archi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Un peu comme c'est fait avec MissionDataResponse
) | ||
|
||
const goBackToList = () => { | ||
navigate(`/backoffice${BACK_OFFICE_MENU_PATH[BackOfficeMenuKey.PRIOR_NOTIFICATION_SUBSCRIBER_LIST]}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
navigate(`/backoffice${BACK_OFFICE_MENU_PATH[BackOfficeMenuKey.PRIOR_NOTIFICATION_SUBSCRIBER_LIST]}`) | |
navigate(`/${paths.backoffice}${BACK_OFFICE_MENU_PATH[BackOfficeMenuKey.PRIOR_NOTIFICATION_SUBSCRIBER_LIST]}`) |
?
@@ -33,6 +33,7 @@ type VesselSearchProps = Omit<InputHTMLAttributes<HTMLInputElement>, 'defaultVal | |||
onClickOutsideOrEscape?: () => Promisable<void> | |||
onInputClick?: () => Promisable<void> | |||
} | |||
/** @deprecated Use `@features/Vessel/components/VesselSearch` instead. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: On ne paut pas remplacer les autres utilisations de ce composant juste en remplacant defaultValue
par value
? Je n'arrive pas à voir ce qui a été changé hormis ça
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alors ce qui a été changé :
- On passe de uncontrolled à controlled value comme tu l'as dit en utilisant
useFieldControl()
ce qui retire les useEffect du composant (et le remplacent donc par l'unique useEffect interne àuseFieldControl()
) - Plus d'appels aux slices Redux dans le composant interne
- Les navires "cachés" en local (sur la carte pour l'instant) sont maintenant à passer via la prop
cachedVesselIdentities
- On ne manipule plus que des
VesselIdentity
à l'intérieur de ce composant, aucun autre type similaire ou dérivé (=> Plus de transformation viagetOnlyVesselIdentityProperties
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne me sens pas de prendre tout de suite le risque de tout passer vers le "nouveau" composant sur cette PR pour ne rien casser et prendre le temps sur une autre PR de cleaner éventuellement un peu plus ce composant (plus de key interne par exemple) + bien le tester (dont tests unitaires à (ré)-écrire/compléter).
Ca te va ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merci pour les détails.
"Les navires "cachés" en local": tu parles des quelle feature ? je ne vois pas où c'ets utilisé (je ne peux pas faire de recherche avec cette PR, pas mal de fichiers fermés sur Github par défaut).
Sinon dans l'idée ça me va de tout passer dans un 2ème temps, mais j'aimerai bien qu'on ne tarde pas trop quoi, pour éviter d'avoir deux composants à maintenir
const escapeFromKeyboard = useEscapeFromKeyboard() | ||
const clickedOutsideComponent = useClickOutsideWhenOpenedWithinRef(wrapperRef, isExtended, baseRef) | ||
|
||
const { controlledOnChange, controlledValue: selectedVessel } = useFieldControl(value, onChange) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Qu'est-ce que ça apporte ce useFieldControl
par rapport au useState
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le useFieldControl
permet de transformer un composant "field / input" qui utilise un useState()
interne pour suivre sa value
tout en lui permettant de d'être en mode "controlled value".
En clair si la value
passée en prop est modifiée, cette value
va écraser la value du useState
interne qui reprendra contrôle jusqu'à ce que la value soit éventuellement modifiée de nouveau plus tard.
C'est à peu près ce que tu avais déjà fait, mais de manière plus normalisée (controlledValue
+ controlledOnChange
) car je l'utilise déjà sur monitor-ui.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, je n'avais pas connaissance de ce hook.
Je ne suis pas très fan de controlledOnChange
, je ne comprends pas ce que ça veut dire...
Tu peux à minima renommer le nom dans ce fichier ?
data: controlUnits, | ||
error, | ||
isLoading | ||
} = useGetControlUnitsQuery(undefined, RTK_FIVE_MINUTES_POLLING_QUERY_OPTIONS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: le polling ne casse pas le scroll position quand on scrolle ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah oui il faut que je vérifie ça (aussi sur les pno forms car j'ai oublié de le faire :X)
@@ -23,13 +22,11 @@ export function BackofficePage() { | |||
<NamespaceContext.Provider value={LayerSliceNamespace.backoffice}> | |||
<BackofficeMode isBackoffice /> | |||
|
|||
<LegacyRsuiteComponentsWrapper> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enfin?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai pris le risque :D !
@@ -17,7 +17,7 @@ MONITORFISH_VERSION=0.0.0 | |||
#===================================================================== | |||
# VERSION | |||
|
|||
MONITORENV_VERSION=v1.37.0 | |||
MONITORENV_VERSION=v1.45.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bien vu, c'est bien de mettre à jour ça régulièrement
fun execute(id: Int): PriorNotificationSubscriber { | ||
val allFleetSegments = fleetSegmentRepository.findAll() | ||
val allPorts = portRepository.findAll() | ||
val allVessels = vesselRepository.findAll() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Pourquoi ne pas utiliser fun findVesselsByIds(ids: List<Int>): List<Vessel>
avec les vesselId
?
Moins on fait d'enrichissement avec des findAll
, mieux on se porte je trouve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bien vu !
private val portRepository: PortRepository, | ||
private val vesselRepository: VesselRepository, | ||
) { | ||
fun execute( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: On ne pouvait pas faire une requête SQL avec des JOIN pour ne pas faire ces findAll + enrichissement + filters ?
Surtout qu'on ne remonte pas ces données au front
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mais c'est beaucoup plus de code à écrire pour un gain de perf minime non ?
Il va falloir écrire une requête SQL conséquentes incluants (ou non) les filtres + transformer toutes les données via des entités relationnelles...
J'avoue que ça me désespère de faire ça pour une feature backoffice qui sera utilisée quelques fois par mois (je sens que je vais en avoir pour 1/2 journée si ce n'est pas plus pour écrire ça + les tests JPA qui vont avec).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok +1 !
): PriorNotificationSubscriber { | ||
val allFleetSegments = fleetSegmentRepository.findAll() | ||
val allPorts = portRepository.findAll() | ||
val allVessels = vesselRepository.findAll() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, pour le findAll des vessels
348b13b
to
4df84b9
Compare
Quality Gate passedIssues Measures |
Linked issues
VesselSearch
: