feat: Metabase module controller and service

[zz-hh-aa]

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]

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

[jamdelion]

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]

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]

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]

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

[zz-hh-aa]

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

Loading

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.

[zz-hh-aa]

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)

[zz-hh-aa]

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]

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

[zz-hh-aa]

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?

[github-actions]

Pizza

Deployed caa6e65 to https://4072.planx.pizza.

Useful links:

• Editor
• Storybook
• Hasura
• API
• ShareDB

[jamdelion]

Couple of nits to do with comments but overall great work getting this in good shape! 👍 (I assume Daf's request for changes is stale now)

[DafyddLlyr]

Looking great - a few very minor suggestions but no blockers at all! 👏

[DafyddLlyr]

We could also check this with zod using .min(1)