-
Notifications
You must be signed in to change notification settings - Fork 25
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
Réusinage de la désactivation d’un membre d’une organisation #5510
Conversation
0f30ea9
to
ea20f54
Compare
the member activity history. We need it to show to other members | ||
which job applications this user was managing before leaving the organization. | ||
""" | ||
membership.is_active = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je verrais bien un assert getattr(membership, self.members.source_field_name) == self
(ou équivalent) pour vérifier qu'on ne bricole pas les invitations d'une autre orga (et d'ailleurs, les tests échouent 😬 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonne remarque, mieux vaut être ceinture bretelles, comme on accepte le membership en paramètre, on devrait vérifier qu’il s’agit de la bonne organisation 👍
Un test échoue, parce qu’il est construit de manière incorrecte. J’ai ajouté un early commit pour le construire correctement.
The former employer must have a membership to sender_company, not another company.
The object being managed is a membership, not a member. The main reason for this renamed is for symmetry with upcoming `deactivate_membership`.
Allows keeping `add_or_activate_membership` close to the logic of `deactivate_membership` and providing a common interface to manage memberships.
ea20f54
to
93c2816
Compare
@@ -83,6 +83,25 @@ def add_or_activate_membership(self, user): | |||
is_admin = not self.members.exists() | |||
self.memberships.create(user=user, is_admin=is_admin) | |||
|
|||
def deactivate_membership(self, membership, *, updated_by): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je brancherais bien aussi l'admin sur cette méthode en remplaçant les case "delete" par un "désactiver" :)
(mais je peux le faire moi dans une prochaine PR à la limite)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je vais faire, on a déjà prévu ce branchement pour #5430.
🤔 Pourquoi ?
Faciliter les mises à jour futures.