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: first PR for Usage dashboard #26807

Merged
merged 8 commits into from
Jan 3, 2025

Conversation

patricio-posthog
Copy link
Contributor

Problem

We are planning to add an Usage dashboard, where organizations will be able to see some information about their events consumptions and general usage in the current period. It will be part of Billing.

This first PR introduces the feature flag that will be used in this project (billing-usage-dashboard) and minor cosmetics changes, so we can get ahead on this.

Changes

  • New feature flag billing-usage-dashboard
  • Change text in bottom navigation from Billing to Biling & Usage
  • New organizationBillingUsage scene that has two links in the left navigation, and will link to each section (similar to Settings)

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

It doens't have an impact.

How did you test this code?

Test it locally, checking that the changes do not affect anything if the feature flag is not present.

Copy link
Contributor

github-actions bot commented Dec 10, 2024

Size Change: +145 B (+0.01%)

Total Size: 1.11 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.11 MB +145 B (+0.01%)

compressed-size-action

Copy link
Contributor

@zlwaterfield zlwaterfield left a comment

Choose a reason for hiding this comment

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

I love the speed here but I think we should think this through just a little more. I am thinking more of a structure like the current settings page where we do /[...path to settings]/billing and /[...path to settings]/usage as two different URLs so we can control access to the pages and support more pages of that structure in the future.

@patricio-posthog
Copy link
Contributor Author

I love the speed here but I think we should think this through just a little more. I am thinking more of a structure like the current settings page where we do /[...path to settings]/billing and /[...path to settings]/usage as two different URLs so we can control access to the pages and support more pages of that structure in the future.

I agree 100%. My idea was to have a root path and then two internal sections, as you said, with .../billing and .../usage. Right now I just created the root one, with billing_usage, although I agree that's a bad name (but I couldn't think of something better).

But in the future it would be {new_root_for_billing_and_usage}/billing and {new_root_for_billing_and_usage}/usage. I can change it in this PR once we decide on that :)

And thanks for the quick review!

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@patricio-posthog
Copy link
Contributor Author

@zlwaterfield as discussed the other day, fort he initial PR for the new billing & usage dashboards, the URLs will be
billing/overview and billing/usage

@posthog-bot posthog-bot removed the stale label Dec 27, 2024
frontend/src/scenes/sceneTypes.ts Outdated Show resolved Hide resolved
@@ -209,6 +209,8 @@ export const urls = {
// Cloud only
organizationBilling: (products?: ProductKey[]): string =>
`/organization/billing${products && products.length ? `?products=${products.join(',')}` : ''}`,
organizationBillingOverview: (): string => `/organization/billing/overview`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern is good for now but we may want to move to how settings is done (look above in file) where it's a function that passes in the section id for the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into a function that receives a BillingSectionId

@@ -0,0 +1,31 @@
.BillingSection {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're trying to move away from scss so it's best to use inline tailwind styling now. LLMs are good at converting this if you need help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks!

to={
featureFlags[FEATURE_FLAGS.BILLING_USAGE_DASHBOARD]
? urls.organizationBillingOverview()
: urls.organizationBilling()
Copy link
Contributor

Choose a reason for hiding this comment

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

There are quite a few instances of this call so I think it might be better to change in the function

E.g.

    organizationBilling: (products?: ProductKey[]): string =>
	featureFlags[FEATURE_FLAGS.BILLING_USAGE_DASHBOARD] 
		? `/organization/billing/overview${products && products.length ? `?products=${products.join(',')}` : ''}`,
	        : `/organization/billing${products && products.length ? `?products=${products.join(',')}` : ''}`,

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 only found one instance of this, so I don't know if it's a good idea to create a function for this now. Maybe in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see 12 instances and it will also make it so if anyone adds another this this doesn't get missed. Change one place instead of 12.
Screenshot 2024-12-27 at 10 25 08 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhh I haven't thought in change those yet. I was looking for instances where the if feature flag was already implemented. Ok, going to do this.

Copy link
Contributor

@zlwaterfield zlwaterfield left a comment

Choose a reason for hiding this comment

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

Looks good, we can address this other things on the next PR

@patricio-posthog patricio-posthog merged commit 1b9aa47 into master Jan 3, 2025
100 checks passed
@patricio-posthog patricio-posthog deleted the patricio/usage-dashboard-first branch January 3, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants