-
Notifications
You must be signed in to change notification settings - Fork 333
WPB-21964: Add Wire Meetings delete #5066
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
base: develop
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| `DELETE /meetings/:domain/:meetingId` for deleting meetings. | ||
|
|
||
| Authorization: only the meeting creator can delete the meeting. | ||
| Validity: meetings that ended too long ago cannot be deleted (configurable validity period). | ||
|
|
||
| When a meeting is deleted, the associated MLS conversation is also deleted if it's a MeetingConversation type. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,8 @@ interpretMeetingsSubsystem validityPeriod = interpret $ \case | |
| createMeetingImpl zUser newMeeting | ||
| UpdateMeeting zUser meetingId update -> | ||
| updateMeetingImpl zUser meetingId update validityPeriod | ||
| DeleteMeeting zUser meetingId -> | ||
| deleteMeetingImpl zUser meetingId validityPeriod | ||
| GetMeeting zUser meetingId -> | ||
| getMeetingImpl zUser meetingId validityPeriod | ||
|
|
||
|
|
@@ -167,6 +169,38 @@ updateMeetingImpl zUser meetingId update validityPeriod = do | |
| update.recurrence | ||
| pure $ storedMeetingToMeeting (tDomain zUser) updatedMeeting | ||
|
|
||
| deleteMeetingImpl :: | ||
| ( Member Store.MeetingsStore r, | ||
| Member ConversationSubsystem r, | ||
| Member Now r | ||
| ) => | ||
| Local UserId -> | ||
| Qualified MeetingId -> | ||
| NominalDiffTime -> | ||
| Sem r Bool | ||
| deleteMeetingImpl zUser meetingId validityPeriod = do | ||
| -- Get existing meeting | ||
| result <- | ||
| runMaybeT $ do | ||
| meeting <- MaybeT $ Store.getMeeting (qUnqualified meetingId) | ||
| now <- lift Now.get | ||
| let cutoff = addUTCTime (negate validityPeriod) now | ||
| guard $ meeting.endTime >= cutoff | ||
| -- Check authorization (only creator can delete) | ||
| guard $ meeting.creator == tUnqualified zUser | ||
| -- Delete meeting | ||
| lift $ Store.deleteMeeting (qUnqualified meetingId) | ||
| -- Delete associated conversation if it's a meeting conversation | ||
| let convId = meeting.conversationId | ||
| maybeConv <- lift $ ConversationSubsystem.getConversation convId | ||
| case maybeConv of | ||
| Just conv | ||
| | conv.metadata.cnvmGroupConvType == Just MeetingConversation -> | ||
| lift $ ConversationSubsystem.deleteConversation convId | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should make sure that the team -> conversation index also gets removed (I think only relevant for cassandra)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it be part of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure. It would be best if it would use the same code as normal conversation deletion through the conversation action. There is even much more to consider than just the team to conversation index. See: SConversationDeleteTag -> do
let deleteGroup groupId = do
E.removeAllMLSClients groupId
E.deleteAllProposals groupId
let cid = storedConv.id_
for_ (storedConv & mlsMetadata <&> cnvmlsGroupId . fst) $ \gidParent -> do
sconvs <- E.listSubConversations cid
for_ (Map.assocs sconvs) $ \(subid, mlsData) -> do
let gidSub = cnvmlsGroupId mlsData
E.deleteSubConversation cid subid
deleteGroup gidSub
deleteGroup gidParent
key <- E.makeKey (tUnqualified lcnv)
E.deleteCode key
case convTeam storedConv of
Nothing -> E.deleteConversation (tUnqualified lcnv)
Just tid -> E.deleteTeamConversation tid (tUnqualified lcnv)
pure $ mkPerformActionResult actionI don't know how to best achieve this, either.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #5068 |
||
| _ -> pure () | ||
| pure () | ||
| pure $ isJust result | ||
|
|
||
| getMeetingImpl :: | ||
| ( Member Store.MeetingsStore r, | ||
| Member ConversationSubsystem r, | ||
|
|
||
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.
This is a bit strange, I would assume the ConversationSubsystem to also handle authorization/team membership checks.
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.
I would agree, except that:
getLocalConvForUser, ingalleydo directly check permissions.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.
It is only delegating to ConversationStore, can't you use ConverstationStore directly where you need this? Otherwise it's the wrong abstraction, IMO.
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.
@akshaymankar reminded me that we should not use other's subsystems store in #4918 (comment)
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.
IDK, why shouldn't the meeting subsystem use the conversation store? Because a meeting is a conversation, right? (we should not be more catholic than the pope ;-))
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.
If you ask me, the right solution would be to move all the code related to conversation deletion from galley to the conversation subsystem, including all the permission checks and what not. Then call this from here and also from galley. This would also solve the other comment about the team to conversation index and other clean up operations that are still missing here. But this would take some more effort as there are a lot of dependencies.