-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: no billing api calls in selfhosted #37834
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
base: master
Are you sure you want to change the base?
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.
2 files reviewed, no comments
Size Change: +95 B (0%) Total Size: 2.73 MB ℹ️ View Unchanged
|
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.
Should we check against isCloud
rather than isCloudOrDev
? I wouldn't want to obscure that this won't trigger locally in dev.
(Though I might have wrong assumptions about the definitions of cloud/dev in this context)
@@ -97,7 +105,7 @@ export const billingSpendLogic = kea<billingSpendLogicType>([ | |||
null as BillingSpendResponse | null, | |||
{ | |||
loadBillingSpend: async () => { | |||
if (!canAccessBilling(values.currentOrganization)) { | |||
if (!canAccessBilling(values.currentOrganization) || !values.isCloudOrDev) { |
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.
Should we check against isCloud rather than isCloudOrDev? I wouldn't want to obscure that this won't trigger locally in dev.
(Though I might have wrong assumptions about the definitions of cloud/dev in this context)
i think this means in posthog cloud, or in local development on an engineer machine, and otherwise not
i guess then maybe i should invert this and be more clear so there's a !isSelfHosted
so we don't have to keep adding conditions like isCI
🤔
@@ -3578,8 +3578,10 @@ export interface PreflightStatus { | |||
client_id?: string | |||
} | |||
} | |||
/** Whether PostHog is running in DEBUG mode. */ | |||
/** Whether PostHog is running in settings.DEBUG or settings.E2E_TESTING. */ |
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.
slightly weird that debug includes E2E_TESTING, but let's maintain that since it already exists
on self hosted installs as I click around I may trigger some code that hits the various billing logics. they check if i'm an admin or higher and i am, so they try and call billing APIs, and then show an error toast.
but there's nothing I can do about it
so, let's just not?