Skip to content

Retirer l’heure de fin des diagnostics d’éligibilité et utiliser uniquement la date#5210

Merged
francoisfreitag merged 1 commit into
masterfrom
ff/expires-at
Dec 17, 2024
Merged

Retirer l’heure de fin des diagnostics d’éligibilité et utiliser uniquement la date#5210
francoisfreitag merged 1 commit into
masterfrom
ff/expires-at

Conversation

@francoisfreitag

Copy link
Copy Markdown
Member

🤔 Pourquoi ?

Un datetime n’a pas vraiment de sens pour les diagnostics d’éligibilité. On n’affiche jamais les heures/minutes/secondes aux utilisateurs, et la date d’expiration est administrative, ce n’est pas un point dans le temps.

@francoisfreitag

Copy link
Copy Markdown
Member Author

Ça fait un gros UPDATE sur 1M de lignes, qui met 2-3 secondes à s’exécuter sur ma machine. Sûrement un peu moins sur notre super base de données musclée.

@francoisfreitag francoisfreitag added the modifié Modifié dans le changelog. label Dec 6, 2024
@francoisfreitag francoisfreitag changed the title Utiliser un DateField pour EligibilityDiagnosis.expires_at Retirer l’heure de fin des diagnostics d’éligibilité et utiliser uniquement la date Dec 6, 2024

@xavfernandez xavfernandez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cela me semble être une bonne idée 👍

migrations.AlterField(
model_name="eligibilitydiagnosis",
name="expires_at",
field=models.DateField(db_index=True, verbose_name="date d'expiration"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Est-ce que la migration se passe bien ?
Dans le sens où un diag qui expire à "JOUR 23h50UTC" (donc "JOUR+1 00h50 CET" actuellement) doit avoir comme date d'expiration convertie en "JOUR+1".
Si ce n'est pas le cas, il faudrait idéalement d'abord adapter les heures pour que la conversion se fasse bien ensuite.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

On s’en fiche, on n’est pas à un jour près sur les dates. Le diag a une durée très arbitraire, pas grave si on arrondit mal dans un cas limite.
https://www.notion.so/plateforme-inclusion/Mise-en-conformit-du-fonctionnement-des-dates-du-PASS-IAE-prolongation-suspension-7b0dbd5e87e045f1acc2f2352e0e3aec

@xavfernandez

Copy link
Copy Markdown
Contributor

Ça fait un gros UPDATE sur 1M de lignes, qui met 2-3 secondes à s’exécuter sur ma machine. Sûrement un peu moins sur notre super base de données musclée.

Ça pourrait lui faire du bien de découper la migration en deux pour n'avoir un LOCK que sur une seule table à la fois ?

@francoisfreitag

Copy link
Copy Markdown
Member Author

Il n’y a que 10K diag d’éligibilité GEIQ, ils ne pèsent pas dans la balance.

@francoisfreitag

Copy link
Copy Markdown
Member Author

C’est aussi une table peu mise à jour.

Comment thread itou/eligibility/models/common.py Outdated
created_at = models.DateTimeField(verbose_name="date de création", default=timezone.now, db_index=True)
updated_at = models.DateTimeField(verbose_name="date de modification", auto_now=True, db_index=True)
expires_at = models.DateTimeField(verbose_name="date d'expiration", db_index=True)
expires_at = models.DateField(verbose_name="date d'expiration", db_index=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Je me demande si un petit help_text expliquant que le diag n'est donc plus valide à cette date ne serait pas le bienvenu.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pour préciser que la date est exclusive ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oui exclusive sur la validité et inclusive sur l'expiration 😬

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

😁 J’ai essayé de tourner ça clairement.

Comment thread itou/fixtures/django/20_eligibility_diagnoses.json Outdated
Comment thread itou/eligibility/models/common.py
@francoisfreitag francoisfreitag force-pushed the ff/expires-at branch 4 times, most recently from e19e1b7 to ba98dba Compare December 12, 2024 15:13
@francoisfreitag francoisfreitag added this pull request to the merge queue Dec 17, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
Using a datetime doesn’t make much sense, we’re dealing with
administrative periods.

test_update_diagnosis_extend_the_validity_only_when_we_have_the_same_author_and_the_same_criteria
was modified because the updated diagnosis (only author) has a pretty
good chance of expiring the same day.
@francoisfreitag francoisfreitag added this pull request to the merge queue Dec 17, 2024
Merged via the queue into master with commit b680afe Dec 17, 2024
@francoisfreitag francoisfreitag deleted the ff/expires-at branch December 17, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

modifié Modifié dans le changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants