Add support for teams [556]. #12
Conversation
twittner
force-pushed
the
teams
branch
2 times, most recently
from
79c5100 to
4986d3e
Compare
| <*> o .: "type" | ||
| <*> o .: "creator" | ||
| <*> o .: "access" | ||
| <*> o .:? "name" |
There was a problem hiding this comment.
Interesting, this was not optional before?
There was a problem hiding this comment.
Apparently not, but as you can see in the definition of Conversation above, name is an optional field.
| , _teamCreator :: UserId | ||
| , _teamName :: Text | ||
| , _teamIcon :: Text | ||
| , _teamIconKey :: Maybe Text |
There was a problem hiding this comment.
What is the intended use/differentiation between teamIcon and teamIconKey?
There was a problem hiding this comment.
teamIconKey denotes the asset key in case the team icon asset is not meant to be public.
| markTeamDeleted = "update team set deleted = true where team = ?" | ||
|
|
||
| deleteTeam :: PrepQuery W (Identity TeamId) () | ||
| deleteTeam = "delete from team using timestamp 32503680000000000 where team = ?" |
There was a problem hiding this comment.
Trying to educate myself here, looking at the docs this would mean deleting values older than this timestamp. What is the benefit of setting it here versus not setting any timestamp?
There was a problem hiding this comment.
My understanding is from http://cassandra.apache.org/doc/latest/cql/dml.html#delete:
DELETEsupports theTIMESTAMPoption with the same semantics as in updates. [...]
TIMESTAMP: sets the timestamp for the operation. If not specified, the coordinator will use the current time (in microseconds) at the start of statement execution as the timestamp.
The idea being that a future timestamp increases the chance of not having deleted data revived by repairs due to clockdrift.
There was a problem hiding this comment.
The idea being that a future timestamp increases the chance of not having deleted data revived by repairs due to clockdrift.
I see 👍
| deleteTeamMember = "delete from team_member where team = ? and user = ?" | ||
|
|
||
| updatePermissions :: PrepQuery W (Permissions, TeamId, UserId) () | ||
| updatePermissions = "update team_member set perms = ? where team = ? and user = ?" |
There was a problem hiding this comment.
AFAIK, this is never used - was it meant to be exposed over the API?
There was a problem hiding this comment.
I expect it to be used eventually but did not get around to implement the API endpoint yet due to time pressure.
| errorResponse Error.noTeamMember | ||
| errorResponse (Error.operationDenied AddTeamMember) | ||
| errorResponse Error.notConnected | ||
| errorResponse Error.invalidPermissions |
There was a problem hiding this comment.
Can also return a tooManyTeamMembers Error?
| @@ -98,36 +103,62 @@ unblockConv (usr ::: conn ::: cnv) = do | |||
| joinConversation :: UserId ::: ConnId ::: ConvId ::: JSON -> Galley Response | |||
| joinConversation (zusr ::: zcon ::: cnv ::: _) = do | |||
| c <- Data.conversation cnv >>= ifNothing convNotFound | |||
| when (Data.isConvDeleted c) $ do | |||
| Data.deleteConversation cnv | |||
| throwM convNotFound | |||
There was a problem hiding this comment.
Is there an instance of calling Data.conversation where we do not want to delete a conversation from cassandra is it's marked as deleted? Just wondering if we could/should instead use something like lookupAndDeleteIfDeleted (or something a bit easier to read) or even change the semantics of the lookup?
There was a problem hiding this comment.
Presumably we can make this transparent by moving it into Data.conversation and return Nothing in those cases. I would suggest to do that little refactoring later though.
|
Besides my nitpicks, |
| @@ -233,6 +242,14 @@ assertMatchN t wss f = awaitMatchN t wss f >>= mapM assertSuccess | |||
| assertSuccess :: (MonadIO m, MonadThrow m) => Either MatchTimeout Notification -> m Notification | |||
| assertSuccess = either throwM return | |||
|
|
|||
| assertNoEvent :: (MonadIO m, MonadCatch m) => Timeout -> [WebSocket] -> m () | |||
| assertNoEvent t ww = do | |||
| results <- awaitMatchN t ww (const $ pure ()) | |||
There was a problem hiding this comment.
The following might be a bit nicer and yield better error messages (due to throwing HUnitFailures instead of userErrors):
assertNoEvent t ww = void $ assertMatchN t ww (assertFailure . f)
where
f n = "unexpected notification received: " ++ show n
services/galley/src/Galley/Data.hs
Outdated
| retry x5 $ batch $ do | ||
| setType BatchLogged | ||
| setConsistency Quorum | ||
| addPrepQuery Cql.deleteTeamConv (tid, cid) |
There was a problem hiding this comment.
A logged batch for a single query?
services/galley/src/Galley/Data.hs
Outdated
|
|
||
| updateTeam :: MonadClient m => TeamId -> TeamUpdateData -> m () | ||
| updateTeam tid u = retry x5 $ batch $ do | ||
| setType BatchLogged |
There was a problem hiding this comment.
Since there doesn't seem to be an important consistency requirement between team name and icon, is a logged batch really necessary?
There was a problem hiding this comment.
I chose to use it because of the coupling between icon and icon_key updates.
|
|
||
| isConvAlive :: MonadClient m => ConvId -> m Bool | ||
| isConvAlive cid = do | ||
| result <- retry x1 (query1 Cql.isConvDeleted (params Quorum (Identity cid))) |
There was a problem hiding this comment.
Given the read / write ratio of this flag, wouldn't it be preferable to use consistency One here and, if really necessary, consistency All for the write?
) * Implement basic auth in a simpler way (without contexts etc) * Throw errors via ExceptT * Cleanup CCP switch for servant compatibility. (#13) * Throw authorization errors as 'SCIMError's * Move language extensions to package.yaml * Export siteServer * Add 'serverError' * Make 'decodeAuth' more informative * Remove TestStorage duplication * Add all the necessary instances for FromJSON StoredUser
…12) * Documented how to recover an etcd cluster where one node got corrupt
No description provided.