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: Metabase dashboard service and controller (re-pushed) #4157

Merged
merged 24 commits into from
Feb 6, 2025

Conversation

zz-hh-aa
Copy link
Collaborator

Re-pushed version of #4150 hopefully sidestepping the Vultr phishing issue (Slack thread here).

What does this PR do?
Creates services getDashboard, copyDashboard, updateFilter and generatePublicLink
Adds necessary types and tests
Creates new route for analytics module
Exports the Metabase client as a singleton instance (this also happens in 8f12f20 in #4072 but there were other changes in the same commit so I've isolated the change here)
Updates Swagger yaml
Why?
Following on from #4072, this should complete all the services we need to automate Metabase dashboard creation
This PR became a bit bigger than I'd anticipated... let me know if it'd be better to break it up!

@zz-hh-aa zz-hh-aa requested a review from a team January 16, 2025 13:12
Copy link

github-actions bot commented Jan 16, 2025

Removed vultr server and associated DNS entries

Copy link
Contributor

@jamdelion jamdelion left a comment

Choose a reason for hiding this comment

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

Shaping up nicely! A few pointers for refactoring and type strengthening 💪

@zz-hh-aa zz-hh-aa requested a review from jamdelion January 21, 2025 10:37
Copy link
Contributor

@jamdelion jamdelion left a comment

Choose a reason for hiding this comment

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

Nice one! Extremely minor nits but happy for you to change them and merge whenever suits :)

Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Shaping up great! A few comments around types, error handling and snake_case.

Tests look great and descriptive, this is really very close just a few small things to iron out.

Very happy to catch up and talk through feedback if any of it is unclear - please just let me know.

Comment on lines 80 to 77
export interface GetDashboardResponse {
name: string;
id: number;
collection_id: number;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should find a good way of handling this issue of Metabase using snake_case but our API using camelCase.

All requests and responses to Metabase must be in snake_case, but any internal types passed around our API (as well as requests and response to our API) should be in camelCase.

There's a number of ways of handling this, one could be a Metabase types module so that we can type them as Metabase.GetDashboardResponse (namespaces are sort of deprecated / not recommended in TS).

We can assign these types to our client (axios already supports these generics) -

const response = await $metabase.get<Metabase.GetDashboardResponse>(`/api/dashboard/${dashboardId}`);

Then, if we're returning from out internal functions, we should use camelCase types (map across).

I appreciate this is a bit tedious/repetative but it's worth doing and avoid a lot of confusion down the time. Happy to talk this one through if you'd like 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'm a little confused on this one and could do with talking it through!

I'm wondering if a.) since this PR is convoluted enough as it is (especially since I just rebased and added a bunch of other commits 😬) and b.) since this matter involves the collection/ folder as well, if it would make sense to talk about this next week and figure out a solution that will work across the Metabase module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - on board with this, happy to park for now and we can catch up this week about it. Please send across a calendar invite for a time that works for you 👍

api.planx.uk/modules/analytics/metabase/dashboard/types.ts Outdated Show resolved Hide resolved
Base automatically changed from oz/analytics-metabase-collection to main January 28, 2025 13:29
@zz-hh-aa zz-hh-aa force-pushed the oz/metabase-dash-controller-service branch 2 times, most recently from 1bbda5b to 2d6993a Compare January 30, 2025 14:15
@zz-hh-aa zz-hh-aa force-pushed the oz/metabase-dash-controller-service branch from f94f08a to da42196 Compare February 4, 2025 10:53
@zz-hh-aa zz-hh-aa merged commit a369e1d into main Feb 6, 2025
13 checks passed
@zz-hh-aa zz-hh-aa deleted the oz/metabase-dash-controller-service branch February 6, 2025 10:38
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