-
Notifications
You must be signed in to change notification settings - Fork 2
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 module controller and service #4072
base: main
Are you sure you want to change the base?
Changes from 10 commits
71fb982
530f60e
50f0c18
6c34c73
28b3de6
07a969c
521689b
b097861
30cfbb4
6f251b1
398d9b8
08d38f2
632834e
15ad49c
8f12f20
85d73f4
445a576
f4dea96
5d8818c
55d089b
c9e9740
6d8cd9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { createCollectionIfDoesNotExist } from "./service.js"; | ||
import type { NewCollectionRequestHandler } from "./types.js"; | ||
|
||
export const MetabaseCollectionsController: NewCollectionRequestHandler = | ||
async (_req, res) => { | ||
try { | ||
const params = res.locals.parsedReq.body; | ||
const collection = await createCollectionIfDoesNotExist(params); | ||
res.status(201).json({ data: collection }); | ||
} catch (error) { | ||
res.status(400).json({ | ||
error: | ||
error instanceof Error | ||
? error.message | ||
: "An unexpected error occurred", | ||
}); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { createMetabaseClient } from "../shared/client.js"; | ||
import type { NewCollectionParams } from "./types.js"; | ||
|
||
const client = createMetabaseClient(); | ||
|
||
export async function createCollection( | ||
params: NewCollectionParams, | ||
): Promise<any> { | ||
const transformedParams = { | ||
name: params.name, | ||
parent_id: params.parentId, | ||
}; | ||
|
||
const response = await client.post(`/api/collection/`, transformedParams); | ||
|
||
console.log( | ||
zz-hh-aa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
`New collection: ${response.data.name}, new collection ID: ${response.data.id}`, | ||
); | ||
return response.data.id; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { createMetabaseClient } from "../shared/client.js"; | ||
import type { GetCollectionResponse } from "./types.js"; | ||
|
||
const client = createMetabaseClient(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than instantiating multiple clients, we could follow a singleton pattern and only have one. This pattern is used for the planx-core clients (e.g. |
||
/** | ||
* Retrieves info on a collection from Metabase, use to check a parent. Currently only used in tests but could be useful for other Metabase functionality | ||
* @param id | ||
* @returns | ||
*/ | ||
export async function getCollection( | ||
id: number, | ||
): Promise<GetCollectionResponse> { | ||
const response = await client.get(`/api/collection/${id}`); | ||
return response.data; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,33 @@ | ||||||
import { updateMetabaseId } from "./updateMetabaseId.js"; | ||||||
import type { NewCollectionParams } from "./types.js"; | ||||||
import { getTeamIdAndMetabaseId } from "./getTeamIdAndMetabaseId.js"; | ||||||
import { createCollection } from "./createCollection.js"; | ||||||
|
||||||
/** | ||||||
* First uses name to get teams.id and .metabase_id, if present. | ||||||
* If metabase_id is null, return an object that includes its id. If not, create the collection. | ||||||
* @params `name` is required, but `description` and `parent_id` are optional. | ||||||
* @returns `response.data`, so use dot notation to access `id` or `parent_id`. | ||||||
*/ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd reconsider this comment - most of this is repeating type information which is redundant. Comments should ideally explain the why of something, and then we can allow the code and type system to explain the how. I actually think that if the function has a meaningful name we can drop this entire comment. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, I think it was inexperience that led me to writing some 'what' comments for myself :) In this case I think there was some slightly better reasoning too: that it was hard to come up with a name that also communicated the |
||||||
export async function createCollectionIfDoesNotExist( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think just Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that works! With not creating Metabase clutter in mind, I don't know that we need to run this every time a team is created, especially if PlanX will over time take on more non-planning uses? (eg we don't need collections for demo, sandbox and other agency--eg EA--spaces). But I think that's a question that's outside of the scope of this PR. (One other thing to note is that we are currently hard-coding a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point - I think that the non-council, non-Metabase teams, would the exceptions generally. We can always add a guard for these or remove collections at a later date if this is an issue. Agree it's outside the scope of this PR and something to circle back to later. |
||||||
params: NewCollectionParams, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We could use destructing here to save a big of complexity and mapping below. |
||||||
): Promise<number> { | ||||||
try { | ||||||
const { metabaseId, id: teamId } = await getTeamIdAndMetabaseId( | ||||||
params.name, | ||||||
); | ||||||
|
||||||
if (metabaseId) { | ||||||
return metabaseId; | ||||||
} | ||||||
|
||||||
// Create new Metabase collection if !metabaseId | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Redundant |
||||||
const newMetabaseId = await createCollection(params); | ||||||
await updateMetabaseId(teamId, newMetabaseId); | ||||||
console.log({ newMetabaseId }); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||||||
return newMetabaseId; | ||||||
} catch (error) { | ||||||
console.error("Error in createCollectionIfDoesNotExist:", error); | ||||||
throw error; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import { z } from "zod"; | ||
import type { ValidatedRequestHandler } from "../../../../shared/middleware/validate.js"; | ||
|
||
type ApiResponse<T> = { | ||
data?: T; | ||
error?: string; | ||
}; | ||
|
||
/** Interface for incoming request, in camelCase */ | ||
export interface NewCollectionParams { | ||
name: string; | ||
description?: string; | ||
/** Optional; if the collection is a child of a parent, specify parent ID here */ | ||
parentId?: number; | ||
} | ||
|
||
/** Interface for request after transforming to snake case (Metabase takes snake while PlanX API takes camel) */ | ||
export interface MetabaseCollectionParams { | ||
name: string; | ||
description?: string; | ||
parent_id?: number; | ||
} | ||
|
||
/** Metbase collection ID for the the "Council" collection **/ | ||
// const COUNCILS_COLLECTION_ID = 58; | ||
Comment on lines
+22
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we hitting issues here with 58 just being the ID on production and not locally and on staging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that's why it's commented out. Is there a better way to handle this in the meantime? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there's three main approaches -
We might end up on the third option, but let's work our way there via 1 and 2 first! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the meantime no issues keeping this in place and commented out. |
||
|
||
export const createCollectionIfDoesNotExistSchema = z.object({ | ||
body: z | ||
.object({ | ||
name: z.string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
description: z.string().optional(), | ||
parentId: z.number().optional(), //.default(COUNCILS_COLLECTION_ID), | ||
}) | ||
.transform((data) => ({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably on me sorry for not being clear on the whole camelCase / snake_case question! We are no correctly accepting all requests in camelCase, but our API should also only handle camelCase variables. We do need to transform it for Metabase, but we can just do that when we pass it along - it looks like this is already happening in camelCase in from consumer → API → Some logic → Pass to metabase in snake_case → Parse response back to camelCase → Some logic → camelCase back to consumer Happy to talk this one through if not clear 🙂 |
||
name: data.name, | ||
description: data.description, | ||
parent_id: data.parentId, | ||
})), | ||
}); | ||
|
||
export type NewCollectionRequestHandler = ValidatedRequestHandler< | ||
typeof createCollectionIfDoesNotExistSchema, | ||
ApiResponse<number> | ||
>; | ||
|
||
export interface NewCollectionResponse { | ||
id: number; | ||
name: string; | ||
} | ||
|
||
export interface GetCollectionResponse { | ||
id: number; | ||
name: string; | ||
parent_id: number; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,6 +5,8 @@ import { | |||||
logUserExitController, | ||||||
logUserResumeController, | ||||||
} from "./analyticsLog/controller.js"; | ||||||
import { MetabaseCollectionsController } from "./metabase/collection/controller.js"; | ||||||
import { createCollectionIfDoesNotExistSchema } from "./metabase/collection/types.js"; | ||||||
|
||||||
const router = Router(); | ||||||
|
||||||
|
@@ -18,5 +20,10 @@ router.post( | |||||
validate(logAnalyticsSchema), | ||||||
logUserResumeController, | ||||||
); | ||||||
router.post( | ||||||
"/collection", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API docs ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also contextualise this a little more - currently the route is just It would be useful I think to prefix all of the Metabase specific paths, e.g. I actually think that Later routes could be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I have sorted this (f4dea96), though I went with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! Order here is totally fine 👍 |
||||||
validate(createCollectionIfDoesNotExistSchema), | ||||||
MetabaseCollectionsController, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Functions should have camelCase names, only types, interfaces and classes should be in PascalCase. I'll see if I can find an ESLint rule for this. |
||||||
); | ||||||
|
||||||
export default router; |
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.
nit: We want to avoid using
any
wherever we can