-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(plg): Use react-query for team management #63267
Conversation
client/web/src/cody/management/api/react-query/subscriptions.ts
Outdated
Show resolved
Hide resolved
46ba421
to
102a073
Compare
const queryClient = useQueryClient() | ||
return useMutation({ | ||
mutationFn: async requestBody => callCodyProApi(Client.updateTeamMember(requestBody)), | ||
onSettled: () => queryClient.invalidateQueries({ queryKey: queryKeys.teams.teamMembers() }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the request to update fails, do we still need to invalidate team members?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, as the mutationFn
resolves the updated team members list, same as get team members call
https://github.com/sourcegraph/sourcegraph/blob/cc1cd46cfb9acc3225cd9139c5f29a53e463adf6/client/web/src/cody/management/api/client.ts#L51-L57
instead of invalidating team members in React Query cache, we can update it directly, e.g. like here
https://github.com/sourcegraph/sourcegraph/blob/cc1cd46cfb9acc3225cd9139c5f29a53e463adf6/client/web/src/cody/management/api/react-query/subscriptions.ts#L51-L60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the request to update fails, do we still need to invalidate team members?
It was not a conscious decision, but now after thinking about it, I think it'd hurt us more if we didn't do this.
Like, what if there was an error for some reason but the update still affected the DB?
So it feels safer to invalidate it regardless.
It's a rare edge case, so I'd not think a lot about it.
Also, as the mutationFn resolves the updated team members list, same as get team members call
Thanks, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I misread the second suggestion at first. Good idea, I'll do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I think we don't need the invalidation line. Am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what you meant, @taras-yemets? https://github.com/sourcegraph/sourcegraph/commit/9311bba7a6c8676beb1e378d3a3d481c57433c3e
onSuccess: (data: TeamMember[]) => {
queryClient.setQueryData(queryKeys.teams.teamMembers(), data)
}
We already updated the cache record with the key queryKeys.teams.teamMembers()
, no need to invalidate it (don't return anything from success handler).
I think it'd hurt us more if we didn't do this.
Like, what if there was an error for some reason but the update still affected the DB?
I expect backend to handle unsuccessful DB transactions (e.g., rollback). But I agree, it doesn't hurt.
TBH, I think we don't need the invalidation line. Am I right?
If we remove the team member (or change their role) in this mutation, useTeamMembers
query needs to be invalidated. Otherwise it still keeps the old state with the team member existing (or with "old" role). WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Is this even better, then? https://github.com/sourcegraph/sourcegraph/pull/63267/commits/f27539367a4793a6d8ea55f376ac968449d4f8de
(I've also updated the component to simplify it, which I forgot earlier.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Is this even better, then? https://github.com/sourcegraph/sourcegraph/commit/f27539367a4793a6d8ea55f376ac968449d4f8de
👍🏻
I've also updated the component to simplify it
someMutation.mutateAsync.call
seems overcomplicated to me (more on it in https://github.com/sourcegraph/sourcegraph/pull/63227#discussion_r1644225613).
const queryClient = useQueryClient() | ||
return useMutation({ | ||
mutationFn: async requestBody => callCodyProApi(Client.sendInvite(requestBody)), | ||
onSuccess: () => queryClient.invalidateQueries({ queryKey: queryKeys.invites.teamInvites() }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link is broken but I guess you meant this? https://github.com/sourcegraph/sourcegraph/pull/63267/commits/65b09e80b62199311ae2a196b580732a6075d16f
(the resend endpoint returns an empty response, not the invite)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it refers to updating the cache from response instead of invalidating the query (similar to https://github.com/sourcegraph/sourcegraph/pull/63267#discussion_r1642617096).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3,8 +3,8 @@ export type TeamRole = 'member' | 'admin' | |||
export interface TeamMember { | |||
accountId: string | |||
displayName: string | |||
email: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking: do we have an accompanying PR to SSC repo exposing this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should already expose the field because the old approach was already using it. I'll double-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we don't expose it: convert function, type definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, weird. I'll add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const [invitesResponse, invitesDataError] = useSSCQuery<{ invites: TeamInvite[] }>('/team/current/invites') | ||
const teamInvites = invitesResponse?.invites | ||
const subscriptionQueryResult = useCurrentSubscription() | ||
const isPro = subscriptionQueryResult.data?.subscriptionStatus !== 'canceled' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isPro
may also be true
if subscriptionQueryResult
is loading or errored out.
Later in useEffect
we use this value to decide whether we need to navigate to "/cody/manage" page. I think the comment may helpf to inform the reader that "data is loaded, we know user's plan, it's not on a Pro, redirect them to another page". WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch. Fixed in https://github.com/sourcegraph/sourcegraph/pull/63267/commits/bb2ab106c20cf970466149e34207f2e840f24ce2. Does it make sense now?
@@ -42,7 +45,7 @@ export const InviteUsers: React.FunctionComponent<InviteUsersProps> = ({ | |||
try { | |||
const responses = await Promise.all( | |||
emailAddresses.map(emailAddress => | |||
requestSSC('/team/current/invites', 'POST', { email: emailAddress, role: 'member' }) | |||
sendInviteMutation.mutateAsync.call(undefined, { email: emailAddress, role: 'member' }) | |||
) | |||
) | |||
if (responses.some(response => response.status !== 200)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to revisit error-handling logic here after we switch custom mutation hooks?
sendInviteMutation
's mutateFn
uses callCodyProApi
which throws on non-2xx responses:
https://github.com/sourcegraph/sourcegraph/blob/cc1cd46cfb9acc3225cd9139c5f29a53e463adf6/client/web/src/cody/management/api/react-query/callCodyProApi.ts#L51-L61
Promise.all
rejects in any of the input promises rejects, thus we won't likely get to the following line:
if (responses.some(response => response.status !== 200)) {
and will get to the catch
block (disclaimer: it's an assumption, I haven't run the code myself).
Also, some of the invites may be sent successfully before one fails. We may want to keep track of what invites exactly failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to keep invitesSendingStatus
and invitesSendingErrorMessage
in state? Can we derive these values from sendInviteMutation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we derive invitesSentCoiunt
from emailAddresses.length
and sendInviteMutation
status?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to revisit error-handling logic here after we switch custom mutation hooks?
Also, some of the invites may be sent successfully before one fails. We may want to keep track of what invites exactly failed.
Completely rewrote error handling here: https://github.com/sourcegraph/sourcegraph/pull/63267/commits/22ce799f3485e47c06d01d858e1892eac36400fc
Do we still need to keep
invitesSendingStatus
andinvitesSendingErrorMessage
in state? Can we derive these values fromsendInviteMutation
?
Can we derive
invitesSentCount
fromemailAddresses.length
andsendInviteMutation
status?
Did both in: https://github.com/sourcegraph/sourcegraph/pull/63267/commits/be05e798a50dcb4a6d0139d832db9b0ab089f20c
65b09e8
to
bb2ab10
Compare
Not for modifying, yet; that's coming up shortly.
3289c87
to
1f0e9cb
Compare
1f0e9cb
to
ce73390
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock.We can address remaining feedback (if any) in follow up PRs.
See the issue for context and the problem definition.
This PR uses
react-query
for data access, instead of the previous, custom solution.To code reviewers: Probably easiest to review commit by commit. Not too fat PR as a whole either, but I think it's even easier by the small chunks.
Test plan
I've manually tested that the team management page auto-updates upon various changes: