Projet

Général

Profil

Development #26909

clean-unused-account doit dépendre d'une configuration lié à l'OU

Ajouté par Benjamin Dauvergne il y a plus de 5 ans. Mis à jour il y a presque 4 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Catégorie:
-
Version cible:
-
Début:
02 octobre 2018
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

Description

Par défaut on ne supprime rien.

Les OUs doivent contenir un champ délai avant suppression sur inactivité, D, donné en jours et une délai idem en jours pour envoyer un mail de rappel signalant une future dé-activation, R.

clean-unused-account doit envoyer deux mails: D-R jours après le dernier login un mail indiquant que le compte va être supprimé, ce mail doit être adaptable en fonction de l'ou (account-deletion-on-inactivity-reminder-<ou.slug>.html) puis D jours après le dernier login un mail signalant la suppression doit être envoyé.


Fichiers

0001-WIP-clean-unused-accounts-depending-on-OUs-26909.patch (7,19 ko) 0001-WIP-clean-unused-accounts-depending-on-OUs-26909.patch Paul Marillonnet, 26 mars 2019 17:39
0001-WIP-clean-unused-accounts-depending-on-OUs-26909.patch (10,3 ko) 0001-WIP-clean-unused-accounts-depending-on-OUs-26909.patch Paul Marillonnet, 27 mars 2019 16:49
0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch (15 ko) 0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch Paul Marillonnet, 29 mars 2019 16:16
0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch (16,7 ko) 0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch Paul Marillonnet, 29 mars 2019 18:29
0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch (17,2 ko) 0001-per-OU-unused-user-accounts-cleaning-policy-26909.patch Paul Marillonnet, 29 mars 2019 19:30
0003-commands-misc-improvements-in-clean-unused-accounts-.patch (1,92 ko) 0003-commands-misc-improvements-in-clean-unused-accounts-.patch Valentin Deniaud, 18 mars 2020 10:16
0002-commands-per-OU-unused-user-accounts-cleaning-policy.patch (18,7 ko) 0002-commands-per-OU-unused-user-accounts-cleaning-policy.patch Valentin Deniaud, 18 mars 2020 10:16
0001-a2_rbac-add-missing-migration-26909.patch (1,49 ko) 0001-a2_rbac-add-missing-migration-26909.patch Valentin Deniaud, 18 mars 2020 10:16
0003-commands-misc-improvements-in-clean-unused-accounts-.patch (1,92 ko) 0003-commands-misc-improvements-in-clean-unused-accounts-.patch Valentin Deniaud, 01 avril 2020 18:25
0002-commands-per-OU-unused-user-accounts-cleaning-policy.patch (20,9 ko) 0002-commands-per-OU-unused-user-accounts-cleaning-policy.patch Valentin Deniaud, 01 avril 2020 18:25
0001-a2_rbac-add-missing-migration-26909.patch (1,49 ko) 0001-a2_rbac-add-missing-migration-26909.patch Valentin Deniaud, 01 avril 2020 18:25
0003-commands-misc-improvements-in-clean-unused-accounts-.patch (1,92 ko) 0003-commands-misc-improvements-in-clean-unused-accounts-.patch Valentin Deniaud, 02 avril 2020 11:03
0002-commands-per-OU-unused-user-accounts-cleaning-policy.patch (21,8 ko) 0002-commands-per-OU-unused-user-accounts-cleaning-policy.patch Valentin Deniaud, 02 avril 2020 11:03
0001-a2_rbac-add-missing-migration-26909.patch (1,49 ko) 0001-a2_rbac-add-missing-migration-26909.patch Valentin Deniaud, 02 avril 2020 11:03

Demandes liées

Lié à Publik - Development #26907: Cycle de vie des comptesFermé24 novembre 2020

Actions
Lié à Authentic 2 - Development #41284: Ne pas ré-initialiser last_account_deletion_alert s'il est déjà à NoneFermé02 avril 2020

Actions

Révisions associées

Révision 7ff5d1cb (diff)
Ajouté par Valentin Deniaud il y a presque 4 ans

a2_rbac: add missing migration (#26909)

Révision 5a8fbd9e (diff)
Ajouté par Valentin Deniaud il y a presque 4 ans

commands: per-OU unused user accounts cleaning policy (#26909)

Révision ba85c7eb (diff)
Ajouté par Valentin Deniaud il y a presque 4 ans

commands: misc improvements in clean-unused-accounts (#26909)

Historique

#2

Mis à jour par Benjamin Dauvergne il y a plus de 5 ans

#3

Mis à jour par Paul Marillonnet il y a plus de 5 ans

  • Assigné à mis à Paul Marillonnet
#4

Mis à jour par Paul Marillonnet il y a plus de 5 ans

Est-ce que je peux me permettre de patcher directement le modèle de l'OU ?

#5

Mis à jour par Benjamin Dauvergne il y a plus de 5 ans

Oui oui, tu peux même patcher le modèle User pour conserver la date du dernier rappel.

#6

Mis à jour par Paul Marillonnet il y a plus de 5 ans

Benjamin, dans la description du ticket, tu commences pas "Par défaut on ne supprime rien".
Est-ce que ça veut dire qu'on vire l'option clean-threshold que cette commande peut prendre en paramètre ?

Qu'en est-il des options alert_threshold et period ? Est-ce qu'on :
1) Supprime ces options ?
2) Superpose les mails générés par ces deux options, en tant que politique globale de l'IdP, aux mails générés par la configuration lié à l'OU ?

#7

Mis à jour par Benjamin Dauvergne il y a plus de 5 ans

Paul Marillonnet a écrit :

Benjamin, dans la description du ticket, tu commences pas "Par défaut on ne supprime rien".
Est-ce que ça veut dire qu'on vire l'option clean-threshold que cette commande peut prendre en paramètre ?

Yep, on base tout sur la configuration posée sur l'OU.

Qu'en est-il des options alert_threshold et period ? Est-ce qu'on :
1) Supprime ces options ?
2) Superpose les mails générés par ces deux options, en tant que politique globale de l'IdP, aux mails générés par la configuration lié à l'OU ?

Non non on vire tout et on repart proprement d'une configuration par OU, par défaut pour une OU, on ne supprime (ces champs sont vides).

#8

Mis à jour par Benjamin Dauvergne il y a environ 5 ans

  • Tracker changé de Support à Development
#9

Mis à jour par Paul Marillonnet il y a presque 5 ans

Benjamin Dauvergne a écrit :

Non non on vire tout et on repart proprement d'une configuration par OU, par défaut pour une OU, on ne supprime (ces champs sont vides).

Voilà rapidement (en pseudo-code) ce que j'ai en tête.
Le code, simplifié (peut-être à outrance ?), ne supporte plus des périodes à respecter entre mails d'avertissement.
Au contraire il assume que :
- la commande est exécutée une fois par jour au moins (par un cron quotidien, par exemple), car si ce n'est pas le cas certains usagers ne vont pas recevoir le mail d'avertissement.
- en particulier, si la commande est exécutée alors qu'un usager ne s'est pas loggué depuis au moins R+1 jours (cf description du ticket), et que cette commande ne s'était pas exécutée lorsque l'usager ne s'était pas loggué depuis R jours exactement, alors cet usager ne recevra pas le mail d'avertissement.

Si ce n'est pas ce qu'on veut, alors il faut revoir le code pour qu'entre deux runs on conserve l'information nécessaire pour que ne survienne pas le cas où un usager voit son compte supprimé alors qu'il n'a pas reçu de mail d'avertissement.

Qu'en dis-tu ?

#10

Mis à jour par Paul Marillonnet il y a presque 5 ans

Ah oui, autre chose encore : j'ai quand même gardé le filtre sur les usagers, qui me paraît utile indépendamment de toute appartenance à une OU.
En revanche, je réalise après coup qu'on aurait peut-être intérêt à rattacher à l'OU l'adresse courriel expéditrice des avertissements et notifications de suppression. Non ?

#11

Mis à jour par Benjamin Dauvergne il y a presque 5 ans

Je disais :

Oui oui, tu peux même patcher le modèle User pour conserver la date du dernier rappel.

Donc oui je m'attendais à ce qu'on ait de nouveaux champs sur l'objet user pour ne pas avoir de contraintes sur le délai entre lancement de la commande, c'est ingérable. Donc on garde trace de la date du dernier rappel, comme ça on peut aussi prévoir d'en faire plusieurs (si last_login > x jours et dernier_rappel is null ou dernier_rappel > y jours) alors nouveau rappel etc...

Quand je disais :

Par défaut on ne supprime rien.

Je parlais des utilisateurs que par défaut on ne supprimera plus, donc oui il faut supprimer les options de la commande, elle sera uniquement pilotée via le paramétrage sur les OUs, on peut potentiellement la virer même et s'appuyer sur la commande cleanupauthentic qui est déjà appelée tous les jours et appelle automatiquement les méthodes Model.objects.cleanup() quand elles existent.

Tout ceci étant dit je vais relire le patch mais je pense que ça ne colle pas.

#12

Mis à jour par Benjamin Dauvergne il y a presque 5 ans

Ne pas avoir peur d'ajouter des champs aux modèles en général, si c'est nécessaire, et bien c'est nécessaire.

#13

Mis à jour par Paul Marillonnet il y a presque 5 ans

Benjamin Dauvergne a écrit :

Donc oui je m'attendais à ce qu'on ait de nouveaux champs sur l'objet user pour ne pas avoir de contraintes sur le délai entre lancement de la commande, c'est ingérable. Donc on garde trace de la date du dernier rappel, comme ça on peut aussi prévoir d'en faire plusieurs (si last_login > x jours et dernier_rappel is null ou dernier_rappel > y jours) alors nouveau rappel etc...

Oui ok je comprends.

Je parlais des utilisateurs que par défaut on ne supprimera plus, donc oui il faut supprimer les options de la commande, elle sera uniquement pilotée via le paramétrage sur les OUs, on peut potentiellement la virer même et s'appuyer sur la commande cleanupauthentic qui est déjà appelée tous les jours et appelle automatiquement les méthodes Model.objects.cleanup() quand elles existent.

Tout ceci étant dit je vais relire le patch mais je pense que ça ne colle pas.

Je n'ai pas (encore) bougé le code dans la commande cleanupauthentic, je le ferais dans un second temps (une fois qu'on se sera mis d'accord sur ce que fait le code).

Je pose un patch WIP ici, je vais écrire des tests.

#15

Mis à jour par Paul Marillonnet il y a presque 5 ans

#17

Mis à jour par Paul Marillonnet il y a presque 5 ans

Des outputs de sérialization JSON écrits en dur dans tests/test_all.py

#18

Mis à jour par Benjamin Dauvergne il y a presque 5 ans

  • Statut changé de Solution proposée à En cours
  • clean_unused_accounts_from_email est inutile, on ne va pas varier l'email pour chaque type de message.
  • clean_unused_accounts_filter aussi, on restreint déjà par OU, si besoin de plus on verra une autre fois.
  • il manque un clean pour vérifier que clean_unused_accounts_alert est inférieur à clean_unused_accounts_deletion, comme ça rend les choses compliqués je serais pour un champ unique dérivé de CharField contenant une liste de nombres séparés par des virgules, ex.: 150, 300, 1 ça veut dire une alerte au bout de 150 jours puis une autre à 300 puis suppression après 1j, il suffit toujours de conserver la date de la dernière alerte. C'est assez simple1.
  • il faut un atomic() autour de l'envoi de mails et de l'écriture de last_account_deletion_alert, comme ça en cas de problème SMTP on refera (logger juste en warning les erreurs SMTP, temporiser puis continuer).

1 https://stackoverflow.com/questions/45266996/django-field-that-stores-a-list-as-a-comma-separated-string il faut prévoir le FormField aussi.

#19

Mis à jour par Valentin Deniaud il y a environ 4 ans

  • Assigné à changé de Paul Marillonnet à Valentin Deniaud

Je vais reprendre les patches en appliquant les remarques.

#20

Mis à jour par Valentin Deniaud il y a environ 4 ans

Voilà, il refaut une relecture complète j'ai changé pas mal de trucs.

Notamment, je me suis retrouvé à faire des schémas pour essayer de comprendre les longs if dans le patch, sans grand succès, mais de tout façon rien que de devoir faire ça c'est une bonne raison pour les enlever. Vérifier donc que je ne suis pas passé à côté d'un comportement attendu.

Benjamin Dauvergne a écrit :

ce mail doit être adaptable en fonction de l'ou (account-deletion-on-inactivity-reminder-<ou.slug>.html)

Je vire cette partie là, elle est déjà dans le premier patch de #38966, à finir de relire.

Il manque un clean pour vérifier que clean_unused_accounts_alert est inférieur à clean_unused_accounts_deletion, comme ça rend les choses compliqués

Compliqué, une condition de plus dans clean() ?

je serais pour un champ unique dérivé de CharField contenant une liste de nombres séparés par des virgules, ex.: 150, 300, 1 ça veut dire une alerte au bout de 150 jours puis une autre à 300 puis suppression après 1j, il suffit toujours de conserver la date de la dernière alerte. C'est assez simple

Je trouve que c'est tout le contraire (donc je n'en ai rien fait).

il faut un atomic() autour de l'envoi de mails et de l'écriture de last_account_deletion_alert, comme ça en cas de problème SMTP on refera

Puisqu'avant de supprimer on regarde si on a envoyé l'alerte, ce comportement implique donc qu'un authentic qui n'arrive jamais à envoyer de mail ne supprimera jamais de comptes, moi ça me va c'était juste histoire de le noter explicitement.

Quant à bouger le code pour le faire appeler par cleanupauthentic, je le ferai dans un nouveau ticket si toujours besoin.

#21

Mis à jour par Benjamin Dauvergne il y a environ 4 ans

Valentin Deniaud a écrit :

Voilà, il refaut une relecture complète j'ai changé pas mal de trucs.

Notamment, je me suis retrouvé à faire des schémas pour essayer de comprendre les longs if dans le patch, sans grand succès, mais de tout façon rien que de devoir faire ça c'est une bonne raison pour les enlever. Vérifier donc que je ne suis pas passé à côté d'un comportement attendu.

Benjamin Dauvergne a écrit :

ce mail doit être adaptable en fonction de l'ou (account-deletion-on-inactivity-reminder-<ou.slug>.html)

Je vire cette partie là, elle est déjà dans le premier patch de #38966, à finir de relire.

Le numéro du ticket n'est pas le bon.

Il manque un clean pour vérifier que clean_unused_accounts_alert est inférieur à clean_unused_accounts_deletion, comme ça rend les choses compliqués

Compliqué, une condition de plus dans clean() ?

Compliqué la fonctionnement pas le code, je trouvais plus simple et souple d'avoir une liste de nombres sans contraintes (tu mets 100, 10, 1, ça fait 100j -> alerte, 110j -> alerte et 111j -> suppression). C'est facile à afficher sur la vue de l'OU, à traiter et à comprendre. Mais ça reste juste une suggestion, les champs actuels suffisent bien sûr.

il faut un atomic() autour de l'envoi de mails et de l'écriture de last_account_deletion_alert, comme ça en cas de problème SMTP on refera

Puisqu'avant de supprimer on regarde si on a envoyé l'alerte, ce comportement implique donc qu'un authentic qui n'arrive jamais à envoyer de mail ne supprimera jamais de comptes, moi ça me va c'était juste histoire de le noter explicitement.

Oui on ne doit rien faire sans avoir une demi-certitude qu'on a bien tenté de notifier les gens, c'est super important. S'ils ne reçoivent pas les mails pour d'autres raisons, ce n'est pas notre souci.

Quant à bouger le code pour le faire appeler par cleanupauthentic, je le ferai dans un nouveau ticket si toujours besoin.

Puisqu'on réécrit ce serait mieux de commencer à centraliser tous les appels vers tous les cron-like pour tuer les commandes spécifiques un peu comme dans passerelle.

#22

Mis à jour par Valentin Deniaud il y a environ 4 ans

Benjamin Dauvergne a écrit :

Je vire cette partie là, elle est déjà dans le premier patch de #38966, à finir de relire.

Le numéro du ticket n'est pas le bon.

Oups, il s'agit de #35774.

Quant à bouger le code pour le faire appeler par cleanupauthentic, je le ferai dans un nouveau ticket si toujours besoin.

Puisqu'on réécrit ce serait mieux de commencer à centraliser tous les appels vers tous les cron-like pour tuer les commandes spécifiques un peu comme dans passerelle.

Qu'est-ce que ça implique pour ce ticket ? (je ne vais pas bouger de code, ça ferait trop de taf pour rebaser la suite des patches)

Sinon un test ne passe pas sans que je m'explique pourquoi, en local ça marche, et surtout les tickets suivants basés sur ces patches sont verts... J'ai relancé, sans succès.

#23

Mis à jour par Valentin Deniaud il y a presque 4 ans

Voilà, poussé la même chose dans une nouvelle branche et c'est vert.

#24

Mis à jour par Paul Marillonnet il y a presque 4 ans

  • Attention à la comparaison entre les deux champs nouvellement ajoutés à l'ou dans la méthode clean, qui est cassée en python3 :
Python 3.8.2 (default, Feb 25 2020, 13:04:52) 
[GCC 9.2.1 20200220] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from django_rbac.utils import get_ou_model
>>> ou = get_ou_model().objects.first()
>>> ou
<OrganizationalUnit 'default' 'Default organizational unit'>
>>> ou.clean_unused_accounts_alert
>>> ou.clean_unused_accounts_alert = 3
>>> ou.save()
>>> ou.clean_unused_accounts_alert >= ou.clean_unused_accounts_deletion
Traceback (most recent call last):
  File "<console>", line 1, in <module>
TypeError: '>=' not supported between instances of 'int' and 'NoneType'
  • Le days_to_deletion dans send_alert est je crois erroné, il faut non pas indiquer user.ou.clean_unused_accounts_deletion - alert_delay mais le nombre de jours restants sachant le dernier login avant la suppression (puisque c'est ce test qui est effectué pour déterminer la liste des usagers à supprimer) —— nombre qui peut-être inférieur à ton user.ou.clean_unused_accounts_deletion - alert_delay.
  • Et donc de la même façon, toujours dans send_alert, alert_delay ne devrait peut-être pas pas valoir user.ou.clean_unused_accounts_alert mais le nombre de jours depuis le dernier login, qui peut être supérieur à cette valeur user.ou.clean_unused_accounts_alert en fonction de la fréquence d'exécution de la commande.
  • Sur cette partie du patch :
    +            for user in users.filter(last_account_deletion_alert__isnull=True):
    +                logger.info('%s last login %d days ago, sending alert', user,
    +                         ou.clean_unused_accounts_alert)
    +                self.send_alert(user)
    +
    +            to_delete = users.filter(
    +                last_login__lte=now-deletion_delay,
    +                # ensure respect of alert delay before deletion
    +                last_account_deletion_alert__lte=now - (deletion_delay - alert_delay)
    +            )
    

    peut-être mettre un commentaire sur le fait que les deux filtres sont mutuellement exclusifs, (càd que last_account_deletion_alert__isnull=True et last_account_deletion_alert__lte=now - (…) ne peuvent arriver en même temps, ce qui n'est pas une évidence à mon avis).
  • Il faut sans doute aussi gérer les cas où la première alerte n'a été envoyée qu'alors que le nombre de jours avant suppression est bien inférieur à ou.clean_unused_accounts_deletion - ou.clean_unused_accounts_alert. Il faut sans doute accorder un délai supplémentaire (parce que ce n'est pas la faute de l'usager si (i) la commande n'est pas exécutée assez fréquemment, ou si (ii) la fréquence est élevée mais la date de première exécution de la commande est proche de user.last_login + timedelta(days=user.ou.clean_unused_accounts_deletion auquel cas l'usager va avoir très peu de temps pour tenir compte de l'alerte).
  • Last but not least, il faut remettre user.last_account_deletion_alert à null lorsque l'usager effectue une connexion réussie. Je n'ai pas vu ça dans le patch mais je loupe peut-être un truc ?
#25

Mis à jour par Paul Marillonnet il y a presque 4 ans

Paul Marillonnet a écrit :

  • Last but not least, il faut remettre user.last_account_deletion_alert à null lorsque l'usager effectue une connexion réussie. Je n'ai pas vu ça dans le patch mais je loupe peut-être un truc ?

Ouch non pardon, c'est géré par le to_delete = users.filter(…).

#26

Mis à jour par Paul Marillonnet il y a presque 4 ans

Paul Marillonnet a écrit :

Paul Marillonnet a écrit :

  • Last but not least, il faut remettre user.last_account_deletion_alert à null lorsque l'usager effectue une connexion réussie. Je n'ai pas vu ça dans le patch mais je loupe peut-être un truc ?

Ouch non pardon, c'est géré par le to_delete = users.filter(…).

En fait non, pas forcément. Encore une fois ça dépend de la fréquence d'exécution de la commande.

#27

Mis à jour par Valentin Deniaud il y a presque 4 ans

Paul Marillonnet a écrit :

  • Attention à la comparaison entre les deux champs nouvellement ajoutés à l'ou dans la méthode clean, qui est cassée en python3 :

Le vrai problème derrière étant de permettre de mettre un délai d'alerte sans délai de suppression, ou inversement, c'est corrigé.

  • Le days_to_deletion dans send_alert est je crois erroné [...]
  • Et donc de la même façon, [...]
  • Il faut sans doute aussi gérer les cas où la première alerte n'a été envoyée qu'alors que le nombre de jours avant suppression est bien inférieur [...]

Hum, il me semble que ces trois remarques sont caduques du fait des lignes

 83                 # ensure respect of alert delay before deletion
 84                 last_account_deletion_alert__lte=now - (deletion_delay - alert_delay)

Et du test correspondant, test_clean_unused_account_always_alert.
Parce que le seul cas problématique, ce serait si on disait que le compte allait être supprimé dans 14j et qu'il l'était au bout de 10. Si c'est l'inverse qui se produit, ce n'est pas grave, et pour moi c'est plus important d'avoir un mail carré qui te dise quel est la politique de suppression des vieux comptes qu'un mail ultra précis. D'ailleurs si je pense aux mails du genre que j'ai déjà reçus, c'est toujours comme ça.
Je considère aussi que la commande va s'exécuter tous les jours, si le cron est buggé il faut faire en sorte qu'il ne se produise pas de catastrophe, ce qui normalement est le cas, mais les imprécisions qu'un échec ou deux peuvent induire doivent être négligées pour garder le code le plus lisible possible IMO.
Si au contraire je n'ai pas compris tes remarques le plus efficace est sûrement que tu écrives un test pour montrer ce qui ne marche pas.

peut-être mettre un commentaire sur le fait que les deux filtres sont mutuellement exclusifs, (càd que last_account_deletion_alert__isnull=True et last_account_deletion_alert__lte=now - (…) ne peuvent arriver en même temps, ce qui n'est pas une évidence à mon avis).

Il faut faire confiance aux tests pour ça, je ne pense pas qu'il faille indiquer dans notre code comment fonctionne l'ORM django, et il y a un commentaire nettement plus important et utile à proximité, donc je passe mon tour là dessus.

  • Last but not least, il faut remettre user.last_account_deletion_alert à null lorsque l'usager effectue une connexion réussie. Je n'ai pas vu ça dans le patch mais je loupe peut-être un truc ?

Très bien vu, je corrige et ajoute un morceau de test.

#28

Mis à jour par Paul Marillonnet il y a presque 4 ans

Une remarque qui ne me vient à l'esprit que maintenant : je verrais bien des PositiveIntegerField pour les deux champs nouvellement ajoutés à l'OU (au lieu d'IntegerField) pour éviter les soucis d'arithmétique sur les durées et sur les dates dans le code.
Notamment, il n'y a pas d'erreur levée pour un timedelta(days=foo) avec foo entier négatif, même si j'avoue ne pas cerner ce que signifie une durée négative :)

Valentin Deniaud a écrit :

Le vrai problème derrière étant de permettre de mettre un délai d'alerte sans délai de suppression, ou inversement, c'est corrigé.

Ok. Une dernière question à ce sujet, est-ce qu'on n'essaierait pas d'éviter le scénario catastrophe où l'administrateur fonctionnel pense désactiver l'envoi d'alertes et la suppression en mentionnant 0 dans les deux durées dans le formulaire d'édition de l'OU?
Genre remplacer :

for ou in get_ou_model().objects.filter(clean_unused_accounts_alert__isnull=False):

par
for ou in get_ou_model().objects.filter(clean_unused_accounts_alert__gt=0):

Hum, il me semble que ces trois remarques sont caduques du fait des lignes
[...]
Et du test correspondant, test_clean_unused_account_always_alert.
Parce que le seul cas problématique, ce serait si on disait que le compte allait être supprimé dans 14j et qu'il l'était au bout de 10. Si c'est l'inverse qui se produit, ce n'est pas grave, et pour moi c'est plus important d'avoir un mail carré qui te dise quel est la politique de suppression des vieux comptes qu'un mail ultra précis. D'ailleurs si je pense aux mails du genre que j'ai déjà reçus, c'est toujours comme ça.
Je considère aussi que la commande va s'exécuter tous les jours, si le cron est buggé il faut faire en sorte qu'il ne se produise pas de catastrophe, ce qui normalement est le cas, mais les imprécisions qu'un échec ou deux peuvent induire doivent être négligées pour garder le code le plus lisible possible IMO.
Si au contraire je n'ai pas compris tes remarques le plus efficace est sûrement que tu écrives un test pour montrer ce qui ne marche pas.

Non non c'est très clair, c'est moi qui suis passé trop vite sur ce bout de code que tu cites.

Il faut faire confiance aux tests pour ça, je ne pense pas qu'il faille indiquer dans notre code comment fonctionne l'ORM django, et il y a un commentaire nettement plus important et utile à proximité, donc je passe mon tour là dessus.

Ok pas de souci.

Très bien vu, je corrige et ajoute un morceau de test.

Nickel.

#29

Mis à jour par Valentin Deniaud il y a presque 4 ans

Paul Marillonnet a écrit :

Une remarque qui ne me vient à l'esprit que maintenant : je verrais bien des PositiveIntegerField pour les deux champs nouvellement ajoutés

est-ce qu'on n'essaierait pas d'éviter le scénario catastrophe où l'administrateur fonctionnel pense désactiver l'envoi d'alertes et la suppression en mentionnant 0 dans les deux durées dans le formulaire d'édition de l'OU?

Tout à fait, je fais donc d'une pierre deux coups :

-    clean_unused_accounts_alert = models.IntegerField(
+    clean_unused_accounts_alert = models.PositiveIntegerField(
         verbose_name=_('Days after which the user receives an account deletion alert'),
+        validators=[MinValueValidator(
+            30, _('Ensure that this value is greater than 30 days, or leave blank for deactivating.')
+        )],
         null=True,

Et pareil pour l'autre.
Parce que je vois bien aussi quelqu'un mettre genre 1 en pensant que ça veut dire un an, et il n'y a pas d'usage où mettre moins de 30 jours (plus, en vrai) aurait du sens.

#30

Mis à jour par Paul Marillonnet il y a presque 4 ans

  • Statut changé de Solution proposée à Solution validée

Ok parfait.

#31

Mis à jour par Valentin Deniaud il y a presque 4 ans

  • Statut changé de Solution validée à Résolu (à déployer)
commit ba85c7eb576d2723849b54e56cd8b1f261c582da
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Tue Mar 17 15:26:52 2020 +0100

    commands: misc improvements in clean-unused-accounts (#26909)

commit 5a8fbd9ef1351bd1369979522947d249b9c650f9
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Wed Mar 18 10:09:04 2020 +0100

    commands: per-OU unused user accounts cleaning policy (#26909)

commit 7ff5d1cbcd37655cbb9cf5b3bd4f5411ee7cf30a
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Tue Mar 17 15:15:23 2020 +0100

    a2_rbac: add missing migration (#26909)
#32

Mis à jour par Benjamin Dauvergne il y a presque 4 ans

  • Lié à Development #41284: Ne pas ré-initialiser last_account_deletion_alert s'il est déjà à None ajouté
#33

Mis à jour par Paul Marillonnet il y a presque 4 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF