Servantify POST /connections endpoint; remove deprecated 'message' field#1726
Servantify POST /connections endpoint; remove deprecated 'message' field#1726
Conversation
jschaul
changed the title
jschaul
changed the title
jschaul
changed the title
jschaul
requested review from
akshaymankar,
mdimjasevic and
pcapriotti
and removed request for
akshaymankar
pcapriotti
left a comment
pcapriotti
left a comment
There was a problem hiding this comment.
This looks good.
I would suggest a minor improvement, possibly in a separate PR: now that you have nicely abstracted out this pattern of "existed200/created201", it would be useful to also abstract out the corresponding MultiVerb response list, so that the association between the two responses and the status code is fixed and users cannot get it wrong.
For example, we can add a type to the Util module like:
type ResponsesForExistedCreated eDesc cDesc a = '[
Respond 200 eDesc a,
Respond 201 cDesc a ]and use that as part of the MultiVerb type in the routes that use the pattern.
More suggestions below.
mdimjasevic
left a comment
mdimjasevic
left a comment
There was a problem hiding this comment.
I left some comments inlined.
Overall, this looks good, though I wish more things were reflected in types given the move to Servant.
| crName :: Text, | ||
| -- | Initial message | ||
| crMessage :: Message | ||
| -- FUTUREWORK investigate: shouldn't this name be optional? Do we use this name actually anywhere? |
There was a problem hiding this comment.
I'm just guessing now, but does a connection name ever make it to a one-to-one conversation title?
Clients have a logic related to conversation titles (e.g., in checking if a group conversation in a team is logically one-to-one conversation).
There was a problem hiding this comment.
I'm just guessing now, but does a connection name ever make it to a one-to-one conversation title?
I'm not sure, I didn't take the time to look more deeply into how this is used and what clients send and receive and what they do with it. That's why there is a FUTUREWORK here. A potential refactor that removes this can be done in a subsequent PR.
| P.object "ConnectionRequest" $ | ||
| ConnectionRequest | ||
| <$> crUser P..= P.fieldWithDocModifier "user" (P.description ?~ "user ID of the user to request a connection with") P.schema | ||
| <*> crName P..= P.fieldWithDocModifier "name" (P.description ?~ "Name of the (pending) conversation being initiated (1 - 256) characters)") P.schema |
There was a problem hiding this comment.
I'm not too familiar with connection names and conversation names/titles, but see my previous comment if a connection name is really promoted to a one-to-one conversation title.
improve noIdentity error message ConnectionResult -> ResponseForExistedCreated link to docs in FUTUREWORK comment about cassandra migrations add futurework about 'Connect' event. remove redundant golden tests.
|
I tried to address all your review comments now. |
jschaul
changed the title
3 participants
POST /connectionsmessagefield in connection requests, as it has been long deprecated. Some clients may still send it (as it was a mandatory field), but it was never visible at UI level in the past years, so we can remove it.The main changeset to types is in libs/wire-api/src/Wire/API/Connection.hs
Some golden tests have been removed as they are no longer relevant.
Checklist