WPB-26704: add full conversation to POST /meetings (create) and PUT /meetings/{domain}/{id}#5301
WPB-26704: add full conversation to POST /meetings (create) and PUT /meetings/{domain}/{id}#5301blackheaven wants to merge 3 commits into
conversation to POST /meetings (create) and PUT /meetings/{domain}/{id}#5301Conversation
zebot
added
the
ok-to-test
| domain <- meeting %. "qualified_id" %. "domain" >>= asString | ||
| pure (meetingId, domain) | ||
|
|
||
| -- | On create/update responses, the full @conversation@ object is returned |
There was a problem hiding this comment.
I usually put function declarations after their first call. Not very important, but it improves my reading flow.
|
|
||
| -- | A 'Meeting' extended with the full 'Conversation' associated with it, as | ||
| -- returned when creating or updating a meeting. | ||
| data MeetingWithConversation = MeetingWithConversation |
There was a problem hiding this comment.
The old Meeting is still around, right? So, we have to keep data MeetingWithConversation and data Meeting aligned forever.
I'm wondering if it wouldn't be better to have a data type like:
data MeetingWithConversation = MeetingWithConversation {
meeting :: Meeting,
conversation :: Conversation
}(could be an isomorph tupel as well)
and combine these things in the ToSchema instance?
There was a problem hiding this comment.
Or, if that's easier, you could keep MeetingWithConversation, but only use it in the frontend interface; i.e. in ToSchema and Servant types.
There was a problem hiding this comment.
It hurts me because it did not cross my mind, thanks, changed!
| ( storedMeetingToMeeting (tDomain zUser) storedMeeting, | ||
| storedConv | ||
| ) | ||
| -- Return created meeting with its conversation |
There was a problem hiding this comment.
| -- Return created meeting with its conversation |
IMHO we don't need this comment.
| instance ToSchema MeetingWithConversation where | ||
| schema = | ||
| objectWithDocModifier (description ?~ "A scheduled meeting with its associated conversation") $ | ||
| MeetingWithConversation | ||
| <$> (.meeting) .= meetingObject | ||
| <*> (.conversation) .= field "conversation" schema |
There was a problem hiding this comment.
This will change the JSON format for clients, though
There was a problem hiding this comment.
I've just checked, it looks fine
| conv <- MaybeT $ ConversationSubsystem.internalGetConversation updatedMeeting.conversationId | ||
| pure $ storedMeetingToMeetingWithConversation zUser conv updatedMeeting |
There was a problem hiding this comment.
IMHO this deserves at least a log entry if conv is Nothing. Otherwise, this may fail silently, indicating that the meeting doesn't exist.
| meeting.meeting.title `shouldBe` fromJust (checked "Test Meeting") | ||
| meeting.conversation.qualifiedId `shouldBe` meeting.meeting.conversationId | ||
| (fetched <&> (.id)) `shouldBe` Just meeting.meeting.id |
There was a problem hiding this comment.
We weakened assertions here (full equality vs. a couple of fields).
Is this fine? Or, should be reintroduce a full equality check?
There was a problem hiding this comment.
I guess the comment meant to be above.
I'm a bit confused because createMeeting gives MeetingWithConversation and getMeeting Maybe Meeting.
Should I also fetch Conversation and build MeetingWithConversation to compare them?
| API.MeetingWithConversation | ||
| storedMeetingToMeetingWithConversation lUser conv sm = | ||
| API.MeetingWithConversation | ||
| { API.meeting = storedMeetingToMeeting (tDomain lUser) sm, |
There was a problem hiding this comment.
Is there a federation case for meetings? If so, does this work with the domain of the local user?
There was a problem hiding this comment.
Meetings are not federated: every CRUD operation guards qDomain meetingId == tDomain zUser (see updateMeetingImpl, deleteMeetingImpl, getMeetingImpl, addInvitedEmailsImpl, removeInvitedEmailsImpl, replaceInvitedEmailsImpl), and the conversation is always created locally via internalCreateGroupConversation zUser. So using tDomain lUser to qualify the meeting / creator / conversation — and conversationView lUser (Just lUser) conv — is correct. I added a short note on storedMeetingToMeetingWithConversation documenting this invariant (0675d7b).
blackheaven
force-pushed
the
gdifolco/WPB-26704-meeting-full-conversation
branch
4 times, most recently
from
efdc635 to
3b9434c
Compare
…PUT /meetings/{domain}/{id}`
Wrap MeetingWithConversation around Meeting + Conversation instead of duplicating Meeting's fields. Reuse a shared meetingObject schema so the flattened JSON (and OpenAPI) output is unchanged. Also simplify storedMeetingToMeetingWithConversation to delegate to storedMeetingToMeeting, drop a redundant comment, and move test helpers after their first call.
- Log a warning when a meeting's conversation is missing in updateMeetingImpl/deleteMeetingImpl (previously failed silently, indistinguishable from a missing meeting for callers) - Restore full-equality assertion in the create-and-retrieve test (compare fetched against meeting.meeting) - Document the non-federation invariant on storedMeetingToMeetingWithConversation
blackheaven
force-pushed
the
gdifolco/WPB-26704-meeting-full-conversation
branch
from
e104a72 to
a4b5a10
Compare
3 participants
https://wearezeta.atlassian.net/browse/WPB-26704
Checklist
changelog.d