loicsteinmetz / alternbot-app Goto Github PK

SPOILER: Ça concerne un détail insignifiant et on est vraiment en plein dans le pinaillage.

J'ai déjà remarqué à plusieurs endroits dans les tests que les champs expected et actual des certaines assertions étaient inversés.
Un exemple ici:
https://github.com/B4va/alternbot-app/blob/d7ba548f2a49eef9b9a34f39f55bab995024100c/src/test/java/process/schedule/data/TestIcalMappingProcess.java#L24 https://github.com/B4va/alternbot-app/blob/d7ba548f2a49eef9b9a34f39f55bab995024100c/src/test/java/process/schedule/data/TestIcalMappingProcess.java#L128

Ça donne une sortie "inversée" lors de l'exécution du test:

expected: <sessions.get(0).getEnd()> but was: <10:00>
Comparison Failure: 
Expected :sessions.get(0).getEnd()
Actual   :10:00

https://github.com/B4va/alternbot-app/blob/b6ddc6d6ef2c7e2b1707367bcf4d4fa3f431de46/src/main/java/controllers/workers/DailyWorker.java#L12

Ajouter la possibilité d'exclure des jours de la semaine.

La mise à jour des cours est réalisée en comparant les données récupérées aux données enregistrées. Bon fonctionnement pour un nouveau cours sur le créneau d'un ou plusieurs cours existant, mais aucune gestion des cours simplement supprimés (sans cours à la place).

Solution : Supprimer les Session ne correspondant à aucun cours dont les données ont été récupérée et ajouter ce cours à la liste de SessionChange pour émettre une alerte.

https://github.com/B4va/alternbot-app/blob/367ed05015692b7ce6e2c7e5546bd65c6acf9b75/src/test/java/models/TestSession.java#L4

Pas urgent parce qu'il suffit d'enlever l'import du fichier pour résoudre le problème mais peut être à faire à l'occasion.

Je pense que la classe process.commons.Publication mértirait d'être refactorisée.

À mon sens, on devrait seulement exposer les méthodes sendMessage et sendFile, le reste des méthodes devrait être passé en private
Dans ce cas, on pourrait remplacer l'unique appel à sendLongMessage directement par la seule instruction qu'elle contient:
https://github.com/B4va/alternbot-app/blob/17fe288fa3a2cc2e73f5848518f3d4ca6eebe5ad/src/main/java/process/commons/Publication.java#L43 https://github.com/B4va/alternbot-app/blob/17fe288fa3a2cc2e73f5848518f3d4ca6eebe5ad/src/main/java/process/commons/Publication.java#L86-L88

Les méthodes suivantes pourraient être fusionnés en une seule:

// Vérifie l’existence d'un channel sur un serveur
private boolean hasChannel(Guild guild, String channel);
// Crée un channel sur un serveur
private boolean createChannel(Guild guild, String channel);
// Renvoie un objet TextChannel si un channel est trouvé sur le serveur donné, sinon renvoie null
private TextChannel getChannel(Guild guild, String channel);

Actuellement, on vérifie deux fois de suite que le channel existe. Une fois dans les fonctions sendMessage et sendFile en appelant hasChannel et une autre fois dans les méthodes privées doSendMessage et doSendFile en appelant getChannel et en comparant son résultat avec null.

Je propose d'implémenter cette unique méthode en remplacement:

private TextChannel getOrCreateChannel(Guild guild, String channel, boolean createIfNotExists);

// Vérifie l’existence d'un channel sur un serveur
isNull(getOrCreateChannel(guild, "super-channel", false));
// Crée un channel sur un serveur
getOrCreateChannel(guild, "super-channel", true);
// Renvoie un objet TextChannel si un channel est trouvé sur le serveur donné, sinon renvoie null
getOrCreateChannel(guild, "super-channel", false);

Ces changements permettraient d'alléger le code et les tests associés.

Tous les jours, vérifier pour chaque Schedule enregistré en base que le l'url vers l'emploi du temps est toujours valide. S'il ne l'est pas, envoyer une notification ; par exemple :

L'URL saisi ne permet pas de récupérer l'emploi du temps. 
Vous pouvez modifier l'URL via la commande suivante : $url <URL>

Permet de gérer le paramétrage sans avoir à modifier le code. Exemple : heure de publication quotidienne de l'edt.

Exemple : Configurer l'heure par défaut des rappels d'edt et des tâches en cours via une variable d'environnement.

Permet d'éviter d'avoir à modifier le code pour la configuration. Conserver éventuellement une valeur par défaut dans le code.

Exemple : Alternbot est ajouté au serveur A et B ; la commande $edt est lancée sur le serveur B -> l'edt est publié sur le serveur A

Actuellement, le message laisse penser que le rappel concerne toutes les tâches alors que seules les tâches dont l'échéance est à J+3 sont listées.

Problème : les cours ne portent pas d'id.
En consultant les données de l'iut il est possible de détecter une suppression et un ajout mais pas de détecter qu'il s'agit du même cours qui a en fait juste été déplacé.

Voir si une solution est possible.

Organisation des packages qui pourrait être revue et optimisée.

A gérer dans Publication.

La saisie d'une date erronée devrait renvoyer une erreur.

Ne pas publier le message automatique "Aucun cours prévu ce jour." suite aux retours négatifs des utilisateurs.

• Conception
• Déploiement
• CI
• Environnements
• ...

Problème : Lorsque ScheduleUpdateProcess récupère les données d'un nouvel edt, tous les cours sont considérés comme nouveaux et provoque donc l'envoi d'une alerte.

Il semblerait que l'on soit passé à côté de ça dans la v1

https://github.com/B4va/alternbot-app/blob/367ed05015692b7ce6e2c7e5546bd65c6acf9b75/src/main/java/utils/EnvironmentVariablesUtils.java#L124-L128

Si une variable demandée n'est pas trouvée dans les variables d'environnement du système, la fonction getFromFile est appelée mais celle-ci génère une exception NullPointerException lors de l'appel prop.load(inputStream) si le fichier défini par ENVIRONMENT_VARIABLES_FILE n'existe pas.

C'est un bug qui ne nous dérangera pas dans l'immédiat puisque tout le monde a son environnement de travail configuré comme il faut mais je le report quand même parce que ça m'a fait perdre un peu de temps quand je suis tombé dessus en testant la CI sur mon fork (donc sans fichier ENVIRONMENT.properties) suite à #30.

Je m'étais contenté de faire ça pour résoudre le soucis mais il y a peut être plus clean:

private static String getFromFile(String variable) { 
   try { 
     try (InputStream inputStream = EnvironmentVariablesUtils.class.getResourceAsStream(ENVIRONMENT_VARIABLES_FILE)) { 
       if (Objects.isNull(inputStream))
         return null;
       . . .

https://github.com/B4va/alternbot-app/blob/b6ddc6d6ef2c7e2b1707367bcf4d4fa3f431de46/src/main/resources/logback.xml#L4

@zacharyarnaise :

Il y a une donnée qui figure dans l'iCal renvoyé par le site de l'IUT qui n'est pas récupérée chez nous:

DESCRIPTION:Algorithmique/programmation JAVA - promo - F13 - EI
DESCRIPTION:Langue anglaise - promo - E25 - TD
DESCRIPTION:Droit informatique - promo - E25 - EXA

Par exemple sur la dernière ligne il s'agit d'un examen, ça pourrait être intéressant d'exploiter cette info.

Comme indiqué dans l'image, tous les cours sont générés avec un décalage d'une heure vers le future.
A noter que l'import des cours fonctionne correctement dans google calendar par exemple, et qu'il a fonctionné correctement dans Outlook jusque fin Mars.
Je suspecte fortement que le changement d'heure (heure d'été -> dimanche 28 mars 2021 à 2h de matin) pose un soucis ici, mais je n'ai pas encore trouvé le soucis dans le fichier.

https://github.com/B4va/alternbot-app/blob/954128b02206ace0382c1285849574bc9a4b476f/src/main/java/process/commons/Publication.java#L33-L46

Les vérifications isNull(guild) et hasChannel(guild, channel) ne sont pas faites si le message à envoyer est long (> 2000 caractères).

https://github.com/B4va/alternbot-app/blob/954128b02206ace0382c1285849574bc9a4b476f/src/main/java/process/commons/Publication.java#L123-L125
Ça aura pour conséquence une exception non-gérée IndexOutOfBoundsException si le channel spécifié n'existe pas.

Problème : La sélection de certains objets passent actuellement par la récupération d'une table entière puis par l'application d'un processus de sélection.
Solution : Passer directement par une requête SQL plus complexe permettant la sélection des objets nécessaires.

https://github.com/B4va/alternbot-app/blob/b6ddc6d6ef2c7e2b1707367bcf4d4fa3f431de46/src/main/java/config/HibernateConfiguration.java#L11

Si Alternbot est supprimé d'un serveur, on peut supprimer le serveur de la base, ainsi que l'edt qui lui est associé, afin d'éviter de continuer à récupérer l'emploi du temps et de continuer à réaliser les publications.
Purge à réaliser de façon hebdomadaire ?