Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSC: Teams and Invites: Create "Manage team" page #62453

Merged
merged 21 commits into from May 15, 2024

Conversation

vdavid
Copy link
Contributor

@vdavid vdavid commented May 6, 2024

Contributes to https://github.com/sourcegraph/self-serve-cody/issues/725

4-min Loom for @rrhyne (and others) here.

  • Added "Manage Cody team" page at /cody/team/manage for dotcom only, behind the useEmbeddedCodyProUi flag.
  • The page redirects non-Pro users to /cody/subscription
  • Added fetchThroughSSCProxy function (but not using it yet on the checkout page—left a TODO to do that later)
  • Wired up team member, invite, and subscription data (but not the team data yet because the API is not ready)
  • Extracted TeamMembers and InviteUsers into separate components.

Notes:

Test plan

This is WIP and behind a feature flag so low stakes for now. I've clicked around but my main goal was not to make it steadily functional but to check off as many of the items in https://github.com/sourcegraph/self-serve-cody/issues/725 as possible and build on this in future commits.

@cla-bot cla-bot bot added the cla-signed label May 6, 2024
Copy link
Contributor

@chrsmith chrsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it is on the right track. And the screen shots you posted on Slack look great!

Most of my feedback can be summed up with:

"This is totally fine for a draft PR. Here is a suggestion for how to clean things up when this is ready for a proper review. (But is more than likely something you were probably already planning on doing anyways, since this is just a WIP Draft.)"

client/web/src/cody/team/CodyManageTeamPage.tsx Outdated Show resolved Hide resolved
client/web/src/cody/team/CodyManageTeamPage.tsx Outdated Show resolved Hide resolved
client/web/src/cody/team/CodyManageTeamPage.tsx Outdated Show resolved Hide resolved
client/web/src/routes.constants.ts Outdated Show resolved Hide resolved
client/web/src/routes.tsx Outdated Show resolved Hide resolved
client/web/src/cody/team/CodyManageTeamPage.tsx Outdated Show resolved Hide resolved
@vdavid vdavid force-pushed the dv/add-first-piece-of-teams-and-invites branch from 129e288 to 24e3b9d Compare May 10, 2024 12:11
try {
const response = await fetchThroughSSCProxy('/subscription', 'GET')
const responseJson = (await response.json()) as { endedAt: string | null; maxSeats: number } | null
setIsProUser(responseJson && !!responseJson.endedAt)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrsmith, this seemed like the best way to determine whether the user is Pro or not based on this type. Is this fine or can you recommend a better solution?

const remainingInviteCount = useMemo(() => {
const memberCount = teamMembers?.length ?? 0
const invitesUsed = (teamInvites ?? []).filter(invite => invite.status === 'sent').length
return Math.max(subscriptionSeatCount || 0 - (memberCount + invitesUsed), 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrsmith let me know if you have a better suggestion to calculate this based on the available data.

@vdavid vdavid marked this pull request as ready for review May 10, 2024 13:29
@vdavid vdavid marked this pull request as draft May 10, 2024 13:41
@vdavid vdavid force-pushed the dv/add-first-piece-of-teams-and-invites branch from 2b71b3e to 3502d76 Compare May 10, 2024 14:15
@vdavid vdavid marked this pull request as ready for review May 10, 2024 14:20
@vdavid vdavid changed the title SSC: Teams and Invites v1 SSC: Teams and Invites: Create "Manage team" page May 10, 2024
@vdavid vdavid force-pushed the dv/add-first-piece-of-teams-and-invites branch from 3502d76 to 1f5358d Compare May 10, 2024 14:38
@vdavid vdavid requested a review from rrhyne May 10, 2024 14:46
Copy link
Contributor

@chrsmith chrsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of good stuff in this PR, and it's super-exciting to see the Teams and Invites functionality come together! But I think this PR still needs a bit more work before it is checked-in.

As far as a next step, let's try to see if we can find a time to review this PR in a VC meeting so we can discuss tradeoffs more quickly. (I can come in a couple hours earlier any day of the week, just let me know with some notice.)

Overall, each of my PR comments probably boils down to one of the following:

  1. A starting point for discussing how we want to build Cody Pro. e.g. not so much a concrete suggestion, but more double checking how we want to approach certain aspects of the design or UX.

  2. Engineering rigor types of things that I (annoyingly) won't budge on. We are going to create a Cody Pro codebase that we will be proud of, and be constructed in such a way that anybody on the team will be able to easily and safely make changes to. This will require adding some layers of abstraction or formalizing things to a higher degree than immediately seems required. But alas, "sorry not sorry".

  3. Engineering rigor types of things that I (perhaps reluctantly) will budge on. While I have suggestions for how to make the code easier to read or maintain, you don't need to accept every single one. (Nor should you?) Maybe I'm just factually wrong. Or it's a matter of opinion, and you just prefer it some other way. Or we both acknowledge that something is wrong, but it just doesn't make sense to deal with it at this time.

Unfortunately I cannot say for certain which things fall into camp 2 vs. camp 3. (In the extremes, some things are clearly "wrong", IMO and other things I could easily be convinced are A-OK.)

But it'll probably take some time for us to norm on where exactly we want to draw the line here. e.g. coming up with a standard for code quality that we want to shoot for on the Cody Prime team. Or the degree to which we rely on sophisticated data types or design patterns in order to make it easier to write correct code.

Anyways, if you could try to address the PR feedback that you agree we can figure out the best way to address some of these other issues next week. (e.g. I'll try to send out a PR with a TypeScript "API client library" for the SSC backend API, which will address some of the things I call out.)

client/web/src/cody/team/CodyManageTeamPage.tsx Outdated Show resolved Hide resolved
client/web/src/cody/util.ts Show resolved Hide resolved
client/web/src/routes.tsx Outdated Show resolved Hide resolved
client/web/src/cody/team/TeamMembers.tsx Show resolved Hide resolved
client/web/src/cody/team/TeamMembers.tsx Show resolved Hide resolved
client/web/src/cody/team/TeamMembers.tsx Show resolved Hide resolved
client/web/src/cody/team/TeamMembers.tsx Show resolved Hide resolved
@vdavid
Copy link
Contributor Author

vdavid commented May 13, 2024

As far as a next step, let's try to see if we can find a time to review this PR in a VC meeting so we can discuss tradeoffs more quickly. (I can come in a couple hours earlier any day of the week, just let me know with some notice.)

The earliest we can do that is tomorrow (Tuesday). Let's do that tomorrow, then. Could you schedule something within my work hours that fits your schedule?

However, as I've addressed each piece of feedback, I suggest you re-review this PR to unblock, and we can address any further concerns in follow-ups.

@vdavid vdavid force-pushed the dv/add-first-piece-of-teams-and-invites branch from 30c6e19 to 8aa4a85 Compare May 14, 2024 13:46
Copy link
Contributor

@chrsmith chrsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to not hold up the Teams and Invites stuff any more.

Though as we continue to flesh out this part of the codebase, let's try to pin down the specific patterns we do and do not want to follow in order to keep things simple and maintainable. (e.g. using the withAuthenticatedUser() wrapper, and similar things.)

@vdavid vdavid enabled auto-merge (squash) May 15, 2024 07:49
@vdavid vdavid disabled auto-merge May 15, 2024 07:49
@vdavid vdavid enabled auto-merge (squash) May 15, 2024 07:50
@vdavid vdavid merged commit 99ebb4f into main May 15, 2024
9 checks passed
@vdavid vdavid deleted the dv/add-first-piece-of-teams-and-invites branch May 15, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants