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

[FEATURE] Supprimer la colonne "grainId" de la table "element-answers" (PIX-12459) #8935

Merged

Conversation

reibecca
Copy link
Contributor

@reibecca reibecca commented May 15, 2024

🦄 Problème

Cette information n'est pas utilisée par l'équipe métier, nous n'avons donc pas le besoin de l'enregistrer.

🤖 Proposition

Faire une migration pour supprimer la colonne grainId de la table element-anwers de la BDD.

🌈 Remarques

On a choisi de mettre la colonne à nullable dans le down de la migration, sinon le rollback ne fonctionne pas dans le cas où il y a déjà des données dans la BDD.

💯 Pour tester

En local :

  1. Dans le dossier api lancer la commande npm run db:migrate
  2. Dans votre BDD, vérifiez que la colonne grainId de la table element-anwers n'existe plus
  3. Lancer Pix App et aller sur le module didacticiel-modulix
  4. Répondez aux questions et assurez vous que lorsque vous cliquez sur le bouton Vérifiez il n'y a pas d'erreur
  5. Ensuite retournez dans votre terminal et exécutez la commande npm run db:rollback:latest
  6. Retourner dans votre BDD et vérifiez que la colonne grainId de la table element-anwers est à nouveau présente

Sur la RA :

  1. Vérifiez qu'il n'y a pas de regression en allant sur le module didacticiel-modulix
  2. Répondez aux questions et assurez vous que lorsque vous cliquez sur le bouton Vérifiez il n'y a pas d'erreur

@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@reibecca reibecca marked this pull request as ready for review May 15, 2024 14:24
@reibecca reibecca requested a review from a team as a code owner May 15, 2024 14:24
@reibecca reibecca force-pushed the PIX-12459-remove-grainId-from-element-answers branch from 2df6e31 to 2795683 Compare May 15, 2024 14:28
@dlahaye
Copy link
Contributor

dlahaye commented May 15, 2024

5. Ensuite retourner dans votre terminal et exécutez la commande npm run db:rollback:latest

ça ne marchera pas 👉 Dans le fichier de migration > ici <, lors du scénario de test proposé, on fini avec une ou plusieurs lignes dans la table, et le fait que la colonne ne soit pas nullable génère une erreur SQL lors du rollback.

‼️ migration failed with error: alter table "element-answers" add column "grainId" varchar(255) not null - column "grainId" of relation "element-answers" contains null values

Réponse 1 heure plus tard:
On a choisi de mettre la colonne à nullable dans le down.

Une autre stratégie aurait été de gérer la rétro-compatibilité en deux PR distinctes:

  • Une PR où on supprime les usages dans le code

Puis une version plus tard:

  • Une PR avec la migration qui drop la colonne

@dlahaye dlahaye force-pushed the PIX-12459-remove-grainId-from-element-answers branch from 2795683 to cf6bf54 Compare May 15, 2024 16:14
@VincentHardouin
Copy link
Member

Je suis vraiment pas convaincu de supprimer cette colonne, comme je n'étais pas là lors des points qui ont fait émergé ce ticket, je ne pense pas avoir tout le contexte.
Pour moi, avoir le grainId permet quand même d'avoir le contexte dans lequel est passé l'élément et ça me parait intéressant comme information, comme un élément est sensé être réutilisable.
On peut imaginer faire un élément de pré-module, et le même en fin de module. Si nous n'avons plus le grainId, alors l'analyse des résultats sera compliqué.
Même autre que dans le même module, si nous souhaitons voir si un élément fonctionne mieux dans un module où l'autre, cela nous permets de récupérer l'information sans faire le lien avec les passages et le module.

@VincentHardouin
Copy link
Member

Je suis vraiment pas convaincu de supprimer cette colonne, comme je n'étais pas là lors des points qui ont fait émergé ce ticket, je ne pense pas avoir tout le contexte. Pour moi, avoir le grainId permet quand même d'avoir le contexte dans lequel est passé l'élément et ça me parait intéressant comme information, comme un élément est sensé être réutilisable. On peut imaginer faire un élément de pré-module, et le même en fin de module. Si nous n'avons plus le grainId, alors l'analyse des résultats sera compliqué. Même autre que dans le même module, si nous souhaitons voir si un élément fonctionne mieux dans un module où l'autre, cela nous permets de récupérer l'information sans faire le lien avec les passages et le module.

Pour le contexte : le métier ne souhaite pas réutiliser les éléments, ni les grains c'est donc OK de supprimer la colonne

@pix-service-auto-merge pix-service-auto-merge force-pushed the PIX-12459-remove-grainId-from-element-answers branch from cf6bf54 to d1f0ce6 Compare May 17, 2024 08:02
mirawlin and others added 2 commits May 17, 2024 08:11
Co-authored-by: Rébecca Kaci <82950611+reibecca@users.noreply.github.com>
don't save grainId in element-answers table anymore

Co-authored-by: miranda.lin-guignard <miranda.w.lin@gmail.com>
@pix-service-auto-merge pix-service-auto-merge force-pushed the PIX-12459-remove-grainId-from-element-answers branch from d1f0ce6 to bd8c4de Compare May 17, 2024 08:11
@pix-service-auto-merge pix-service-auto-merge merged commit 0cc0406 into dev May 17, 2024
5 of 7 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the PIX-12459-remove-grainId-from-element-answers branch May 17, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants