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

[Alertes] Ajout de l'alerte de FAR manquant pour 48h et de la suspenstion d'alertes #2383

Merged
merged 27 commits into from
Sep 15, 2023

Conversation

VincentAntoine
Copy link
Collaborator

@VincentAntoine VincentAntoine commented Jul 22, 2023

Linked issues

@VincentAntoine VincentAntoine force-pushed the vincent/create_missing_far_48h_alert branch 2 times, most recently from 85e7be6 to 521e8e0 Compare July 22, 2023 16:06
@sonarcloud
Copy link

sonarcloud bot commented Jul 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@VincentAntoine
Copy link
Collaborator Author

@louptheron @AdelineCelier
J'ai ajouté un paramètre au flow missing_far_alerts permettant de configurer le nombre de jours consécutifs en mer sans FAR au bout duquel on remonte une alerte, et créé un nouveau type d'alerte MISSING_FAR_48_HOURS_ALERT en plus de la MISSING_FAR_ALERT existant, de sorte qu'on peut faire remonter :

  • des MISSING_FAR_ALERT avec nombre de jours consécutifs sans FAR = 1
  • des MISSING_FAR_48_HOURS_ALERT avec nombre de jours consécutifs sans FAR = 2

Et j'ai aussi créé un flow validate_pending_alerts qui valide toutes les alertes en cours d'un type donné, et je l'utilise pour valider automatiquement les MISSING_FAR_ALERT juste après qu'elles ont été créées par le flow de missing_far_alerts. Ce flow utilise le endpoint des alertes opérationnelle sur bff/v1/id/operational_alerts donc il faudra peut-être le passer sur l'API publique api/v1 @louptheron ?

Entre la création des MISSING_FAR_ALERT et leur validation il pourra y avoir quelques minuntes entre le run de missing_far_alerts et le run de validate_pending_alerts, donc il faut peut-être retirer les MISSING_FAR_ALERT de la liste des types d'alertes remontées par le backend pour éviter qu'elles remontent pendant cet intervalle.

Il reste aussi l'admin des navires exemptés JPE à faire.

@louptheron louptheron force-pushed the vincent/create_missing_far_48h_alert branch 3 times, most recently from 9bb8b8f to bb42b80 Compare August 28, 2023 09:01
@louptheron louptheron changed the title Add MISSING_FAR_48_HOURS_ALERT alert [Alertes] Ajout de l'alerte de FAR manquant pour 48h et de la suspenstion d'alertes Aug 29, 2023
@louptheron louptheron marked this pull request as ready for review August 29, 2023 15:31
@louptheron louptheron force-pushed the vincent/create_missing_far_48h_alert branch from dfb4e48 to 742d6b3 Compare August 29, 2023 15:35
@louptheron louptheron force-pushed the vincent/create_missing_far_48h_alert branch 3 times, most recently from d187af3 to df8e4c3 Compare September 1, 2023 13:31
Copy link
Member

@ivangabriele ivangabriele left a comment

Choose a reason for hiding this comment

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

Sympa la continuation de la refacto de la gestion du sous-menu de la SideWindow :).

const [searchQuery, setSearchQuery] = useState<string>()
const [isAddSilencedAlertDialogOpen, setIsAddSilencedAlertDialogOpen] = useState(false)

const fuse = useMemo(() => new Fuse(silencedAlerts, PENDING_ALERTS_SEARCH_OPTIONS), [silencedAlerts])
Copy link
Member

Choose a reason for hiding this comment

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

On n'utilise pas la lib CustomSearch de monitor-ui ?

import type { SilencedAlertData } from '../../../../domain/entities/alerts/types'

/**
* Require at least one of the specified fields to be required
Copy link
Member

Choose a reason for hiding this comment

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

Cette partie est assez complexe.

  • Je typerais le contexte : atLeastOneRequired(this: this: ObjectSchema<{ [key: string]: any }>, list: string[], message: string)
  • Je nommerais le param list => requiredFields
  • Je splitterais le code en deux étapes pour le rendre plus clair (en sortant les fonctions utilisées dans les reducers pour les décrire et les typer, ce qui évitera peut-être en bonus les @ts-ignore)
  • Description => "Require at least one of the specified fields to be present."
  • Je ne suis pas sûr de bien comprendre (l'utilité de) list.reduce((acc, item, idx, all) => [...acc, ...all.slice(idx + 1).map(i => [item, i])], [])
  • Ca mérite une petit exemple explicatif et un test unitaire ^^

Copy link
Collaborator

Choose a reason for hiding this comment

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

Disclaimer: je n'ai pas écris ce code ^^
J'ai rajouté un test pour valider son fonctionnement mais je ne comprends pas (comme toi) la dernière ligne, donc je propose de le laisser en mode "black box", ce code devrait clairement faire partie d'une lib, et la logique de Yup est assez complexe je trouve.

Copy link
Member

@ivangabriele ivangabriele Sep 15, 2023

Choose a reason for hiding this comment

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

Je te propose alors de simplement mettre à jour la description (EDIT tu l'as déjà fait pardon !) et de mettre un todo pour qu'éventuellement on voit ça plus tard si ça te va ^^. Je suis d'accord que ça deviendra une sorte de lib ou plutôt un util dans monitor-ui.

@louptheron louptheron force-pushed the vincent/create_missing_far_48h_alert branch from edd5777 to 888bd18 Compare September 15, 2023 09:28
@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 23 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@louptheron louptheron merged commit 377b7ed into master Sep 15, 2023
24 checks passed
@louptheron louptheron deleted the vincent/create_missing_far_48h_alert branch September 15, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants