feat(api): Add "close account" API endpoint #7146
Conversation
Generated by 🚫 danger |
| Also removes organizations if they are an owner | ||
| :pparam string user_id: user id | ||
| :param list organizations: List of organization ids to remove |
There was a problem hiding this comment.
i'm wondering if we should actually have people send a list or if we should just delete all of the ones that should be deleted automatically? do we allow people to delete their account without removing the single owner orgs?
There was a problem hiding this comment.
Yeah I was thinking the same thing - the existing behavior is that single owner orgs get listed and always get deleted.
If it is an org w/ multiple owners, you can choose to delete it (but maybe it's better to not be able to delete those at all?)
There was a problem hiding this comment.
oh interesting. i didn't realize that you could also delete non-single owner orgs from here. hmmmm i mean i think in the interest of plowing through the setting stuff, it's probably a bad idea to change behavior, but seems like it'd be better if we forced people through the delete org flow when it's not single owner since it's so destructive. soo i'll defer to you/maybe you could check with chris to see if he's against removing the ability to delete those orgs from here?
There was a problem hiding this comment.
I'd make it explicit (send the list of orgs to remove). Otherwise it should probably:
- delete orgs explicitly selected
- delete orgs with no owners
- retain orgs with owners and not selected
Org deletion at least queues for 24 hours and emails all owners a way to undo it.
| status=OrganizationStatus.VISIBLE, | ||
| ) | ||
| org_results = [] | ||
| for org in sorted(org_list, key=lambda x: x.name): |
There was a problem hiding this comment.
since you aren't displaying this to the user, you probably don't need to sort
| }) | ||
|
|
||
| avail_org_slugs = set([o['organization'].slug for o in org_results]) | ||
| orgs_to_remove = set(request.DATA.get('organizations')).intersection(avail_org_slugs) |
There was a problem hiding this comment.
you should do set(request.DATA.get('organizations', [])) because set(None) raises a TypeError
and actually, you should probably do at least some minimal validation that if the key does exist, it's a list
dcramer
left a comment
dcramer
left a comment
There was a problem hiding this comment.
tests look like they cover what id expect
| Also removes organizations if they are an owner | ||
| :pparam string user_id: user id | ||
| :param list organizations: List of organization ids to remove |
There was a problem hiding this comment.
I'd make it explicit (send the list of orgs to remove). Otherwise it should probably:
- delete orgs explicitly selected
- delete orgs with no owners
- retain orgs with owners and not selected
Org deletion at least queues for 24 hours and emails all owners a way to undo it.
| """ | ||
|
|
||
| # be strict since we're removing orgs | ||
| if 'organizations' not in request.DATA or not isinstance( |
There was a problem hiding this comment.
you could use a validation/serializer thing to make this a bit more concise (and move the code into that)
|
|
||
| # from `frontend/remove_account.py` | ||
| org_list = Organization.objects.filter( | ||
| member_set__role=roles.get_top_dog().id, |
There was a problem hiding this comment.
i think we should just change this to be like member_set__role__in=[roles_with_org_delete_scope]
billyvg
force-pushed
the
feat/api/remove-account
branch
from
8e67727 to
bb1f479
Compare
Mostly existing code from
remove_account.py:Takes a list of organization slugs and removes the orgs if they are an owner, also removes ALL orgs of which user is the sole owner of.