Skip to content
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

User moderation: Delete a user's communities on block #1023

Merged
merged 5 commits into from
Sep 25, 2023

Conversation

max-moser
Copy link
Contributor

When a user gets blocked, delete all communities of which they are the only owner

@@ -463,18 +448,27 @@ def featured_delete(
# Deletion workflows
#
@unit_of_work()
def delete_community(self, identity, id_, data, expand=False, uow=None):
def delete_community(
self, identity, id_, data=None, revision_id=None, expand=False, uow=None
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the revision_id is accepted but ignored because the old signature (and the resource actually provides a value from the if_match request context header)

not sure if that's the way to go?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines -260 to -262
# Set a record to 'metadata only' if its files got cleaned up
if not record.files.entries:
record.files.enabled = False
Copy link
Contributor Author

@max-moser max-moser Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that caused the community's settings page to break after a restore...
it's just relevant for records (where it was copied from), but not for communities

Comment on lines 113 to 116
current_user_filter = CommunityMembers().query_filter(identity) & dsl.Q(
"term", **{"deletion_status": CommunityDeletionStatusEnum.PUBLISHED.value}
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will certainly hide deleted records from the search results for the user's communities (without it, they'd still pop up)

maybe this should be done via generator query filters somehow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PIng @kpsherva

Comment on lines +38 to +50
# collect the owners for each community
comm_owners = defaultdict(list)
for comm_owner in [
mem_cls(m.data, model=m)
for m in mem_model_cls.query.filter(mem_model_cls.role == "owner").all()
]:
comm_owners[comm_owner.community_id].append(comm_owner)

# filter for communities that are owned solely by the user in question
relevant_comm_ids = [
comm_id
for comm_id, owners in comm_owners.items()
if len(owners) == 1 and str(owners[0].user_id) == user_id
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have a feeling that this should be doable with a group_by and/or having for better performance

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have a helper function to get all memberships of a user. It returns though all memberships and not filtered by a specific role but it could be improved. Ping @ntarocco I think in this case the performance issue with getting all communities of a user from DB is done on demand so it should not affect overall performance right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be discussed, it seems to me that it is not a blocker for now

@max-moser max-moser force-pushed the mm/community-deletion branch 2 times, most recently from 75cf6ea to e4574f0 Compare September 20, 2023 16:21
@max-moser max-moser linked an issue Sep 20, 2023 that may be closed by this pull request
self, identity, id_, data=None, expand=False, revision_id=None, uow=None
):
"""Alias for ``delete_community()``."""
return self.delete_community(identity, id_, data, expand=expand, uow=uow)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we just keep the delete and we introduce a delete_community?

@@ -463,18 +448,27 @@ def featured_delete(
# Deletion workflows
#
@unit_of_work()
def delete_community(self, identity, id_, data, expand=False, uow=None):
def delete_community(
self, identity, id_, data=None, revision_id=None, expand=False, uow=None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +38 to +50
# collect the owners for each community
comm_owners = defaultdict(list)
for comm_owner in [
mem_cls(m.data, model=m)
for m in mem_model_cls.query.filter(mem_model_cls.role == "owner").all()
]:
comm_owners[comm_owner.community_id].append(comm_owner)

# filter for communities that are owned solely by the user in question
relevant_comm_ids = [
comm_id
for comm_id, owners in comm_owners.items()
if len(owners) == 1 and str(owners[0].user_id) == user_id
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have a helper function to get all memberships of a user. It returns though all memberships and not filtered by a specific role but it could be improved. Ping @ntarocco I think in this case the performance issue with getting all communities of a user from DB is done on demand so it should not affect overall performance right?

Comment on lines 113 to 116
current_user_filter = CommunityMembers().query_filter(identity) & dsl.Q(
"term", **{"deletion_status": CommunityDeletionStatusEnum.PUBLISHED.value}
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PIng @kpsherva

max-moser and others added 3 commits September 23, 2023 17:34
* when a user gets blocked, delete all communities of which they are the
  only owner
* resources: add restore and delete communities actions
@kpsherva kpsherva force-pushed the mm/community-deletion branch 2 times, most recently from 9579fa3 to 6b8d966 Compare September 23, 2023 15:44
@kpsherva kpsherva force-pushed the mm/community-deletion branch from 6b8d966 to 54a0778 Compare September 23, 2023 15:44
@kpsherva kpsherva force-pushed the mm/community-deletion branch from a2e0a30 to cf1ed1c Compare September 25, 2023 08:40
@kpsherva kpsherva merged commit aa44546 into inveniosoftware:master Sep 25, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

communities: tombstone and restore
3 participants