feat: Metabase module controller and service #4072

zz-hh-aa · 2024-12-11T17:43:00Z

What does this PR do

Creates createCollectionIfDoesNotExist service
Creates createCollection Metabase API call
Adds necessary types
Adds necessary tests
Creates new route for analytics module
Fixes an import that was missing from the Metabase client (in order to get this working) [Maybe this should have gone in another PR..?]

Why

This is the second PR that breaks up #3971 (follows #4071)

jamdelion · 2024-12-13T14:38:22Z

api.planx.uk/modules/analytics/metabase/collection/service.ts

+import { getTeamIdAndMetabaseId } from "./getTeamIdAndMetabaseId.js";
+import { createCollection } from "./createCollection.js";
+
+const client = createMetabaseClient();


I don't think this line is needed, the client doesn't appear to be used anywhere

Might be worth following this kind of pattern in all these places where client is used - see the getClient pattern in this file:

planx-new/api.planx.uk/modules/flows/moveFlow/service.ts

Line 7 in 47aba73

const $client = getClient();

jamdelion · 2024-12-13T14:57:44Z

We're also getting an API test failure here in a test suite unrelated to metabase: https://github.com/theopensystemslab/planx-new/blob/47aba73d1eefdf8782b12493264a925e1a58969c/api.planx.uk/modules/send/s3/index.test.ts

I think this is because there we are globally mocking axios and not accounting for the new use case we have in metabase/shared/client.ts: axios.create. I'm not immediately sure how to fix this, somehow I think we need to restrict the mock in this file so that it doesn't care about how we are using axios elsewhere

zz-hh-aa · 2024-12-16T15:21:00Z

Thanks @jamdelion, I sought Claude's advice on restricting the mock in this file and tried a few things out but to no avail.

It looked like the axios spy in client.test.ts wasn't actually being used but a.) that didn't solve the problem and b.) even if it did, it probably wouldn't belong in this PR anyway would it 😅. So guess I should remove commit 778b6d4?

Maybe we can flag at dev call on Thursday?

DafyddLlyr · 2024-12-18T16:15:12Z

Happy to pick this one up and take a look once rebased and the merge conflicts are resolved 👍

zz-hh-aa · 2024-12-18T16:55:07Z

Cheers @DafyddLlyr I've just resolved those--up for talking things through tomorrow if you've got time then but no rush :)

DafyddLlyr

I've had a quick pass through with some initial comments - always happy to catch up and talk through if you'd like.

I'll checkout the branch and see if I can get to the bottom of the issue with tests and point you in the right direction.

In my mind, we're aiming for something like this - does this match your mental model?

sequenceDiagram
    participant User
    participant Hasura
    participant API
    participant MetabaseClient

    Note over User, Hasura: User Integration
    User->>Hasura: Create Team
    Hasura->>Hasura: Add record to teams table
    Hasura-->>User: Success

    Note over Hasura, MetabaseClient: Async action
    Hasura->>Hasura: Event: New team created
    Hasura->>API: Create collection request
    API->>MetabaseClient: Create collection
    MetabaseClient-->>API: Success
    API->>Hasura: Update Team Table
    Hasura-->>API: Success

Once we have an API endpoint that we can call to create a collection for a team, the next step will be to call it automatically when a team is created, and to populate the current production db with the existing collection ids.

DafyddLlyr · 2024-12-19T08:30:41Z

api.planx.uk/modules/analytics/routes.ts

+router.post(
+  "/collection",
+  validate(createCollectionIfDoesNotExistSchema),
+  MetabaseCollectionsController,


Suggested change

MetabaseCollectionsController,

metabaseCollectionsController,

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.

DafyddLlyr · 2024-12-19T08:32:37Z

api.planx.uk/modules/analytics/routes.ts

@@ -18,5 +20,10 @@ router.post(
  validate(logAnalyticsSchema),
  logUserResumeController,
 );
+router.post(
+  "/collection",


API docs (docs.yaml) need to be updated each time we create a new route. There's (currently!) no automated step for this.

DafyddLlyr · 2024-12-19T08:38:48Z

api.planx.uk/modules/analytics/metabase/collection/types.ts

+      description: z.string().optional(),
+      parentId: z.number().optional(), //.default(COUNCILS_COLLECTION_ID),
+    })
+    .transform((data) => ({


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 createCollection.ts?

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 🙂

DafyddLlyr · 2024-12-19T08:40:18Z

api.planx.uk/modules/analytics/metabase/collection/createCollection.ts

+
+export async function createCollection(
+  params: NewCollectionParams,
+): Promise<any> {


nit: We want to avoid using any wherever we can

api.planx.uk/modules/analytics/metabase/collection/createCollection.ts

DafyddLlyr · 2024-12-19T08:45:01Z

api.planx.uk/modules/analytics/metabase/collection/service.ts

+      return metabaseId;
+    }
+
+    // Create new Metabase collection if !metabaseId


nit: Redundant

DafyddLlyr · 2024-12-19T08:46:24Z

api.planx.uk/modules/analytics/metabase/collection/getCollection.ts

+import { createMetabaseClient } from "../shared/client.js";
+import type { GetCollectionResponse } from "./types.js";
+
+const client = createMetabaseClient();


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. $public or $api).

DafyddLlyr · 2024-12-19T08:53:10Z

api.planx.uk/modules/analytics/routes.ts

@@ -18,5 +20,10 @@ router.post(
  validate(logAnalyticsSchema),
  logUserResumeController,
 );
+router.post(
+  "/collection",


We should also contextualise this a little more - currently the route is just api.editors.planx.dev/collection.

It would be useful I think to prefix all of the Metabase specific paths, e.g. api.editors.planx.dev/metabase/collection.

I actually think that api.editors.planx.dev/metabase/:teamSlug/collection (instead of using name in the request body) might be a good naming convention - it's clear we're creating a collection for a team.

Later routes could be api.editors.planx.dev/metabase/:teamSlug/collection/:collectionId/dashboard etc

I think I have sorted this (f4dea96), though I went with collection/:teamSlug--does that order seem important?

Nice! Order here is totally fine 👍

DafyddLlyr · 2024-12-19T08:56:13Z

api.planx.uk/modules/analytics/metabase/collection/types.ts

+export const createCollectionIfDoesNotExistSchema = z.object({
+  body: z
+    .object({
+      name: z.string(),


name is a little vague - this is actually the teamSlug right? Took me a minute to catch that - we should update this.

DafyddLlyr · 2024-12-19T08:58:59Z

api.planx.uk/modules/analytics/metabase/collection/types.ts

+/** Metbase collection ID for the the "Council" collection **/
+// const COUNCILS_COLLECTION_ID = 58;


Are we hitting issues here with 58 just being the ID on production and not locally and on staging?

Yep, that's why it's commented out. Is there a better way to handle this in the meantime?

I think there's three main approaches -

We set up COUNCILS_COLLECTION_ID as an env var and pass in a valid ID for staging/pizza/localhost - this will differ per dev so not ideal but perfectly workable as an initial step

We commit to Metabase being prod only, and add a middleware to all /metabase routes on our API which just exits early if we're not on production - makes testing tricky!

We set up Metabase on staging and sync data down

We might end up on the third option, but let's work our way there via 1 and 2 first!

In the meantime no issues keeping this in place and commented out.

zz-hh-aa · 2024-12-19T15:23:28Z

Made this back into a draft because I realised that I've just made team slugs the name of the Metabase collections too--will fix that (and look into api.editors.planx.dev/metabase/:teamSlug/collection routes question too)

not team slug

zz-hh-aa · 2024-12-19T16:55:14Z

Okay I think we're back up and running (aside from the failing tests 😅). No rush at all for feedback.

To respond to @DafyddLlyr's previous point:

In my mind, we're aiming for something like this - does this match your mental model?

Largely yes, asides from the script being triggered by the User - Create Team event (but as mentioned above, this is outside the scope of this PR). Originally, since not all services will have analytics and not all teams will have Metabase collections, I thought the createTeamCollection would only run when a service that needed analytics went live. I guess this is still to be ironed out exactly!

DafyddLlyr

Code looking great!

Just looking a bit deeper at tests now to sort out CI. One initial issue is that we don't have the right env vars set up on AWS (again 🤦‍♂️).

The good news is that I think I worked out the cause of our headaches here. There's been a few PRs like this recently which have update the env file - #4094. I think that these have just pushed a new .env file without first pulling, thus overwriting your new variables. The timing seems to line up. Could you please update the .env files on S3 again please (pull then push)?

DafyddLlyr · 2024-12-20T11:07:23Z

api.planx.uk/modules/analytics/docs.yaml

+
 tags:
  - name: analytics
+  - name: metabase
+


Despite the plural "tags", Swagger only allows a route to be in one location -

We generally want these to match the structure of out API modules - we can either being metabase out to it's own module, or keep it in analytics.

I'd argue it's it's own module (is there any shared types/code/logic?) but this isn't a blocker and we can make this decision later - it's just a case of moving file around.

For now, let's chose the easy path and just apply the analytics tag.

DafyddLlyr · 2024-12-20T11:10:30Z

api.planx.uk/modules/analytics/metabase/shared/client.ts

-//   metabaseClient = createMetabaseClient();
-//   return metabaseClient;
-// };
+export const $metabase = createMetabaseClient();


DafyddLlyr · 2024-12-20T11:12:34Z

api.planx.uk/modules/analytics/metabase/collection/service.ts

+    const metabaseParams = {
+      name,
+      ...rest,
+    } as const;


I'm not sure this as const is needed here.

🧙‍♂️ https://www.youtube.com/watch?v=6M9aZzm-kEc

DafyddLlyr · 2024-12-20T11:14:49Z

api.planx.uk/modules/analytics/metabase/collection/service.ts

+      return metabaseId;
+    }
+
+    const { slug, ...rest } = params;


Please see linting comment on PR about unused var.

DafyddLlyr · 2024-12-20T11:16:05Z

api.planx.uk/modules/analytics/metabase/collection/service.ts

+ * @returns `response.data`, so use dot notation to access `id` or `parent_id`.
+ */
+export async function createTeamCollection(
+  params: NewCollectionParams,


Suggested change

params: NewCollectionParams,

{ slug, parentId, description }: NewCollectionParams,

We could use destructing here to save a big of complexity and mapping below.

DafyddLlyr · 2024-12-20T15:23:04Z

@zz-hh-aa Please see 55d089b which should resolve the test issues in the API files.

zz-hh-aa · 2024-12-23T11:20:43Z

Thanks for the review once again @DafyddLlyr! Sadly things aren't quite working yet🫠

I've done the tidying up tasks, hopefully those are now resolved.

I also re-pulled / pushed the vars (cd into scripts/ and ran pull-secrets.sh planx-staging followed by push-secrets.sh planx-staging), but it looks like that hasn't helped. Not sure if I have done something wrong here?

API tests are also failing despite 55d089b. I don't quite understand the issue, since it looks like there aren't explicit error messages, just warnings. I also thought the 'code style' issues should be addressed through pre-commit scripts, but maybe I've misunderstood?

zz-hh-aa requested a review from jamdelion December 11, 2024 17:43

jamdelion reviewed Dec 13, 2024

View reviewed changes

Base automatically changed from oz/analytics-planx-db to main December 18, 2024 15:55

zz-hh-aa added 10 commits December 18, 2024 16:46

feat: new createCollection metabase api call

71fb982

feat: new get collection from metabase function

530f60e

feat: createCollectionIfDoesNotExist function

50f0c18

feat: new types for collection service

6c34c73

fix: import in metabase client

28b3de6

feat: new collection route in api

07a969c

fix: controller type error

521689b

test: test for full metabase collection service

b097861

chore: remove unnecessary client import

30cfbb4

test: remove unused axios spy

6f251b1

zz-hh-aa force-pushed the oz/analytics-metabase-collection branch from 778b6d4 to 6f251b1 Compare December 18, 2024 16:47

DafyddLlyr requested changes Dec 19, 2024

View reviewed changes

zz-hh-aa added 5 commits December 19, 2024 09:28

chore: change functions to camelCase

398d9b8

chore: tidy types

08d38f2

feat: update function name for clarity

632834e

fix: update name to slug

15ad49c

chore: replace metabase client w singleton instance

8f12f20

zz-hh-aa marked this pull request as draft December 19, 2024 15:21

zz-hh-aa force-pushed the oz/analytics-metabase-collection branch from a7c471e to e1a5032 Compare December 19, 2024 15:27

zz-hh-aa added 3 commits December 19, 2024 16:12

feat: update route and swagger docs

85d73f4

feat: use team name for metabase collection name

445a576

not team slug

feat: change route to include team slug

f4dea96

zz-hh-aa force-pushed the oz/analytics-metabase-collection branch from e1a5032 to f4dea96 Compare December 19, 2024 16:44

chore: remove console.logs

5d8818c

zz-hh-aa marked this pull request as ready for review December 19, 2024 16:56

DafyddLlyr reviewed Dec 20, 2024

View reviewed changes

test(api): Set up mock for metabase client

55d089b

zz-hh-aa added 2 commits December 23, 2024 10:35

chore: remove metabase tag from swagger docs

c9e9740

chore: tidy imports and variables

6d8cd9c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Metabase module controller and service #4072

feat: Metabase module controller and service #4072

zz-hh-aa commented Dec 11, 2024

jamdelion Dec 13, 2024

jamdelion Dec 13, 2024 •

edited

Loading

jamdelion commented Dec 13, 2024

zz-hh-aa commented Dec 16, 2024

DafyddLlyr commented Dec 18, 2024

zz-hh-aa commented Dec 18, 2024

DafyddLlyr left a comment

DafyddLlyr Dec 19, 2024

DafyddLlyr Dec 19, 2024

DafyddLlyr Dec 19, 2024

DafyddLlyr Dec 19, 2024

DafyddLlyr Dec 19, 2024

DafyddLlyr Dec 19, 2024

DafyddLlyr Dec 19, 2024

zz-hh-aa Dec 19, 2024 •

edited

Loading

DafyddLlyr Dec 20, 2024

DafyddLlyr Dec 19, 2024

DafyddLlyr Dec 19, 2024

zz-hh-aa Dec 19, 2024

DafyddLlyr Dec 19, 2024

DafyddLlyr Dec 19, 2024

zz-hh-aa commented Dec 19, 2024

zz-hh-aa commented Dec 19, 2024 •

edited

Loading

DafyddLlyr left a comment

DafyddLlyr Dec 20, 2024

DafyddLlyr Dec 20, 2024

DafyddLlyr Dec 20, 2024

DafyddLlyr Dec 20, 2024

DafyddLlyr Dec 20, 2024

DafyddLlyr commented Dec 20, 2024

zz-hh-aa commented Dec 23, 2024

	MetabaseCollectionsController,
	metabaseCollectionsController,

		/ Metbase collection ID for the the "Council" collection /
		// const COUNCILS_COLLECTION_ID = 58;

	params: NewCollectionParams,
	{ slug, parentId, description }: NewCollectionParams,

feat: Metabase module controller and service #4072

Are you sure you want to change the base?

feat: Metabase module controller and service #4072

Conversation

zz-hh-aa commented Dec 11, 2024

What does this PR do

Why

Choose a reason for hiding this comment

jamdelion Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

jamdelion commented Dec 13, 2024

zz-hh-aa commented Dec 16, 2024

DafyddLlyr commented Dec 18, 2024

zz-hh-aa commented Dec 18, 2024

DafyddLlyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-hh-aa Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-hh-aa commented Dec 19, 2024

zz-hh-aa commented Dec 19, 2024 • edited Loading

DafyddLlyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DafyddLlyr commented Dec 20, 2024

zz-hh-aa commented Dec 23, 2024

jamdelion Dec 13, 2024 •

edited

Loading

zz-hh-aa Dec 19, 2024 •

edited

Loading

zz-hh-aa commented Dec 19, 2024 •

edited

Loading