Candidatures : Archiver les candidatures inactives depuis 180+ jours#4649
Candidatures : Archiver les candidatures inactives depuis 180+ jours#4649francoisfreitag merged 1 commit intomasterfrom
Conversation
|
🥁 La recette jetable est prête ! 👉 Je veux tester cette PR ! |
8c82dd9 to
cfbd638
Compare
cfbd638 to
3e33006
Compare
|
🥁 La recette jetable est prête ! 👉 Je veux tester cette PR ! |
3e33006 to
f8a6132
Compare
c512648 to
51200fb
Compare
| JobApplication.objects.is_active_company_member(request.user) | ||
| .filter(pk=job_application_id) | ||
| .exclude(state=JobApplicationState.ACCEPTED) | ||
| .update(archived_at=archived_at, archived_by=archived_by) |
There was a problem hiding this comment.
Dans le cas d'appels successifs on va écraser les informations précédentes, en général on veux plutôt garder le premier.
There was a problem hiding this comment.
Ici, comme on s’en fiche, j’aurais tendance à ne pas compliquer et garder le comportement actuel. Si deux employeurs archivent en même temps, celui qui a archivé en dernier apparaîtra.
|
|
||
| def test_already_in_target_state(self, client, archived_at_func, expected_func, viewname): | ||
| target_archived_at = expected_func() | ||
| job_app = JobApplicationFactory(archived_at=target_archived_at) |
There was a problem hiding this comment.
A mon avis si ici tu rajoutes archived_by=job_app.to_company.members.get() tu tombes dans le cas de 404 que je mentionnais.
Ce qui me fait me demander si il faudrait pas ajouter le champ au parametrize pour tester le chemin de la commande et le chemin depuis l'UI.
There was a problem hiding this comment.
Nope, je confirme ne pas avoir d’erreurs avec:
diff --git a/tests/www/apply/test_edit.py b/tests/www/apply/test_edit.py
index 41a27ef48..932f56cba 100644
--- a/tests/www/apply/test_edit.py
+++ b/tests/www/apply/test_edit.py
@@ -294,8 +292,15 @@ class TestArchiveView:
def test_already_in_target_state(self, client, archived_at_func, expected_func, viewname):
target_archived_at = expected_func()
- job_app = JobApplicationFactory(archived_at=target_archived_at, state=JobApplicationState.REFUSED)
- client.force_login(job_app.to_company.members.get())
+ company = CompanyWithMembershipAndJobsFactory()
+ employer = company.members.get()
+ job_app = JobApplicationFactory(
+ to_company=company,
+ archived_at=target_archived_at,
+ archived_by=employer,
+ state=JobApplicationState.REFUSED,
+ )
+ client.force_login(employer)
response = client.post(reverse(viewname, args=(job_app.pk,)))
assertRedirects(response, reverse("apply:details_for_company", args=(job_app.pk,)))
job_app.refresh_from_db()| def test_archived(client): | ||
| company = CompanyFactory(with_membership=True) | ||
| active = JobApplicationFactory(to_company=company) | ||
| archived = JobApplicationFactory(to_company=company, archived_at=timezone.now()) | ||
| archived_badge_html = """\ | ||
| <span class="badge rounded-pill badge-sm mb-1 bg-emploi-light text-primary" | ||
| aria-label="candidature archivée" | ||
| data-bs-toggle="tooltip" | ||
| data-bs-placement="top" | ||
| data-bs-title="Candidature archivée"> | ||
| <i class="ri-archive-line mx-0"></i> | ||
| </span> | ||
| """ | ||
|
|
||
| client.force_login(company.members.get()) | ||
| url = reverse("apply:list_for_siae") | ||
| response = client.get(url) | ||
| assertContains(response, active.pk) | ||
| assertNotContains(response, archived.pk) | ||
| assertNotContains(response, archived_badge_html, html=True) | ||
| response = client.get(url, data={"archived": ""}) | ||
| assertContains(response, active.pk) | ||
| assertNotContains(response, archived.pk) | ||
| assertNotContains(response, archived_badge_html, html=True) | ||
| response = client.get(url, data={"archived": "archived"}) | ||
| assertNotContains(response, active.pk) | ||
| assertContains(response, archived.pk) | ||
| assertContains(response, archived_badge_html, html=True, count=1) | ||
| response = client.get(url, data={"archived": "all"}) | ||
| assertContains(response, active.pk) | ||
| assertContains(response, archived.pk) | ||
| assertContains(response, archived_badge_html, html=True, count=1) | ||
|
|
||
|
|
There was a problem hiding this comment.
Je pense que tu peux mettre les deux test_archived en commun dans tests/www/apply/test_list.py vu que c'est pas spécifique à 1 seule liste.
There was a problem hiding this comment.
Noui, parce que les setups et les vues sont différents. Et rien ne dit qu’elles évolueront ensemble, du coup je garderais plutôt les deux tests simples, plutôt que de m’amuser à créer des embranchements complexes.
d9674c2 to
225d529
Compare
|
|
||
| JobApplication.objects.filter( | ||
| hidden_for_company=True, | ||
| state__in=archivable_states, |
There was a problem hiding this comment.
Tu pourrais rajouter un archived_at=None également car il y a de bonnes chances qu'une grosse partie des hidden_for_company=True archivables n'aient pas été modifiés depuis longtemps...
There was a problem hiding this comment.
@francoisfreitag Ce matin tu parlais de 200k lignes pour celles-ci ? Ça va peut-être être un peu trop gros de tout faire en 1 UPDATE, non ?
There was a problem hiding this comment.
- avec
archived_at, best of 2, 5 secondes - sans
archived_at, best of 2,quatre20 secondes
Edit, j’ai du me planter en mesurant ce matin.
Donc, j’aurais tendance à laisser ainsi, quitte à passer la migration en dehors des heures de trafic intense. Dans tous les cas, vu le nombre d’updates, ça me semble une bonne idée.
Qu’en dîtes vous ?
There was a problem hiding this comment.
En réalité, il ne reste que 39K candidatures à archiver (donc de moins de 6 mois)
039769d to
0c3647b
Compare
| while chunk := JobApplication.objects.filter( | ||
| state__in=archivable_states, | ||
| updated_at__lte=six_months_ago, | ||
| archived_at=None, |
There was a problem hiding this comment.
Attention : en itérant sur un filtres sur des champs sans index ça va durer des plombes (ça sera de plus en plus long de trouver les 1000 prochaines candidature sans archived_at.
Il vaut mieux extraire la liste des ids des candidatures à traiter, puis itérer dessus.
Voir https://itou-inclusion.slack.com/archives/C0412CTV63D/p1725541863233409?thread_ts=1725526366.771849&cid=C0412CTV63D
et https://github.com/gip-inclusion/les-emplois/pull/4662/files#diff-52d271ec0e4d9971fc1a02420ea32c7e1711afe3512aa8a92d7745526bcef79f
There was a problem hiding this comment.
La remarque reste vraie mais Il y a un index sur archived_at donc ça sera moins pire :).
There was a problem hiding this comment.
Effectivement, il reste bien plus efficace d’identifier d’abord. La version qui identifie à chaque itération passait en 1200s sur ma machine, la version de tout identifier d’un coup et faire des batchs prend 300s. La requête pour tout identifier est un peu lourde (10s), mais je ne pense pas utile de la découper en buckets de dates.
There was a problem hiding this comment.
Et pas besoin de rattrapage, le cron du lendemain attrapera les candidatures qui auraient pu être archivées le temps que la migration se déroule.
There was a problem hiding this comment.
Je vois que le archived_at=None à disparu, ça devrais pas poser problème mais l'avantage que je vois de le garder c'est que vu qu'on est dans une transaction non-atomique alors si jamais on devais rejouer la migration (par exemple si timeout ou autre erreur lors du déploiement) alors on ne se retaperais pas ce qui est déjà fait (et donc possiblement la même erreur qu'avant :)).
e230efc to
e7a5b2c
Compare
|
J’ai poussé un certain nombre de fixups suite aux dernières discussions. J’aimerais bien merger tard ce soir pour passer la migration en dehors des heures de bureau et éviter un impact sur les utilisateurs et pouvoir surveiller demain. Aucun problème pour attendre la fin de semaine prochaine si quelqu’un veut prendre le temps de refaire une revue détaillée, il suffit de me le signaler. |
rsebille
left a comment
There was a problem hiding this comment.
J'ai refait une passe rapide mais les tests me semblent assez complets donc je pense que c'est safe de ce coté là :D.
| while chunk := JobApplication.objects.filter( | ||
| state__in=archivable_states, | ||
| updated_at__lte=six_months_ago, | ||
| archived_at=None, |
There was a problem hiding this comment.
Je vois que le archived_at=None à disparu, ça devrais pas poser problème mais l'avantage que je vois de le garder c'est que vu qu'on est dans une transaction non-atomique alors si jamais on devais rejouer la migration (par exemple si timeout ou autre erreur lors du déploiement) alors on ne se retaperais pas ce qui est déjà fait (et donc possiblement la même erreur qu'avant :)).
e7a5b2c to
ddcd384
Compare
Remove old applications from the employer and prescriber job applications list. The `hidden_for_company` feature is removed, because making job applications disappear completely was confusing. Usually, one of their colleague would delete the application, then users log in and do not see the application anymore. Instead, the `archived_at` field allows storing inactive applications on the side, and restoring them to the active state when necessary. Employers and prescribers can access archived job applications using a filter on the job application list. Only employers can change the state of an application, the goal of this feature being to help them be more responsive to recent job applications. Accepted and prior to hire job applications cannot be archived, as they represent an employment relationship, so we can assume employers need to find these applications more often.
ddcd384 to
410590c
Compare
🤔 Pourquoi ?
Supprimer les candidatures inactives de la file active.
La fonctionnalité de suppression a été retirée, car elle entrait en conflit avec cette nouvelle fonctionnalité, et perturbait les utilisateurs. Il n’est pas rare qu’un collègue supprime une candidature et qu’une autre personne de l’organisation contacte le support au sujet d’une candidature disparue.
Avec la fonctionnalité d’archivage, il sera possible de mettre de côté les candidatures inactives et de les ramener dans la file active lorsque c’est nécessaire.
Les employeurs et les prescripteurs peuvent filtrer les candidatures archivées à l’aide des filtres de candidature.
Seuls les employeurs peuvent changer l’état archivée / active d’une candidature, car cette fonctionnalité leur est destinée pour aider à réduire la file active.
Les candidatures acceptée représentent une relation de travail (un contrat) entre un employeur et un candidat. On ne les archive par car il est probable que l’employeur souhaite accéder fréquemment à la candidature.
🏝️ Comment tester
💻 Captures d'écran
Questions ?
hidden_for_company,archived_atest transmis en tant quebool. En fait-on une date ? => NONhidden_for_companyest bien souhaitée, et qu’ils acceptent que les candidatures supprimées ré-apparaissent comme archivées. => OUI