SSC: Refactor team management#62694
Conversation
| // take care of exchanging the Sourcegraph session credentials for a SAMS access token. | ||
| // And then proxy the request onto the SSC backend, which will actually create the | ||
| // checkout session. | ||
| // TODO: Use fetchThroughSSCProxy instead of fetch. |
There was a problem hiding this comment.
We had this TODO in the code to do this and I meant to do it soon, so here we go.
| @@ -0,0 +1,40 @@ | |||
| import { useCallback } from 'react' | |||
There was a problem hiding this comment.
Extracted the response types to interfaces (they can be extended later if we need more data from those responses), and also our internal data types, which are a bit more processed. I put everything needed to use subscription data in this file. Same for invites and team members.
| authenticatedUser: AuthenticatedUser | ||
| } | ||
|
|
||
| // TODO: Remove this mock data |
There was a problem hiding this comment.
This is less helpful now so cleaned it up.
| const newSeatsPurchased: number | null = newSeatsPurchasedParam ? parseInt(newSeatsPurchasedParam, 10) : null | ||
|
|
||
| // Load data | ||
| const [subscriptionData, setSubscriptionData] = useState<{ |
There was a problem hiding this comment.
All of this red is taken care of our new hooks, in a reusable way.
| const subscriptionSeatCount = subscriptionData?.subscriptionSeatCount | ||
| const isProUser = subscriptionData?.isProUser |
There was a problem hiding this comment.
These two variables in particular were unhelpful, so I inlined them.
|
|
||
| {subscriptionDataError || subscriptionSummaryDataError || membersDataError || invitesDataError ? ( | ||
| <div className={classNames('mb-4', styles.alert, styles.errorAlert)}> | ||
| <H3>Failed to load team data.</H3> |
There was a problem hiding this comment.
Unified these error messages based on @chrsmith's suggestion earlier. We could concat them or remove it altogether—I didn't want to invest more time in this at this point.
| export interface TeamMember { | ||
| accountId: string | ||
| displayName: string | null | ||
| email: string | ||
| avatarUrl: string | null | ||
| role: 'admin' | 'member' | ||
| } |
There was a problem hiding this comment.
These types were not right here. Now we have the hooks as type-specific logic, so moved these types from here and reserved this file for the React component only.
| // React hook to fetch data through the SSC proxy and convert the response to a more usable format. | ||
| // This is a low-level hook that is meant to be used by other hooks that need to fetch data from the SSC API. | ||
| export const useSSCData = <T extends object, U extends object | null>( | ||
| endpoint: string, | ||
| transformResponse: (response: T) => U | ||
| ): [U | null, Error | null] => { | ||
| const [data, setData] = useState<U | null>(null) | ||
| const [error, setError] = useState<Error | null>(null) | ||
| useEffect(() => { | ||
| async function loadData(): Promise<void> { | ||
| try { | ||
| const response = await callSSCProxy(endpoint, 'GET') | ||
| const responseJson = await response.json() | ||
| setData(transformResponse(responseJson)) | ||
| } catch (error) { | ||
| setError(error) | ||
| } | ||
| } | ||
|
|
||
| void loadData() | ||
| }, [endpoint, transformResponse]) | ||
|
|
||
| return [data, error] | ||
| } |
There was a problem hiding this comment.
This turned out to be quite complex. One thing I'm not satisfied with is that the useCallback() wrapping is in the higher-level hooks that use this one.
It'd be a lot nicer and less error-prone to include that wrapping here in a single location rather than in every higher-level hook, but I didn't manage to do it.
If you have a solution, @taras-yemets, please throw it in.
| export const useCodyTeamInvites = (): [TeamInvite[] | null, Error | null] => | ||
| useSSCQuery<InviteResponse, TeamInvite[]>('/team/current/invites', transformResponse) |
There was a problem hiding this comment.
useCodyTeamInvites and other wrappers around useSSCQuery are used once. Their main functionality is hiding the request URL and the map response function. Maybe we should just use useSSCQuery in components, WDYT?
There was a problem hiding this comment.
We could, I just thought it's neat to make this easy to reuse elsewhere. If you don't mind, I'll leave it as-is for now and inline it if we truly don't use it anywhere else.
0xnmn
left a comment
There was a problem hiding this comment.
Left a couple of comments, overall looks good.
chrsmith
left a comment
There was a problem hiding this comment.
The code in this PR is a step in the right direction, so let's get this checked-in!
I don't think this is the right design for how we surface the data types from our REST API, and that we probably need a more "rigid" approach to that. (But I'll take care of that in a separate PR, as this is strictly an improvement and I don't want to bother you with some other design that can easily land later.)
We'll need to reuse at least some of the data fetching I've created (for example, we'll need the subscription summary on pretty much all the pages), so I extracted that logic into easy-to-use hooks.
Also did some renaming and cleaning to make it easier to reuse and test later.
Test plan
Manually tested that the Team management page and the new Subscription page still work.