Admin : conserver le statut d'administrateur lors du transfert d'une SIAE à une autre, et supprimer les membres lors de la désactivation [GEN-2374]

[EwenKorr]

🤔 Pourquoi ?

Le bouton de transfert d'une entreprise est souvent utilisé pour transférer les données vers une entreprise qui a déjà des membres.

Actuellement, si un membre existe dans les 2 entreprises, quels que soient les attributs du membership (is_admin ou is_active), on ne fait rien.

On voudrait dans un premier temps que si le statut is_admin diffère, celui-ci soit transféré également.

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

• Mettre à jour le CHANGELOG_breaking_changes.md ?
• Ajouter l'étiquette « Bug » ?

🏝️ Comment tester ?

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

[notion-workspace]

🏎️ Conserver le statut d’administrateur lors d’un move siae data

[github-actions]

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

[rsebille]

Honnêtement tout le truc de devoir jouer avec .exclude(.filter(...)) m'a flingué le cerveau, et ça va devenir un enfer si on doit gérer d'autre champs de ce type alors qu'au final on veux "juste" une logique d'upsert.

Il manque du code et il a sûrement plus malin à faire (surtout pour simplifier la gestion de la clé unique) mais j'ai l'impression qu'un truc comme ceci fonctionnerais et permettrais à la fois de gérer d'autres champs facilement mais aussi d'être réutiliser plus tard si besoin tout en étant personnalisable :

diff --git a/itou/companies/transfer.py b/itou/companies/transfer.py
index 2f2d593b7..9dfb4829d 100644
--- a/itou/companies/transfer.py
+++ b/itou/companies/transfer.py
@@ -1,4 +1,6 @@
+from django.db import IntegrityError
 from django.db.models import TextChoices
+from django.db.models.constraints import UniqueConstraint
 from django.utils import timezone
 
 from itou.approvals import models as approvals_models
@@ -69,6 +71,13 @@ TRANSFER_SPECS = {
         "to_filter": lambda qs, to_company: qs.exclude(
             user__in=users_models.User.objects.filter(companymembership__company=to_company)
         ),
+        "upsert": {
+            "key": {"user", "company"},
+            "fields": {
+                "is_admin": max,  # lambda from_value, to_value: ...
+                "is_active": max,  # lambda from_value, to_value: ...
+            },
+        },
     },
     TransferField.INVITATIONS: {
         "related_model": invitations_models.EmployerInvitation,
@@ -182,7 +191,20 @@ def transfer_company_data(
 
                 if not spec.get("report_only"):
                     setattr(item, spec["related_model_field"], to_company)
-                    item.save(update_fields=[spec["related_model_field"]])
+                    try:
+                        item.save(update_fields=[spec["related_model_field"]])
+                    except IntegrityError as e:
+                        if e.violated_constraint is not UniqueConstraint:  # Ça existe pas mais vous voyez l'idée :)
+                            raise
+                        to_item = spec["related_model"].objects.get(
+                            **{field: getattr(item, field) for field in spec["upsert"]["key"]}
+                        )  #
+                        for field, merge_function in spec["upsert"]["fields"].items():
+                            final_value = merge_function(
+                                getattr(item, field),
+                                getattr(to_item, field),
+                            )
+                            setattr(item, field, final_value)
                 reporter.add(transfer_field, _format_model(item))
 
     if save_update_fields:

[EwenKorr]

Merci pour la relecture et la vigilance !

Dernières nouvelles du front (conversation Slack) : on voudrait transférer is_admin d'une part et enlever les anciens membres de l'entreprise source.

On prendrait donc ton mécanisme d'upsert et on supprimerait systématiquement les memberships de l'entreprise source.
Et on rajoute un peu plus d'info dans reporter.

Je vais faire ça.

[EwenKorr]

Ta proposition @rsebille m'a fait des nœuds dans le cerveau, je ne suis pas habitué à autant d'abstraction 😅

[EwenKorr]

Pas trouvé mieux qu'en test sur le message d'erreur :/
J'ai cherché du côté des contraintes également, sans succès.

[rsebille]

J'aillais dire "ouais ça m'étonne qu'à moitié" et puis mon cerveau c'est dit "There must be a better way", et il semblerais que oui 😁.

PostgreSQL aillant un code d'erreur dédié et psycopg faisant bien les choses il y a une exception dédiée pour les contraintes unique : https://github.com/psycopg/psycopg/blob/9bde1475f0e77beaf285d72f97c9ead721e8cd94/psycopg/psycopg/errors.py#L1037-L1039
par contre Django intercepte cette exception et la réécris en django.db.utils.IntegrityError (à ne pas confondre avec psycopg.errors.IntegrityError ;)) donc faut creuser un peu dans l'exception mais ceci devrais faire le boulot :

Suggested change

	if "unique constraint" not in e.args[0]:
	if not isinstance(e.__cause__, psycopg.errors.UniqueViolation)

Cas synthétique de test :

In [27]: with transaction.atomic():
    ...:     try:
    ...:         obj.save()
    ...:     except Exception as e:
    ...:         print(type(e))
    ...:         print(type(e.__cause__))
    ...:         print(isinstance(e.__cause__, psycopg.errors.UniqueViolation))
    ...: 
<class 'django.db.utils.IntegrityError'>
<class 'psycopg.errors.UniqueViolation'>
True

Ressources :

• https://www.postgresql.org/docs/current/errcodes-appendix.html
• https://www.psycopg.org/psycopg3/docs/api/errors.html
• https://docs.python.org/3/library/exceptions.html#exception-context

[rsebille]

Ta proposition @rsebille m'a fait des nœuds dans le cerveau, je ne suis pas habitué à autant d'abstraction 😅

Alors faut pas lire mes nouveaux commentaires .

[rsebille]

J'aillais dire "ouais ça m'étonne qu'à moitié" et puis mon cerveau c'est dit "There must be a better way", et il semblerais que oui 😁.

PostgreSQL aillant un code d'erreur dédié et psycopg faisant bien les choses il y a une exception dédiée pour les contraintes unique : https://github.com/psycopg/psycopg/blob/9bde1475f0e77beaf285d72f97c9ead721e8cd94/psycopg/psycopg/errors.py#L1037-L1039
par contre Django intercepte cette exception et la réécris en django.db.utils.IntegrityError (à ne pas confondre avec psycopg.errors.IntegrityError ;)) donc faut creuser un peu dans l'exception mais ceci devrais faire le boulot :

Suggested change

	if "unique constraint" not in e.args[0]:
	if not isinstance(e.__cause__, psycopg.errors.UniqueViolation)

Cas synthétique de test :

In [27]: with transaction.atomic():
    ...:     try:
    ...:         obj.save()
    ...:     except Exception as e:
    ...:         print(type(e))
    ...:         print(type(e.__cause__))
    ...:         print(isinstance(e.__cause__, psycopg.errors.UniqueViolation))
    ...: 
<class 'django.db.utils.IntegrityError'>
<class 'psycopg.errors.UniqueViolation'>
True

Ressources :

• https://www.postgresql.org/docs/current/errcodes-appendix.html
• https://www.psycopg.org/psycopg3/docs/api/errors.html
• https://docs.python.org/3/library/exceptions.html#exception-context

[rsebille]

Ici il ne manquerais pas le cas "is_admin in from_company" ? (True, True, True, False, True, True), et est-ce que ça vaut le coup de tester le cas où on n'a pas de membership dans la source ? Je suppose qu'on fait confiance au fonctionnement globale de la commande dans ce cas là 🤔.

Et PI tu peux stack les parametrize() ça permet d'éviter d'avoir a gérer la matrice des cas soit même :

diff --git a/tests/companies/test_admin.py b/tests/companies/test_admin.py
index 2dd9ae11e..3bbdb2137 100644
--- a/tests/companies/test_admin.py
+++ b/tests/companies/test_admin.py
@@ -356,44 +356,34 @@ class TestTransferCompanyData:
         )
         assert "Peut apparaître dans la recherche:\n  * is_searchable: False remplacé par True" in remark
 
-    @pytest.mark.parametrize(
-        "is_active_in_from_company,is_admin_in_from_company,is_active_in_to_company,is_admin_in_to_company,expected_is_active,expected_is_admin",
-        [
-            (False, False, False, False, False, False),  # "null" membership
-            (True, False, None, None, True, False),  # no membership in to_company: real transfer
-            (True, False, True, False, True, False),  # equivalent memberships: copy
-            (True, False, True, True, True, True),  # is_admin in to_company: copy
-            (True, True, True, True, True, True),  # equivalent admin memberships: copy
-        ],
-    )
+    @pytest.mark.parametrize("field", {"is_admin", "is_active"})
+    @pytest.mark.parametrize("value_in_from_company,value_in_to_company,expected", [
+        # (None, False, False),
+        (False, None, False),
+        # (None, True, True),
+        (True, None, True),
+        (False, False, False),
+        (True, False, True),
+        (False, True, True),
+        (True, True, True),
+    ])
     def test_transfer_data_memberships(
         self,
         admin_client,
-        is_active_in_from_company,
-        is_admin_in_from_company,
-        is_active_in_to_company,
-        is_admin_in_to_company,
-        expected_is_active,
-        expected_is_admin,
+        field,
+        value_in_from_company,
+        value_in_to_company,
+        expected,
     ):
         user = EmployerFactory()
+        default_args = {"user": user, "is_active": False, "is_admin": False}
         from_company = CompanyFactory(with_membership=False)
-        if is_active_in_from_company is not None:
-            membership = CompanyMembership()
-            membership.user = user
-            membership.company = from_company
-            membership.is_active = is_active_in_from_company
-            membership.is_admin = is_admin_in_from_company
-            membership.save()
+        if value_in_from_company is not None:
+            CompanyMembership(company=from_company, **{**default_args, field: value_in_from_company}).save()
 
         to_company = CompanyFactory(with_membership=False)
-        if is_active_in_to_company is not None:
-            membership = CompanyMembership()
-            membership.user = user
-            membership.company = to_company
-            membership.is_active = is_active_in_to_company
-            membership.is_admin = is_admin_in_to_company
-            membership.save()
+        if value_in_to_company is not None:
+            CompanyMembership(company=to_company, **{**default_args, field: value_in_to_company}).save()
 
         transfer_url = reverse(
             "admin:transfer_company_data", kwargs={"from_company_pk": from_company.pk, "to_company_pk": to_company.pk}
@@ -421,11 +411,10 @@ class TestTransferCompanyData:
         from_company.refresh_from_db()
         to_company.refresh_from_db()
 
-        if is_active_in_to_company is None:
+        if value_in_to_company is None:
             # Real transfer, membership in from_company is moved
             assert from_company.memberships.count() == 0
         else:
             assert from_company.memberships.count() == 1
         assert to_company.memberships.count() == 1
-        assert to_company.memberships.first().is_active == expected_is_active
-        assert to_company.memberships.first().is_admin == expected_is_admin
+        assert getattr(to_company.memberships.first(), field) is expected