[FS-926] Create new conversation type for global team conversation#2753
[FS-926] Create new conversation type for global team conversation#2753
Conversation
elland
changed the title
zebot
added
the
ok-to-test
elland
force-pushed
the
fs-926-add-global-team-conv
branch
from
7ce19ea to
413ebff
Compare
elland
force-pushed
the
fs-926-add-global-team-conv
branch
from
413ebff to
4d99a54
Compare
pcapriotti
left a comment
pcapriotti
left a comment
There was a problem hiding this comment.
There are a few issues, see comments below.
I can't find the code that produces the correct rendering of GTCs when fetched through the normal POST /conversations/list API. There is also no test for listing.
And one more thing that seems to be missing is creating the GTC entry on the list-ids endpoint, so that clients don't need to rely on the mutating semantics of the GET endpoint, just like what we discussed for the self conversation.
pcapriotti
left a comment
pcapriotti
left a comment
There was a problem hiding this comment.
Looks generally good, but I would consider changing the way the creator of a GTC is handled. Since a GTC doesn't really have a creator, I would just try to make the code reflect that, instead of forcing one. That means changing the types around a bit, but it shouldn't be a huge effort.
I'll approve for now and let you decide if it's worth doing that or not.
| ) | ||
| <!! do | ||
| const 201 === statusCode | ||
| const True === isJust . getHeader "Location" |
There was a problem hiding this comment.
why all this reformatting as part of this PR? Makes it much harder to see actual changes that happened.
There was a problem hiding this comment.
We had some issues with ormolu, I might have missed some places.
| Sem r Data.Conversation | ||
| getLocalConvForUser qusr lcnv = do | ||
| conv <- getConversation (tUnqualified lcnv) >>= noteS @'ConvNotFound | ||
| gtc <- getGlobalTeamConversationById lcnv |
There was a problem hiding this comment.
I don't like the sideeffects introduced here into each lookup of a conversation. This looks badly designed.
Elsewhere, it seems global team conversation id is the same as the team Id (even though this is not documented anywhere; and it means there can only ever be one single global team conversation). Can't this mean you can tell earlier, if you have a user object available, you'd know their team, and already know which kind of conv you're dealing with, without an extra lookup?
And why does a GET of any conversation ID lead to an insert of a random user as creator of this conversation? That makes no sense semantically.
There was a problem hiding this comment.
We're taking this out in a follow-up 👍
There was a problem hiding this comment.
I don't like the sideeffects introduced here into each lookup of a conversation. This looks badly designed.
Global team conversations and self conversations are not explicitly created, they are always there conceptually. To keep things consistent with other conversations, we still create records for them in the database at the first chance we get. Namely, they are created when first accessed or when conversations are listed. This way, the creation side effect cannot be observable from the outside.
The reason for this design is to limit the number of places where we have to take some kind of hacky "lock", which is pretty much impossible to do reliably with cassandra. We are already taking one of these locks when processing a commit (and there is no way around that). With this design, we don't need to figure out a way to deal with multiple concurrent creations of the conversation, since all mutable actions happen within commits, and creation itself is idempotent.
And why does a GET of any conversation ID lead to an insert of a random user as creator of this conversation? That makes no sense semantically.
I agree, the creator should not be set on query. That would make creation not idempotent.
* Revert "[FS-1267] Patch issue with initial commit bundle throwing a global team conversation error (#2887)" This reverts commit 1cc2bd2. * Revert "Commented out GTC for release. (#2879)" This reverts commit 49da310. * Revert "Improve global team conversation handling and self conversation creation error. (#2862)" This reverts commit 381bf7b. * Revert "[FS-926] Create new conversation type for global team conversation (#2753)" This reverts commit c4c9ea2.
5 participants
This should cover FS-927, as the endpoint has been added.
Checklist
changelog.d