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

Implement a new Team Billing where Owners can conveniently manage a paid plan for their Team #8041

Merged
merged 4 commits into from
May 16, 2022

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented Feb 4, 2022

Description

Allow teams to easily purchase and manage a Team Plan directly from their team's Settings.

TODO:

  • Implement a new Team Plan shape that's directly linked to / owned by a team: [db][protocol] Implement TeamSubscription2 DB shapes and migration #9655
  • Allow subscribing via Chargebee directly from your team Settings
  • Create a "synthetic" user subscription when you're a member of a paid Team Plan
  • Transparently update the Team Plan quantity in Chargebee when users join or leave a team
  • Pro-rate charge for new members joining in the middle of a billing cycle
  • Allow managing your Billing in team Settings via a "Team Chargebee Portal"
  • Align the new UI with [Design] Team Plans v2 #8352

Related Issue(s)

Fixes #7759

How to test

  1. Create a Team
  2. Invite other team members
  3. Go to Team Settings > Plans
  4. Buy a Team Plan, verify billing amount matches member count, verify all members are indeed upgraded
  5. Add/remove members, verify that the Team Plan billing amount is updated accordingly
  6. Verify that cancelling, re-upgrading, deleting the team all work as expected

Release Notes

Implement a new Team Billing where Team Owners can conveniently manage a paid plan for their Team

Documentation

  • /werft without-vm=true
  • /werft with-helm=true
  • /werft with-payment=true
  • /werft with-clean-slate-deployment

@jankeromnes

This comment was marked as resolved.

@codecov

This comment was marked as outdated.

@roboquat

This comment was marked as resolved.

@roboquat roboquat added size/XL and removed size/L labels Feb 4, 2022
@jankeromnes

This comment was marked as resolved.

@jankeromnes jankeromnes force-pushed the jx/better-team-subscriptions branch 4 times, most recently from 781bacd to 6f646d9 Compare February 7, 2022 16:28
@mads-hartmann

This comment was marked as resolved.

@jankeromnes

This comment was marked as resolved.

@jankeromnes jankeromnes force-pushed the jx/better-team-subscriptions branch 8 times, most recently from 617bd55 to ffa5298 Compare February 14, 2022 15:45
@roboquat roboquat added size/XXL and removed size/XL labels Feb 15, 2022
@jankeromnes jankeromnes removed their assignment May 6, 2022
@jldec
Copy link
Contributor

jldec commented May 6, 2022

/hold because of rebase dependent PR #9655

Copy link
Contributor

@jldec jldec left a comment

Choose a reason for hiding this comment

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

Great stuff - everything works (small pro-rata comment)
Hold before merging and complete caveats.

@jankeromnes jankeromnes force-pushed the jx/better-team-subscriptions branch from a76c3c4 to b3cc98b Compare May 6, 2022 17:08
@jldec
Copy link
Contributor

jldec commented May 9, 2022

@jankeromnes please hold this PR while we review the question of seat limits.

  • Old team-plans : Invite URLs are limited to the number of seats purchased and will fail beyond that limit.
  • New teams : Invite URLs can be shared by any member and do not (currently) have seat limits.

Some options to mitigate this: (open to other ideas)

  • Simple toggle on the billing page to allow / disallow additional members to join
  • Text (number) field on the billing page to limit member count
  • Limit invite URLs to single use, and only owners can create

@jankeromnes jankeromnes force-pushed the jx/better-team-subscriptions branch from b3cc98b to 5390aa8 Compare May 9, 2022 08:01
Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Left a few comments. Looks very good!

`chargeForUpgrade: paymentReference=${teamSubscription.paymentReference}, currentTermRemainingRatio=${currentTermRemainingRatio}, diffInCents=${diffInCents}`,
);
await this.upgradeHelper.chargeForUpgrade(
"",
Copy link
Member

Choose a reason for hiding this comment

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

why ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. This is supposed to be a userId, but it's only used in a logContext:

async chargeForUpgrade(userId: string, chargebeeSubscriptionId: string, amountInCents: number, description: string, upgradeTimestamp: string) {
const logContext: LogContext = { userId };

For team subscription changes, a userId doesn't really make sense (i.e. do we want the user who is joining or leaving the team? or a billing owner? which one if there are multiple? etc)

A "cleaner" solution would be to refactor or implement a new upgradeHelper.chargeForUpgrade, but I thought it wasn't worth it, since we'd like to deprecate all this code soon anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexTugarev Would you like this to be changed/improved somehow?


const coupon = await this.findAvailableCouponForPlan(user, planId);

const email = User.getPrimaryEmail(user);
Copy link
Member

Choose a reason for hiding this comment

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

😢
at least this is not used to id the customers.
just the regular note: User.getPrimaryEmail is not stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was copied over from the non-team checkout method (although I did skip the check/error).

Would you like this code to be changed/improved somehow?

@jldec
Copy link
Contributor

jldec commented May 9, 2022

@jankeromnes - regarding the PR hold to resolve the question of seat limits:
Let's please merge the PR after making a change to hide the "Billing" menu in Team settings (or using a feature flag).
We are leaning toward option 3 to limit who can generate member invite URLs to just team owners.
cc: @geropl

@jankeromnes jankeromnes force-pushed the jx/better-team-subscriptions branch 2 times, most recently from 94cd550 to c106f85 Compare May 10, 2022 08:17
@jankeromnes
Copy link
Contributor Author

jankeromnes commented May 10, 2022

Oops, looks like this doesn't work with the new VM/payment setup. I'm able to buy something from Chargebee, but the team plan never materializes, and the server pod is full of these errors:

QueryFailedError: ER_PARSE_ERROR: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE deleted = true' at line 1
    at QueryFailedError.TypeORMError [as constructor] (/app/node_modules/typeorm/error/TypeORMError.js:9:28)
    at new QueryFailedError (/app/node_modules/typeorm/error/QueryFailedError.js:13:28)
    at Query.<anonymous> (/app/node_modules/typeorm/driver/mysql/MysqlQueryRunner.js:222:57)
    at Query.<anonymous> (/app/node_modules/mysql/lib/Connection.js:526:10)

I wonder what's up. 🤔

@jankeromnes
Copy link
Contributor Author

jankeromnes commented May 10, 2022

/werft run

👍 started the job as gitpod-build-jx-better-team-subscriptions.83
(with .werft/ from main)

@jankeromnes jankeromnes force-pushed the jx/better-team-subscriptions branch from c106f85 to 17f5738 Compare May 10, 2022 12:27
@jankeromnes jankeromnes force-pushed the jx/better-team-subscriptions branch from 17f5738 to fd0ded8 Compare May 11, 2022 13:49
@jankeromnes
Copy link
Contributor Author

jankeromnes commented May 11, 2022

This error is still here 😭

QueryFailedError: ER_PARSE_ERROR: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE deleted = true' at line 1
    at QueryFailedError.TypeORMError [as constructor] (/app/node_modules/typeorm/error/TypeORMError.js:9:28)
    at new QueryFailedError (/app/node_modules/typeorm/error/QueryFailedError.js:13:28)
    at Query.<anonymous> (/app/node_modules/typeorm/driver/mysql/MysqlQueryRunner.js:222:57)
    at Query.<anonymous> (/app/node_modules/mysql/lib/Connection.js:526:10)
    at Query._callback (/app/node_modules/mysql/lib/Connection.js:488:16)
    at Query.Sequence.end (/app/node_modules/mysql/lib/protocol/sequences/Sequence.js:83:24)
    at Query.ErrorPacket (/app/node_modules/mysql/lib/protocol/sequences/Query.js:92:8)
    at Protocol._parsePacket (/app/node_modules/mysql/lib/protocol/Protocol.js:291:23)
    at Parser._parsePacket (/app/node_modules/mysql/lib/protocol/Parser.js:433:10)
    at Parser.write (/app/node_modules/mysql/lib/protocol/Parser.js:43:10)
    at Protocol.write (/app/node_modules/mysql/lib/protocol/Protocol.js:38:16)
    at Socket.<anonymous> (/app/node_modules/mysql/lib/Connection.js:88:28)
    at Socket.<anonymous> (/app/node_modules/mysql/lib/Connection.js:526:10)
    at Socket.emit (node:events:390:28)
    at Socket.emit (node:domain:475:12)
    at addChunk (node:internal/streams/readable:315:12)

EDIT: But it does look unrelated to this PR, maybe. I suspect it might be a MySQL/TypeOrm incompatibility issue, which might not affect staging. 🤞

I have battle-tested this PR again -- everything seems to work fine, with purchase/update/cancel in both legacy Team Plans and the (hidden) new Team Plans. ✅

However, in the spirit of safety, holding off on merging this now, given the mysterious SQL error + tomorrow's prod deployment + that I'm off until Monday. However, please do feel free to have a look, and merge if this looks safe to you (and revert it again if you notice the SQL error or other weirdness once deployed on staging).

@jankeromnes
Copy link
Contributor Author

jankeromnes commented May 16, 2022

Many thanks @AlexTugarev for spotting and fixing the SQL error above in #9957 ! 🎉 ❤️

This is now ready to go then. 🚀

/unhold

@roboquat roboquat merged commit 3479ced into main May 16, 2022
@roboquat roboquat deleted the jx/better-team-subscriptions branch May 16, 2022 07:55
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: webapp Meta team change is running in production deployed Change is completely running in production meta: never-stale This issue can never become stale release-note size/XXL team: webapp Issue belongs to the WebApp team
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Epic: Simplify team plans and align with project teams
5 participants