Skip to content

Commit 70993ec

Browse files
committed
fix(organisations): retain an active admin when cancelling users
`cancel_users` picked the earliest-joined admin without regard to `is_active`. A deactivated admin could therefore be retained while every active membership was deleted, leaving the organisation with no user able to access it and no way to undo it. Prefer the earliest active admin, fall back to the earliest active member, and no-op when no seat is in use \u2014 which also removes the latent `AttributeError` when the organisation has no admin at all. Ref: #8368
1 parent cb13a47 commit 70993ec

2 files changed

Lines changed: 82 additions & 9 deletions

File tree

api/organisations/models.py

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -209,21 +209,30 @@ def rebuild_environments(self): # type: ignore[no-untyped-def]
209209
).values_list("id", flat=True):
210210
rebuild_environment_document.delay(args=(environment_id,))
211211

212-
def cancel_users(self): # type: ignore[no-untyped-def]
212+
def cancel_users(self) -> None:
213+
"""
214+
Reduce the organisation to the single seat the free plan allows.
215+
216+
The retained member must hold an active membership, otherwise the
217+
organisation would be left with nobody able to access it.
218+
"""
219+
active_memberships = UserOrganisation.objects.filter(
220+
organisation=self,
221+
is_active=True,
222+
)
213223
remaining_seat_holder = (
214-
UserOrganisation.objects.filter(
215-
organisation=self,
216-
role=OrganisationRole.ADMIN,
217-
)
224+
active_memberships.filter(role=OrganisationRole.ADMIN)
218225
.order_by("date_joined")
219226
.first()
227+
or active_memberships.order_by("date_joined").first()
220228
)
229+
if remaining_seat_holder is None:
230+
# No seat is in use, so there is nothing to cancel down to.
231+
return
221232

222233
UserOrganisation.objects.filter(
223234
organisation=self,
224-
).exclude(
225-
id=remaining_seat_holder.id # type: ignore[union-attr]
226-
).delete()
235+
).exclude(id=remaining_seat_holder.id).delete()
227236

228237

229238
class UserOrganisation(LifecycleModelMixin, models.Model): # type: ignore[misc]
@@ -389,7 +398,7 @@ def prepare_for_cancel( # type: ignore[no-untyped-def]
389398

390399
if cancellation_date <= timezone.now():
391400
# Since the date is immediate, wipe data right away.
392-
self.organisation.cancel_users() # type: ignore[no-untyped-call]
401+
self.organisation.cancel_users()
393402
self.save_as_free_subscription() # type: ignore[no-untyped-call]
394403
return
395404

api/tests/unit/organisations/test_unit_organisations_deactivated_membership.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
1+
from datetime import timedelta
2+
13
import pytest
24
from django.urls import reverse
5+
from django.utils import timezone
36
from rest_framework import status
47
from rest_framework.test import APIClient
58

@@ -243,3 +246,64 @@ def test_add_organisation__new_membership__is_active(
243246

244247
# Then
245248
assert user.belongs_to(organisation.id) is True
249+
250+
251+
def test_cancel_users__deactivated_earliest_admin__retains_an_active_admin(
252+
organisation: Organisation,
253+
admin_user: FFAdminUser,
254+
staff_user: FFAdminUser,
255+
) -> None:
256+
# Given
257+
# The earliest-joined admin has left and been deactivated, so retaining
258+
# them would leave nobody able to access the organisation.
259+
founding_admin = FFAdminUser.objects.create(email="founder@example.com")
260+
founding_admin.add_organisation(organisation, role=OrganisationRole.ADMIN)
261+
UserOrganisation.objects.filter(
262+
user=founding_admin, organisation=organisation
263+
).update(date_joined=timezone.now() - timedelta(days=365))
264+
founding_admin.set_organisation_membership_active(organisation, is_active=False)
265+
266+
# When
267+
organisation.cancel_users()
268+
269+
# Then
270+
assert admin_user.belongs_to(organisation.id) is True
271+
assert organisation.num_seats == 1
272+
assert not UserOrganisation.objects.filter(
273+
user=founding_admin, organisation=organisation
274+
).exists()
275+
276+
277+
def test_cancel_users__no_active_admin__retains_earliest_active_member(
278+
organisation: Organisation,
279+
admin_user: FFAdminUser,
280+
staff_user: FFAdminUser,
281+
) -> None:
282+
# Given
283+
# Every admin is deactivated, leaving only a regular member holding a seat.
284+
admin_user.set_organisation_membership_active(organisation, is_active=False)
285+
286+
# When
287+
organisation.cancel_users()
288+
289+
# Then
290+
assert staff_user.belongs_to(organisation.id) is True
291+
assert organisation.num_seats == 1
292+
293+
294+
def test_cancel_users__no_active_memberships__is_a_noop(
295+
organisation: Organisation,
296+
admin_user: FFAdminUser,
297+
staff_user: FFAdminUser,
298+
) -> None:
299+
# Given
300+
# Nothing is holding a seat, so there is nothing to cancel down to.
301+
admin_user.set_organisation_membership_active(organisation, is_active=False)
302+
staff_user.set_organisation_membership_active(organisation, is_active=False)
303+
304+
# When
305+
organisation.cancel_users()
306+
307+
# Then
308+
assert organisation.num_seats == 0
309+
assert UserOrganisation.objects.filter(organisation=organisation).count() == 2

0 commit comments

Comments
 (0)