Add/remove user from private channel #67
Conversation
Created using spr 1.3.4
Created using spr 1.3.4
| @@ -3,14 +3,12 @@ import { API_HOST } from 'src/client/consts/consts'; | |||
|
|
|||
| export function useAPIFetch<T extends object = object>( | |||
| path: string, | |||
| method: 'GET' | 'POST' | 'PUT' | 'DELETE' = 'GET', | |||
There was a problem hiding this comment.
Remove these changes which I made in #60
but then I realised there is already useAPIUpdateFetch below to use for these types of request (and passes body etc)
There was a problem hiding this comment.
It looks like most of this PR is UI wrangling -- I guess that means that the idea we had that controlling thread access like this, by adding and removing org members, was as straightforward as we thought it was going to be?
(this is a bit more permissive than Slack's model, which only lets the 'channel manager' remove people, but I think that's ok)
Agree. I mean at the limit we all have access to the signing key for Clack so we could just make API calls directly even if we aren't in the channel :P
I could do some optimistic rendering stuff here so it doesn't look broken for now, but I don't think it's worth the effort
Agree here as well.
| existingUsers: string[]; | ||
| } | ||
|
|
||
| export function AddUsersToChannelModal({ |
There was a problem hiding this comment.
This isn't used outside this file right?
| export function AddUsersToChannelModal({ | |
| function AddUsersToChannelModal({ |
There was a problem hiding this comment.
Yes! sorry originally I had it in another file but it uses a lot of the same styled components as the other modal so I put it back in the same file
| ); | ||
| } | ||
|
|
||
| interface AddUsersToChannelModalProps { |
There was a problem hiding this comment.
IIRC there are some weird properties of interfaces and so using a type is preferred in cases when either works?
| interface AddUsersToChannelModalProps { | |
| type AddUsersToChannelModalProps = { |
There was a problem hiding this comment.
Ah yes I think I just copied UsersInChannelModalProps and didn't even notice it was an interface
| <Avatar userId={user.id} enableTooltip /> | ||
| {/* //todo: fill short name values in db console? */} | ||
| <Name $variant="main"> | ||
| {user.shortName || user.name} |
There was a problem hiding this comment.
I believe @flooey is adding displayName to the API here? I guess it's not in the npm package yet.
| (a.shortName ?? a.name ?? 'Unknown')?.localeCompare( | ||
| b.shortName ?? b.name ?? 'Unknown', |
There was a problem hiding this comment.
Another nice case for displayName :)
| useEffect(() => { | ||
| if (!loading && hasMore) { | ||
| void fetchMore(50); | ||
| } | ||
| }, [hasMore, loading, fetchMore]); |
There was a problem hiding this comment.
Can we use a PaginationTrigger instead?
There was a problem hiding this comment.
Hmm I know I just approved your other PR but I think I'm getting a bit confused here. This code expects to know all the existing users in the channel, so that it can filter them out from a list of clack_all members. I'm not sure how this will work if we can't be sure we loaded all the channel members at the previous step. I think pagination could still work here, although I guess it might be a bit awkward because the org members you load might then get filtered away and so you might not see new results as you'd expect. Although I guess in that case it would load more again?
There was a problem hiding this comment.
You mean we need existingUsers to be a complete list, even if allOrgMembers doesn't need to be, right? Yeah I guess if there is one specific org we need everyone loaded in high up, that's fine -- also Clack doesn't need to be the cleanest code anyway :P
I'll hold off on landing #65 until you're all done in here, it's not an urgent one at all.
Created using spr 1.3.4
Wires up the server handlers added in #60
(and actually changes them slightly so that they take arrays of users in
the request body, rather than one user as a param)
Now, if you are in a private channel, you can add someone from clack_all
who is not already in the channel, and/or remove someone already in the
channel (this is a bit more permissive than Slack's model, which only lets
the 'channel manager' remove people, but I think that's ok)
Note that the current backend of the org members API doesn't handle
subscriptions, so it seems like nothing happens on add/delete, but if
you refresh you will see the updated list. Work is in progress to
fix that (with the new live query) - I could do some optimistic rendering
stuff here so it doesn't look broken for now, but I don't think it's
worth the effort
Test Plan:
after.mov