-
Notifications
You must be signed in to change notification settings - Fork 117
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: Add self serve billing management UI #5431
Conversation
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.
Moving the showUpgrade
state from the query parameter to localStorage
seems overly complicated. Can we just keep the query parameter in the URL?
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.
And a nit for clarity: showUpgradeDialog
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.
This was a bad experience for me. If the url somehow got saved then refreshing or saving it would always show the modal. Added a comment to explain why we need this.
web-admin/src/routes/[organization]/-/settings/billing/upgrade/+page.ts
Outdated
Show resolved
Hide resolved
export const load: PageLoad = async ({ params: { organization } }) => { | ||
let issues: V1BillingIssue[] = []; | ||
try { | ||
issues = await fetchOrganizationBillingIssues(organization); |
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'm wondering if this should happen at the [organization]
route level (semantically that would make sense), and then this callback should just invalidate the parent load function? But maybe not, because that might lead to excessive invalidation?
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.
Lets keep it simple for now.
web-admin/src/routes/[organization]/-/upgrade-callback/+page.svelte
Outdated
Show resolved
Hide resolved
web-admin/src/routes/[organization]/-/settings/billing/+page.svelte
Outdated
Show resolved
Hide resolved
web-admin/src/features/billing/issues/getMessageForPaymentIssues.ts
Outdated
Show resolved
Hide resolved
web-admin/src/features/billing/issues/useBillingIssueMessage.ts
Outdated
Show resolved
Hide resolved
export function getSubscriptionForOrg<T = V1GetBillingSubscriptionResponse>( | ||
organization: string, | ||
queryOptions?: CreateQueryOptions< | ||
V1GetBillingSubscriptionResponse, | ||
ErrorType<RpcStatus>, | ||
T // T is the return type of the `select` function | ||
>, | ||
): CreateQueryResult<T, ErrorType<RpcStatus>> { | ||
return derived( | ||
[createAdminServiceGetOrganization(organization)], | ||
([orgResp], set) => | ||
createAdminServiceGetBillingSubscription(organization, { | ||
query: { | ||
...queryOptions, | ||
enabled: | ||
(queryOptions && "enabled" in queryOptions | ||
? queryOptions.enabled | ||
: true) && | ||
!!orgResp.data?.permissions?.manageOrg && | ||
!!organization, | ||
queryClient, | ||
}, | ||
}).subscribe(set), | ||
); | ||
} |
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.
At first glance, this wrapper seemed a bit heavy. Another approach: see web-admin/src/routes/+layout.svelte
for how specific projectPermissions
are passed through to a child component. Perhaps if we follow that pattern, we can avoid a sprawl of GetOrganization
calls.
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.
Good point. This is not needed at all since I added the BillingBannerManagerForAdmins
and OrganizationHibernatingForAdmins
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 could you point to other places where GetOrg call is not needed? I have added a comment in Payment.svelte
and removed it from useBillingIssueMessage
.
web-admin/src/routes/[organization]/-/settings/billing/payment/+page.ts
Outdated
Show resolved
Hide resolved
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 calls to redirectToLoginIfNotLoggedIn
break Public URLs. We have two options:
- For now, remove them in favor of the call in the global error callback in
error-utils.ts
where we handle the Public URL case. - Handle the Public URL case here. With a refactor, we might actually be able to migrate away from the global error callback & instead rely on throwing errors in the relevant
+layout.ts
files, like you've done here.
1 might be safer & easier, but 2 is probably better long-term.
web-admin/src/routes/+layout.ts
Outdated
if ( | ||
withinProject({ url, route } as Page) && | ||
!isProjectRequestAccessPage({ url, route } as Page) | ||
) { | ||
// if not in request access page (approve or deny routes) then go to a page to get access | ||
throw redirect( | ||
307, | ||
`/-/request-project-access/?organization=${organization}&project=${project}`, | ||
); | ||
} |
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.
This doesn't look like it's supposed to be here
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.
Since this is at the highest level +layout.ts
errorUtils
is not initialised just yet, so it is never hit to run that logic. Lets revisit that, perhaps we could move its initialisation here, but lets not add too many possible regressions in this PR.
* use svelte and vite-plugin-singlefile * cleanup * revert * more cleanup * regen package-lock
* wip * project share initial work * apply avatar circle list to project share * square * user invite org and group * spacing * scrollable users * user invite user set role * user invite group set role * copy * ability to remove user * do not allow current user to remove themselves * ability to remove user group * use invite group tooltip * prop rename * one more * rename * wip tooltip * use popover * add tooltip to org users * polish user email addition exp * copy * comments * lint * no prop drilling * lint * lint * clean up some org groups * feedback * feedback complete * comment out org groups * clean up avatar list item * lint * move avatar list item to avatar dir * defensive * lint * move avatar list item to features users * everyone from text * cursor tweaks * disable role change for current user * hide all users from groups * apply selected to user invite button * comment out manage org members, org set role changes * rebase fix * apply user photo url to popover * default null photo url * apply other photo urls in avatar * spacing set role margin right * support everyone at all-users * use all-users members * lint * wip * lint * multiple access tooltip * remove unused * Revert "multiple access tooltip" This reverts commit 0f62db8. * popover rename * clean up * remove unused * user-management rename * feedback * feedback * lint * defensive * use found all users group
* disable preview button when explore resource is reconciling * update tooltip
* display null values with italics * remove log
* trigger org repair manually * review comments * fix billing cust id, raise issues when updating stripe customer manually * disallow renewal to non public plan
web-admin/src/routes/[organization]/-/settings/billing/payment/+page.ts
Outdated
Show resolved
Hide resolved
hasBlockerIssues(issues) && | ||
(await fetchAllProjectsHibernating(organization)) | ||
) { | ||
shouldRedirectToProjectsList = true; |
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 doesn't look like we need to keep this shouldRedirectToProjectsList
state & we can just fire the redirect right here
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.
We need this because of the try catch. throw redirect...
will end up getting caught there.
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. That's a little confusing, so we could only do the data fetching within the try
, and move the conditional to the bottom?
`${url.protocol}//${url.host}/${organization}`, | ||
); | ||
} catch (e) { | ||
if (e.response?.status !== 403) { |
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.
We should throw the error when it's a 403 too
hasBlockerIssues(issues) && | ||
(await fetchAllProjectsHibernating(organization)) | ||
) { | ||
shouldRedirectToProjectsList = true; |
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. That's a little confusing, so we could only do the data fetching within the try
, and move the conditional to the bottom?
5f6de87
to
0dc2a39
Compare
admin/server/billing.go
Outdated
@@ -233,6 +237,17 @@ func (s *Server) CancelBillingSubscription(ctx context.Context, req *adminv1.Can | |||
return nil, status.Error(codes.Internal, err.Error()) | |||
} | |||
|
|||
// err = s.admin.Email.SendSubscriptionCancelled(&email.SubscriptionCancelled{ |
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 you uncomment this, just remove the `Plan Name' from template no harm in that, once Eric O is back we can discuss.
Adds a self serve billing page under organisation settings. Some local setup is needed to get this working.
Contact me on slack for stripe and orb keys to be added to
.env
. Once.env
is updated runrill devtool start cloud --refresh-dotenv=false
to start cloud.Trial subscription
To test trail subscription there is a helper command to advance time artifically. (might get merged into
devtool
subcommand inrill
) : https://github.com/rilldata/rill-helpers?tab=readme-ov-file#trail-subscription--org
is optional and will default to the org selected usingrill org switch
./rill-helpers sub advance-time --org <org_name> --time <today+2>
to get theEnding soon
email. This is mandatory to keep things consistant../rill-helpers sub advance-time --org <org_name>
next to end the trial../rill-helpers sub advance-time --org <org_name>
next to end the grace period as well.Team plan
To test team plans webhooks forwarder is needed for stripe and orb.
ngrok http --url=<domain name> 8080
https://<domain>/payment/webhook
, add webhook to stripe: https://dashboard.stripe.com/test/webhooks.(select all events while creating this)RILL_ADMIN_STRIPE_WEBHOOK_SECRET
. Reveal signing secret on the webhook page to get this.https://<domain>/billing/webhook
as url, add webhook to orb: https://app.withorb.com/webhooks (Make sure to be on test plan).RILL_ADMIN_ORB_WEBHOOK_SECRET
. Get the signing secret from webhook page.rill devtool start cloud --refresh-dotenv=false
to avoid overwriting the keys.To test a cancelled team plan expiring (needs some familiarity with orb),
./rill-helpers sub advance-time --org <org_name>
to reflect things in rill.To test a failed payment,
./rill-helpers sub advance-time --org <org_name>