wip: feat: collection service & controller

[zz-hh-aa]

Everything seems to work now--all of my tests pass and I have also successfully used Insomnia to check and update the PlanX DB (locally) and create collections in Metabase (staging).

I think the next steps would be to break this up into two PRs with cleaner commit histories. Instinctively I would put everything that interacts with the PlanX DB into one (getTeamAndMetabaseId and updateMetabaseId) plus tests, and everything that only interacts with Metabase into another (createCollection). OR I could put the controller in one along with the routes update, and leave everything else in a bigger one? Feedback and thoughts very welcome!

What does this PR do?

PlanX DB

• Creates a migration adding a new column metabase_id to teams
• Updates api permissions to be able to update metabase_id and select others

Controller and routes

• Creates a new route in PlanX API
• Creates a new controller that runs checkCollections service

Service

• checkCollections is the main service, which runs other relevant functions
  • getTeamAndMetabaseId fetches id and metabase_id from teams
  • if there is no metabase-id, then createCollection runs, creating new collection and returning Metabase collection ID
  • updateMetabaseId runs updating teams.metabase_id

There are also tests!

Questions

• Should getTeamAndMetabaseId be broken up into getTeam and getMetabaseId?
• Did I update the api permissions correctly 😅

[github-actions]

Pizza

Deployed b2702f1 to https://3971.planx.pizza.

Useful links:

• Editor
• Storybook
• Hasura
• API
• ShareDB

[DafyddLlyr]

A few minor comment and questions on the code.

I think a stronger pattern here would be to store Metabase collection IDs to the PlanX database. This will mean we don't have to query all Metabase collections and find the team each time we make a request. We can manually populate these values for existing teams.

When we add a new team, we want to automatically create a collection for them. This action should return an ID which we can persist in our database. When a new flow is created within this team, we'll then already have the collection ID to hand to ensure the dashboard is created in the correct collection.

I think this would be both simpler and a bit more robust (less room for failures) 👍

[DafyddLlyr]

We should be using the type for this request handler defined in types.ts - this is already handling this validation and parsing for us.

[DafyddLlyr]

I'm not yet quite picking up when and where this would be used - I guess we need the collection ID (i.e. teamId) in order to make a new dashboard?

If this is the case, we likely won't need a controller and exposed endpoint here - we can just call this in the service when and where we need it.

[DafyddLlyr]

We already have error handling set up in the API via Express and it's errorHandler() - this should mean we can just throw here with a helpful message and allow it to be caught upstream.

I think we'll need to add a check for MetabaseError there, but that should be it and it will allow us to simplify a lot of this repeated logic 👍

Docs: https://expressjs.com/en/guide/error-handling.html

[DafyddLlyr]

Suggested change

	export type CheckCollectionsRequest = ValidatedRequestHandler<
	export type CheckCollections = ValidatedRequestHandler<

nit: These aren't requests, but request handlers - ValidatedRequestHandler<Req, Res>

[DafyddLlyr]

This is a bit of an unexpected response - I'd expect maybe number | null or number | undefined;

[DafyddLlyr]

nit: Redundant

[DafyddLlyr]

I think it's safe to assume all new collections will be grouped here by default.

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

...
parent_id: z.number().default(COUNCIL_COLLECTION_ID)

[zz-hh-aa]

I think a stronger pattern here would be to store Metabase collection IDs to the PlanX database. This will mean we don't have to query all Metabase collections and find the team each time we make a request. We can manually populate these values for existing teams.

This makes a lot of sense to me! I haven't tried to address this in this PR because a.) I don't know how to deal with the PlanX database and b.) I thought this would get a bit too messy. Let me know if you think this is the right place for it!

[DafyddLlyr]

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.

[DafyddLlyr]

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

[zz-hh-aa]

Ended up using transform instead of importing the Lodash method.

[DafyddLlyr]

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 unknown →
→ Passes through existing validate() middleware in route →
→ Using Zod, is checked against a provided schema →
→ Validated payload is automatically passed along as res.locals.parsedReq →
→ Controller and Service have both type safety and a validated payload

[DafyddLlyr]

Passing through the validate() middleware should handle this step already - no additional keys would be present.

[DafyddLlyr]

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.

[zz-hh-aa]

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 actually get-ing (which happens in service.ts, in checkCollections():

const response = await client.get(`/api/collection/`);

Between validateConfig() in client.ts and running the get above, does that mean authentication is redundant? Or do we still need another check for the valid API key? I believe the first get would just throw a MetabaseError if it was invalid, is that sufficient?

[github-actions]

🤖 Hasura Change Summary compared a subset of table metadata including permissions:

Updated Tables (1)

• public.teams permissions:

  	insert	select	update	delete
  api		➕/➖	➕

  2 added column permissions

  	insert	select	update
  api		➕ metabase_id	➕ metabase_id

[jamdelion]

Probably better in its own file getCollection.ts or something like that :)

[jamdelion]

Probably would be nice to type the response rather than have <any>

[jamdelion]

Same comment as below - would be nice to type the response rather than have <any>

[jamdelion]

I think this is fine as one gql request, maybe I'd change the function name to be getTeamIdAndMetabaseId just to be completely explicit

[jamdelion]

You can do this on one line like this:

    const { metabaseId, id } = await getTeamAndMetabaseId(params.name);

And if you want to recast id to teamId so it's more obvious what it is, you can do:

    const { metabaseId, id: teamId } = await getTeamAndMetabaseId(params.name);

Then make sure you use teamId in the rest of the codeblock.

[jamdelion]

Maybe I'd call this createCollectionIfDoesNotExist - "checking" is a bit of a vague term

[jamdelion]

Then maybe the controller could just be called CollectionsController or even MetabaseCollectionsController

[jamdelion]

Added a few comments to try and answer some questions! :) I think your intuition about splitting on metabase/planX db lines is correct.

I haven't tested this locally (think I'm missing the metabase env vars) but sounds like it was working from our chat yesterday 🙌 Maybe include some brief testing instructions when you have the PRs split out.