diff --git a/api.planx.uk/modules/analytics/docs.yaml b/api.planx.uk/modules/analytics/docs.yaml index bfd6f25e0a..a97bb173d4 100644 --- a/api.planx.uk/modules/analytics/docs.yaml +++ b/api.planx.uk/modules/analytics/docs.yaml @@ -2,12 +2,26 @@ openapi: 3.1.0 info: title: Planâś• API version: 0.1.0 + tags: - name: analytics + components: responses: AnalyticsResponse: description: Successful response with no content. Not awaited from server as endpoint is called via the Beacon API + + schemas: + NewCollection: + type: object + properties: + description: + type: string + description: Optional description for the collection + parentId: + type: integer + description: Optional ID of the parent collection + paths: /analytics/log-user-exit: post: @@ -18,6 +32,7 @@ paths: responses: "204": $ref: "#/components/responses/AnalyticsResponse" + /analytics/log-user-resume: post: summary: Log user resume @@ -27,3 +42,44 @@ paths: responses: "204": $ref: "#/components/responses/AnalyticsResponse" + + /metabase/collection/{slug}: + post: + summary: Create new Metabase collection + description: Creates a new collection in Metabase if it doesn't already exist + tags: + - metabase + parameters: + - name: slug + in: path + required: true + schema: + type: string + description: PlanX team slug + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/NewCollection" + responses: + "201": + description: Collection created successfully + content: + application/json: + schema: + type: object + properties: + data: + type: integer + description: Metabase collection ID + "400": + description: Bad request or collection creation failed + content: + application/json: + schema: + type: object + properties: + error: + type: string + description: Error message diff --git a/api.planx.uk/modules/analytics/metabase/collection/collection.test.ts b/api.planx.uk/modules/analytics/metabase/collection/collection.test.ts index 4b9829dd69..ec1dddc8f5 100644 --- a/api.planx.uk/modules/analytics/metabase/collection/collection.test.ts +++ b/api.planx.uk/modules/analytics/metabase/collection/collection.test.ts @@ -1,6 +1,138 @@ +import { createTeamCollection } from "./service.js"; +import { getCollection } from "./getCollection.js"; +import nock from "nock"; +import { MetabaseError } from "../shared/client.js"; import { $api } from "../../../../client/index.js"; import { updateMetabaseId } from "./updateMetabaseId.js"; import { getTeamIdAndMetabaseId } from "./getTeamIdAndMetabaseId.js"; +import { createCollection } from "./createCollection.js"; + +describe("createTeamCollection", () => { + beforeEach(() => { + nock.cleanAll(); + }); + + test("creates new collection when metabase ID doesn't exist", async () => { + // Mock getTeamIdAndMetabaseId response with null metabase_id + vi.spyOn($api.client, "request").mockResolvedValueOnce({ + teams: [ + { + id: 26, + slug: "barnet", + metabase_id: null, + }, + ], + }); + + // Mock Metabase API calls + const metabaseMock = nock(process.env.METABASE_URL_EXT!) + .post("/api/collection/", { + name: "Barnet", + }) + .reply(200, { + id: 123, + name: "Barnet", + }); + + const collectionId = await createCollection({ + name: "Barnet", + }); + + expect(collectionId).toBe(123); + expect(metabaseMock.isDone()).toBe(true); + }); + + test("successfully places new collection in parent", async () => { + // Mock updateMetabaseId response + vi.spyOn($api.client, "request").mockResolvedValueOnce({ + update_teams: { + returning: [ + { + id: 26, + name: "Barnet", + slug: "barnet", + metabase_id: 123, + }, + ], + }, + }); + + const testName = "Example Council"; + const metabaseMock = nock(process.env.METABASE_URL_EXT!); + + // Mock collection creation endpoint + metabaseMock + .post("/api/collection/", { + name: testName, + parentId: 100, + }) + .reply(200, { + id: 123, + name: testName, + parentId: 100, + }); + + // Mock GET request for verifying the new collection + metabaseMock.get("/api/collection/123").reply(200, { + id: 123, + name: testName, + parentId: 100, + }); + + const collectionId = await createCollection({ + name: testName, + parentId: 100, + }); + + // Check the ID is returned correctly + expect(collectionId).toBe(123); + + // Verify the collection details using the service function + const collection = await getCollection(collectionId); + expect(collection.parentId).toBe(100); + expect(metabaseMock.isDone()).toBe(true); + }); + + test("returns collection correctly no matter collection slug case", async () => { + vi.spyOn($api.client, "request").mockResolvedValueOnce({ + teams: [ + { + id: 26, + name: "Barnet", + slug: "barnet", + metabaseId: 20, + }, + ], + }); + + const collection = await getTeamIdAndMetabaseId("BARNET"); + expect(collection.metabaseId).toBe(20); + }); + + test("throws error if network failure", async () => { + nock(process.env.METABASE_URL_EXT!) + .get("/api/collection/") + .replyWithError("Network error occurred"); + + await expect( + createCollection({ + name: "Test Collection", + }), + ).rejects.toThrow("Network error occurred"); + }); + + test("throws error if API error", async () => { + nock(process.env.METABASE_URL_EXT!).get("/api/collection/").reply(400, { + message: "Bad request", + }); + + await expect( + createCollection({ + name: "Test Collection", + }), + ).rejects.toThrow(MetabaseError); + }); +}); describe("getTeamIdAndMetabaseId", () => { beforeEach(() => { @@ -12,6 +144,7 @@ describe("getTeamIdAndMetabaseId", () => { teams: [ { id: 26, + name: "Barnet", slug: "barnet", metabaseId: 20, }, @@ -90,3 +223,59 @@ describe("updateMetabaseId", () => { await expect(updateMetabaseId(1, 123)).rejects.toThrow("GraphQL error"); }); }); + +describe("edge cases", () => { + beforeEach(() => { + nock.cleanAll(); + vi.clearAllMocks(); + vi.resetAllMocks(); + }); + + test("handles missing slug", async () => { + await expect( + createTeamCollection({ + slug: "", + }), + ).rejects.toThrow(); + }); + + test("handles name with special characters", async () => { + const specialName = "@#$%^&*"; + + nock(process.env.METABASE_URL_EXT!).get("/api/collection/").reply(200, []); + + nock(process.env.METABASE_URL_EXT!) + .post("/api/collection/", { + name: specialName, + }) + .reply(200, { + id: 789, + name: specialName, + }); + + const collection = await createCollection({ + name: specialName, + }); + expect(collection).toBe(789); + }); + + test("handles very long slugs", async () => { + const longSlug = "A".repeat(101); + + nock(process.env.METABASE_URL_EXT!).get("/api/collection/").reply(200, []); + + nock(process.env.METABASE_URL_EXT!) + .post("/api/collection/", { + slug: longSlug, + }) + .reply(400, { + message: "Slug too long", + }); + + await expect( + createTeamCollection({ + slug: longSlug, + }), + ).rejects.toThrow(); + }); +}); diff --git a/api.planx.uk/modules/analytics/metabase/collection/controller.ts b/api.planx.uk/modules/analytics/metabase/collection/controller.ts index e69de29bb2..b88c3de553 100644 --- a/api.planx.uk/modules/analytics/metabase/collection/controller.ts +++ b/api.planx.uk/modules/analytics/metabase/collection/controller.ts @@ -0,0 +1,21 @@ +import { createTeamCollection } from "./service.js"; +import type { NewCollectionRequestHandler } from "./types.js"; + +export const metabaseCollectionsController: NewCollectionRequestHandler = + async (_req, res) => { + try { + const params = { + ...res.locals.parsedReq.params, + ...res.locals.parsedReq.body, + }; + const collection = await createTeamCollection(params); + res.status(201).json({ data: collection }); + } catch (error) { + res.status(400).json({ + error: + error instanceof Error + ? error.message + : "An unexpected error occurred", + }); + } + }; diff --git a/api.planx.uk/modules/analytics/metabase/collection/createCollection.ts b/api.planx.uk/modules/analytics/metabase/collection/createCollection.ts new file mode 100644 index 0000000000..56fe09a0de --- /dev/null +++ b/api.planx.uk/modules/analytics/metabase/collection/createCollection.ts @@ -0,0 +1,9 @@ +import type { MetabaseCollectionParams } from "./types.js"; +import { $metabase } from "../shared/client.js"; + +export async function createCollection( + params: MetabaseCollectionParams, +): Promise { + const response = await $metabase.post(`/api/collection/`, params); + return response.data.id; +} diff --git a/api.planx.uk/modules/analytics/metabase/collection/getCollection.ts b/api.planx.uk/modules/analytics/metabase/collection/getCollection.ts new file mode 100644 index 0000000000..f3031b883b --- /dev/null +++ b/api.planx.uk/modules/analytics/metabase/collection/getCollection.ts @@ -0,0 +1,12 @@ +import type { GetCollectionResponse } from "./types.js"; +import { $metabase } from "../shared/client.js"; + +/** + * 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 + */ +export async function getCollection( + id: number, +): Promise { + const response = await $metabase.get(`/api/collection/${id}`); + return response.data; +} diff --git a/api.planx.uk/modules/analytics/metabase/collection/getTeamIdAndMetabaseId.ts b/api.planx.uk/modules/analytics/metabase/collection/getTeamIdAndMetabaseId.ts index 4007bfef64..5d711158cc 100644 --- a/api.planx.uk/modules/analytics/metabase/collection/getTeamIdAndMetabaseId.ts +++ b/api.planx.uk/modules/analytics/metabase/collection/getTeamIdAndMetabaseId.ts @@ -4,6 +4,7 @@ import { $api } from "../../../../client/index.js"; interface GetMetabaseId { teams: { id: number; + name: string; slug: string; metabaseId: number | null; }[]; @@ -16,6 +17,7 @@ export const getTeamIdAndMetabaseId = async (slug: string) => { query GetTeamAndMetabaseId($slug: String!) { teams(where: { slug: { _eq: $slug } }) { id + name slug metabaseId: metabase_id } diff --git a/api.planx.uk/modules/analytics/metabase/collection/service.ts b/api.planx.uk/modules/analytics/metabase/collection/service.ts index e69de29bb2..8d4296718c 100644 --- a/api.planx.uk/modules/analytics/metabase/collection/service.ts +++ b/api.planx.uk/modules/analytics/metabase/collection/service.ts @@ -0,0 +1,35 @@ +import { updateMetabaseId } from "./updateMetabaseId.js"; +import type { NewCollectionParams } from "./types.js"; +import { getTeamIdAndMetabaseId } from "./getTeamIdAndMetabaseId.js"; +import { createCollection } from "./createCollection.js"; + +/** + * The `getTeamIdAndMetabaseId()` function is run here to first get teams.id and .metabase_id from PlanX db, if present, + * so that the service can figure out if it needs to run `createCollection()` or not. + * Instead of running `getTeamIdAndMetabaseId()` first in the controller, it is encapsulated in `createTeamCollection` to keep business logic in `service.ts` instead of `controller.ts`. + */ +export async function createTeamCollection({ + slug, + parentId, + description, +}: NewCollectionParams): Promise { + try { + const { metabaseId, name, id: teamId } = await getTeamIdAndMetabaseId(slug); + + if (metabaseId) { + return metabaseId; + } + + const newMetabaseId = await createCollection({ + name, + parentId, + description, + }); + + await updateMetabaseId(teamId, newMetabaseId); + return newMetabaseId; + } catch (error) { + console.error("Error in createTeamCollection:", error); + throw error; + } +} diff --git a/api.planx.uk/modules/analytics/metabase/collection/types.ts b/api.planx.uk/modules/analytics/metabase/collection/types.ts index e69de29bb2..2f21a43faf 100644 --- a/api.planx.uk/modules/analytics/metabase/collection/types.ts +++ b/api.planx.uk/modules/analytics/metabase/collection/types.ts @@ -0,0 +1,48 @@ +import { z } from "zod"; +import type { ValidatedRequestHandler } from "../../../../shared/middleware/validate.js"; +import type { ApiResponse } from "../shared/types.js"; + +/** Interface for incoming request, in camelCase */ +export interface NewCollectionParams { + slug: 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 type MetabaseCollectionParams = Omit & { + name: string; +}; + +/** TODO: when running on production, turn below comment back into code + * the Metabase collection ID is for the "Council" collection + * see https://github.com/theopensystemslab/planx-new/pull/4072#discussion_r1892631692 + **/ +// const COUNCILS_COLLECTION_ID = 58; + +export const createTeamCollectionSchema = z.object({ + params: z.object({ + slug: z.string(), + }), + body: z.object({ + description: z.string().optional(), + parentId: z.number().optional(), //.default(COUNCILS_COLLECTION_ID), + }), +}); + +export type NewCollectionRequestHandler = ValidatedRequestHandler< + typeof createTeamCollectionSchema, + ApiResponse +>; + +export interface NewCollectionResponse { + id: number; + slug: string; +} + +export interface GetCollectionResponse { + id: number; + slug: string; + parentId: number; +} diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts index 564909ef6a..ec570fa3aa 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.test.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.test.ts @@ -1,12 +1,10 @@ -import axios from "axios"; import { validateConfig, createMetabaseClient, MetabaseError, } from "./client.js"; import nock from "nock"; - -const axiosCreateSpy = vi.spyOn(axios, "create"); +import { $metabase } from "./client.js"; describe("Metabase client", () => { beforeEach(() => { @@ -19,13 +17,12 @@ describe("Metabase client", () => { }); test("returns configured client", async () => { - const client = createMetabaseClient(); - expect(client.defaults.baseURL).toBe(process.env.METABASE_URL_EXT); - expect(client.defaults.headers["X-API-Key"]).toBe( + expect($metabase.defaults.baseURL).toBe(process.env.METABASE_URL_EXT); + expect($metabase.defaults.headers["X-API-Key"]).toBe( process.env.METABASE_API_KEY, ); - expect(client.defaults.headers["Content-Type"]).toBe("application/json"); - expect(client.defaults.timeout).toBe(30_000); + expect($metabase.defaults.headers["Content-Type"]).toBe("application/json"); + expect($metabase.defaults.timeout).toBe(30_000); }); describe("validates configuration", () => { @@ -64,8 +61,7 @@ describe("Metabase client", () => { .get("/test") .reply(200, { data: "success" }); - const client = createMetabaseClient(); - const response = await client.get("/test"); + const response = await $metabase.get("/test"); expect(response.data).toEqual({ data: "success" }); expect(metabaseScope.isDone()).toBe(true); @@ -79,10 +75,10 @@ describe("Metabase client", () => { .times(4) .reply(500, { message: "Internal Server Error" }); - const client = createMetabaseClient(); + const $metabase = createMetabaseClient(); try { - await client.get("/test"); + await $metabase.get("/test"); expect.fail("Should have thrown an error"); } catch (error) { expect(error).toBeInstanceOf(MetabaseError); @@ -96,8 +92,8 @@ describe("Metabase client", () => { metabaseScope.get("/test").once().reply(200, { data: "success" }); - const client = createMetabaseClient(); - const response = await client.get("/test"); + const $metabase = createMetabaseClient(); + const response = await $metabase.get("/test"); expect(response.data).toEqual({ data: "success" }); diff --git a/api.planx.uk/modules/analytics/metabase/shared/client.ts b/api.planx.uk/modules/analytics/metabase/shared/client.ts index 17c4b7b07c..b7f0b66d03 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/client.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/client.ts @@ -1,4 +1,5 @@ import axios from "axios"; +import assert from "assert"; import type { AxiosInstance, AxiosError, @@ -120,10 +121,4 @@ export const createMetabaseClient = (): AxiosInstance => { return client; }; -// // Export both client and instance with delayed instantiation for test purposes -// export let metabaseClient: AxiosInstance; - -// export const initializeMetabaseClient = () => { -// metabaseClient = createMetabaseClient(); -// return metabaseClient; -// }; +export const $metabase = createMetabaseClient(); diff --git a/api.planx.uk/modules/analytics/metabase/shared/types.ts b/api.planx.uk/modules/analytics/metabase/shared/types.ts index e69de29bb2..77a4fb6d85 100644 --- a/api.planx.uk/modules/analytics/metabase/shared/types.ts +++ b/api.planx.uk/modules/analytics/metabase/shared/types.ts @@ -0,0 +1,4 @@ +export type ApiResponse = { + data?: T; + error?: string; +}; diff --git a/api.planx.uk/modules/analytics/routes.ts b/api.planx.uk/modules/analytics/routes.ts index 9dcaa69010..3de94d5c39 100644 --- a/api.planx.uk/modules/analytics/routes.ts +++ b/api.planx.uk/modules/analytics/routes.ts @@ -5,6 +5,8 @@ import { logUserExitController, logUserResumeController, } from "./analyticsLog/controller.js"; +import { metabaseCollectionsController } from "./metabase/collection/controller.js"; +import { createTeamCollectionSchema } from "./metabase/collection/types.js"; const router = Router(); @@ -18,5 +20,10 @@ router.post( validate(logAnalyticsSchema), logUserResumeController, ); +router.post( + "/metabase/collection/:slug", + validate(createTeamCollectionSchema), + metabaseCollectionsController, +); export default router; diff --git a/api.planx.uk/modules/send/s3/index.test.ts b/api.planx.uk/modules/send/s3/index.test.ts index 4215e17a84..b1c3d958c9 100644 --- a/api.planx.uk/modules/send/s3/index.test.ts +++ b/api.planx.uk/modules/send/s3/index.test.ts @@ -23,6 +23,11 @@ vi.mock("../../file/service/uploadFile.js", () => ({ vi.mock("../../client/index.js"); +// Mock the Metabase client which also relies on Axios which these tests need to mock +vi.mock("../../analytics/metabase/shared/client.js", () => ({ + createMetabaseClient: vi.fn(), +})); + vi.mock("axios", () => ({ default: vi.fn(), }));