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

feat(plg): Add seats #63227

Merged
merged 27 commits into from
Jun 19, 2024
Merged

feat(plg): Add seats #63227

merged 27 commits into from
Jun 19, 2024

Conversation

vdavid
Copy link
Contributor

@vdavid vdavid commented Jun 12, 2024

To code reviewers: This PR consists of 15 commits already, but I'd say it still might be worth going commit by commit because I managed to use almost atomic commits throughout the development. There were unfortunately a few cases where I didn't use the right naming for some entities at first, and as a result, there is a bit of back-and-forth through the commits, but I think it's still relatively easy to follow.

The changes in this PR:

  • Added the "Add seats" state to the checkout page, with the traits specified by the ticket: https://github.com/sourcegraph/self-serve-cody/issues/791
    • Hooked up the new page with the green "Add seats" button
    • Made the credit card + billing address section non-editable, collapsible, and collapsed by default.
    • When changing the seat counts, we now call our API to re-calculate the monthly price, incl. pro-ration.
    • The "Subscribe" button now says "Confirm plan changes" when adding seats
    • Pressing the button actually adds new seats to the plan and charges the card.
  • Fixed the seat counter width here to fit its content without looking broken
  • Extended the summary to let the admin preview and understand the change before committing.
  • Hid the "pro-rated" disclaimer if not adding seats, and made it more specific when we do
  • Removed the "team" query param to simplify the interface of the checkout page.
  • Improved error handling and display for edge cases when some data is not available. The errors are still not beautiful (see this issue: https://github.com/sourcegraph/sourcegraph/issues/63235 where I suggest improving them) but still better than how they were.
  • Now ensuring that the initial seat count is within valid boundaries. Before this, if someone entered seats=2000, the form initialized with 2000. Now it does with 50, as the max seat count.
  • Extracted a few subcomponents (here) because the CodyProCheckoutForm component started being quite complex.

Test plan

Manual QA. I clicked through it a few times, and it worked well. Here is a 5-minute Loom where I demo the whole team creation and seat adjustment flows.

@cla-bot cla-bot bot added the cla-signed label Jun 12, 2024
@vdavid vdavid force-pushed the dv/ssc-add-seats branch 3 times, most recently from 27e25d1 to 66e0ab6 Compare June 12, 2024 20:55
@vdavid vdavid requested a review from a team June 12, 2024 21:20
@vdavid vdavid marked this pull request as ready for review June 12, 2024 21:20
Copy link
Contributor

Choose a reason for hiding this comment

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

Plan state management becomes complex. To simplify it, I think we should consider the following.

1. Move the plan value calculation and plan changes logic into a custom hook.

For example:

type CodyPlanHookArgs = any // whatever we need to set it up properly
type CodyPlanHook = (...args: CodyPlanHookArgs) => {
  // referencing the existing interface, not sure whether it correctly reflects its content
  plan: TeamSizeChange; 
  
  // 🤞🏻 maybe we can even encapsulate initial seat count login in a hook and won't need to expose it 
  initialSeatCount: number; 
  
  // some indicator of whether the returned plan is up to date, e.g.
  status: 'success' | 'pending' | 'error';
  
  previewSeatCountChange: (diff: number) => void;
  updateSubscriptionPlan: () => void;
}

The core returned value here is plan (or however we name it). It can be computed based either on local state (if addSeats if false) or on the server state (if we need to get data from the payment system before).
The hook may also return additional fields and methods. The main idea is to hide as many seat/price calculation details as possible. Currently, they are spread across quite a big component.

2. Keep state as minimal as possible.

We should avoid duplication and redundant state where possible. If we can calculate some information from the component’s props or its existing state variables during rendering, we should not put that information into that component’s state (from docs). E.g., monthlyPriceDiff, newMonthlyPrice, dueNow can be computed from the seats count and price per seat.

3. Preview current subscription update may be a query, not a mutation.

Unlike queries, mutations are typically used to create/update/delete data or perform server side-effects.

From docs.
previewUpdateCurrentSubscriptionMutation doesn't seem to change anything, it defers to backend to request a new price from the payment system.
I'm not 100% sure about it, as I haven't tried to implement it, but it looks to me more like a query that depends on addSeats and seatCountDiff variables.

**4. Use mutations "conventionally".

Having to look for workarounds like mutateAsync.call() hints that we may overcomplicate the way we use React Query mutations. Can we call mutation from an event handler and then derive the needed UI based on the mutation state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a very valid suggestion but want to move fast, so I've created an issue to track this improvement: https://github.com/sourcegraph/sourcegraph/issues/63349

- Load subscription data
- Remove "team" query param to simplify the interface
- Improve error handling and display
- Fix: Make sure the initial seat count is within valid boundaries
- Add price preview UI (without data preview)
Copy link
Contributor

@taras-yemets taras-yemets 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 unblock. Feel free to address any comments in this or subsequent PRs as you see fit.

@vdavid vdavid merged commit 611dcfa into main Jun 19, 2024
11 checks passed
@vdavid vdavid deleted the dv/ssc-add-seats branch June 19, 2024 17:04
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.

2 participants