-
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
wip: feat: collection service & controller #3971
base: main
Are you sure you want to change the base?
Changes from 15 commits
9f469cb
164798f
dc22f0b
a957c1a
9739d6b
16a02b3
165eca6
a4d6b5f
62836ba
73ba1e9
22d3ee4
e19ce99
bc13342
97ee946
1a4ca85
6ac50e7
1d8dfb4
ae037aa
9d6878c
cea1b05
df3345e
628c151
3974b2c
1d885e0
62ca295
052b0f3
bb3c642
d88d118
0b6153c
b76bde1
0b33f0b
7ab0398
110ce9c
5ec8b7a
f2c5ba6
a6f005d
e234edc
c384fee
8378601
1aa5f1c
08f2402
26f9ec2
ee3a730
ecca6d6
7203db0
5248d51
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 { newCollection } from "./service.js"; | ||
import type { NewCollectionRequestHandler } from "./types.js"; | ||
|
||
export const newCollectionController: NewCollectionRequestHandler = async ( | ||
_req, | ||
res, | ||
) => { | ||
try { | ||
const params = res.locals.parsedReq.body; | ||
const collection = await newCollection(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 |
---|---|---|
@@ -1,0 +1,90 @@ | ||
import { MetabaseError, createMetabaseClient } from "../shared/client.js"; | ||
import type { NewCollectionParams } from "./types.js"; | ||
import { toSnakeCase } from "../shared/utils.js"; | ||
|
||
const client = createMetabaseClient(); | ||
|
||
export async function authentication(): Promise<boolean> { | ||
try { | ||
const response = await client.get("/user/current"); | ||
return response.status === 200; | ||
} catch (error) { | ||
console.error("Error testing Metabase connection:", error); | ||
return false; | ||
} | ||
} | ||
|
||
export async function newCollection({ | ||
name, | ||
description, | ||
parent_id, | ||
}: NewCollectionParams): Promise<any> { | ||
try { | ||
// Check if collection exists | ||
const existingCollectionId = await checkCollections(name); | ||
if (existingCollectionId) { | ||
console.log( | ||
`Collection "${name}" already exists with ID: ${existingCollectionId}`, | ||
); | ||
return existingCollectionId; | ||
} | ||
|
||
// If no existing collection, create new one | ||
const requestBody = toSnakeCase({ | ||
name, | ||
description, | ||
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. Rather than casting to snake_case, the API should accept snake_case, and then we can cast or transform as necessary for the Metabase request. Incoming request (camelCase) → Validate → Transform to snake_case → Make request to Metabase 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. Ended up using |
||
|
||
// Remove undefined properties | ||
Object.keys(requestBody).forEach( | ||
(key) => requestBody[key] === undefined && delete requestBody[key], | ||
); | ||
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. Passing through the |
||
|
||
const response = await client.post(`/api/collection/`, { | ||
name, | ||
description, | ||
parent_id, | ||
}); | ||
console.log( | ||
`New collection: ${response.data.name}, new collection ID: ${response.data.id}`, | ||
); | ||
return response.data.id; | ||
} catch (error) { | ||
console.error("Error in newCollection:"); | ||
throw error; | ||
} | ||
} | ||
|
||
/** | ||
* Checks if a collection exists with name matching `teamName` exists. | ||
* Returns the matching collection ID if exists, otherwise false. */ | ||
export async function checkCollections(teamName: string): Promise<any> { | ||
try { | ||
console.log("Checking for collection: ", teamName); | ||
|
||
// Get collections from Metabase | ||
const response = await client.get(`/api/collection/`); | ||
|
||
const matchingCollection = response.data.find((collection: any) => | ||
collection.name.toLowerCase().includes(teamName.toLowerCase()), | ||
); | ||
|
||
if (matchingCollection) { | ||
console.log("Matching collection found with ID: ", matchingCollection.id); | ||
return matchingCollection.id; | ||
} else { | ||
console.log("No matching collection found"); | ||
return undefined; | ||
} | ||
} catch (error) { | ||
console.error("Error: ", error); | ||
if (error instanceof MetabaseError) { | ||
console.error("Metabase API error:", { | ||
message: error.message, | ||
statusCode: error.statusCode, | ||
}); | ||
} | ||
throw error; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import { z } from "zod"; | ||
import type { ValidatedRequestHandler } from "../../../../shared/middleware/validate.js"; | ||
|
||
type ApiResponse<T> = { | ||
data?: T; | ||
error?: string; | ||
}; | ||
|
||
export interface NewCollectionParams { | ||
jamdelion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
name: string; | ||
description?: string; | ||
/** Optional; if the collection is a child of a parent, specify parent ID here | ||
* For council teams, parent collection should be 58 | ||
*/ | ||
parent_id?: number; | ||
jamdelion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** Metbase collection ID for the the "Council" collection **/ | ||
const COUNCIL_COLLECTION_ID = 58; | ||
|
||
export const newCollectionSchema = z.object({ | ||
body: z.object({ | ||
name: z.string(), | ||
description: z.string().optional(), | ||
parent_id: z.number().default(COUNCIL_COLLECTION_ID), | ||
}), | ||
}); | ||
|
||
export type NewCollectionRequestHandler = ValidatedRequestHandler< | ||
typeof newCollectionSchema, | ||
ApiResponse<NewCollectionResponse> | ||
>; | ||
|
||
export interface NewCollectionResponse { | ||
id: number; | ||
name: string; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
export interface Input { | ||
teamName: string; | ||
originalDashboardId: number; | ||
/** The name of the filter to be updated */ | ||
filter: string; | ||
/** The value of the new filter; updateFilter() only supports strings right now */ | ||
filterValue: string; | ||
} | ||
|
||
export function validateInput(input: unknown): input is Input { | ||
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 looks like it's currently unused - is that right? In terms of validation, it would be best to follow the existing pattern in the API. → Incoming request of type |
||
// check that input type is object | ||
if (typeof input !== "object" || input === null) { | ||
return false; | ||
} | ||
|
||
// check that input object is same shape as Input type with same properties | ||
const { teamName, originalDashboardId, filter, filterValue } = input as Input; | ||
|
||
if (typeof teamName !== "string" || teamName.trim() === "") { | ||
console.error("Invalid teamName: must be a non-empty string"); | ||
return false; | ||
} | ||
|
||
if (!Number.isInteger(originalDashboardId) || originalDashboardId <= 0) { | ||
console.error("Invalid originalDashboardId: must be a positive integer"); | ||
return false; | ||
} | ||
|
||
if (typeof filter !== "string" || filter.trim() === "") { | ||
console.error("Invalid filter: must be a non-empty string"); | ||
return false; | ||
} | ||
|
||
if (typeof filterValue !== "string") { | ||
console.error("Invalid filterValue: must be a string"); | ||
return false; | ||
} | ||
console.log("Input valid"); | ||
return true; | ||
} |
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. For functions like this, I'd suggest importing from lodash (which we already use in the API) instead of rolling our own. It means we're using well tested and efficient utility functions, and there's not much overhead in terms of package size. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/** | ||
* Converts object keys from camelCase to snake_case | ||
*/ | ||
export function toSnakeCase<T extends object>(obj: T): Record<string, unknown> { | ||
return Object.entries(obj).reduce( | ||
(acc, [key, value]) => { | ||
// Convert camelCase to snake_case | ||
const snakeKey = key.replace( | ||
/[A-Z]/g, | ||
(letter) => `_${letter.toLowerCase()}`, | ||
); | ||
acc[snakeKey] = value; | ||
return acc; | ||
}, | ||
{} as Record<string, unknown>, | ||
); | ||
} | ||
|
||
/** | ||
* Converts object keys from snake_case to camelCase | ||
*/ | ||
export function toCamelCase<T extends object>(obj: T): Record<string, unknown> { | ||
return Object.entries(obj).reduce( | ||
(acc, [key, value]) => { | ||
// Convert snake_case to camelCase | ||
const camelKey = key.replace(/_([a-z])/g, (_, letter) => | ||
letter.toUpperCase(), | ||
); | ||
acc[camelKey] = value; | ||
return acc; | ||
}, | ||
{} as Record<string, unknown>, | ||
); | ||
} |
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 also looks unused currently.
I think this should be a middleware added to the route - before proceeding to make additional requests to Metabase, we can first check the API key is valid.
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 had a quick chat with Jo, who pointed out that it might not be necessary as the authentication function was basically checking that we could
get
before actuallyget
-ing (which happens in service.ts, incheckCollections()
:Between
validateConfig()
inclient.ts
and running theget
above, does that mean authentication is redundant? Or do we still need another check for the valid API key? I believe the firstget
would just throw aMetabaseError
if it was invalid, is that sufficient?