Idea: Improve Feature Access #792
Conversation
There was a problem hiding this comment.
Thanks for the PR. I'm afraid I'm going to play the bad cop however, because I don't really agree with the modelization here (DB+models+API), and I don't think corresponds to what we agreed on when we last discussed this :/
When we last discussed this, I was strongly in favor of a modelization broadly like this:
- an
organization_feature_accesstable, with columnstest_run,sanctions(thinking ahead here) as well as all the usual boilerplate (id, org_id, created_at...) - columns are "active" by default when the row is created, though we'll replace this by "all unavailable" or "all test" when we create an org in our internal back-office
- a row in this table is created for every org at org creation time (see the org usecase)
- then basically all we need is the most basic CRUD (GET/PATCH only) on this entity for marble admins.
This is the minimum viable implementation that brings exactly the functionality we need today while minimizing migration overhead (this being something that your implementation does not touch on yet), and while being easy to migrate to something more modular if needed at a later time.
So, I'm sorry but i'll ask you to get rid of most of the PR and rewrite it based on this :/ (or let me do it, but at this point I think you understood the end to end handler-usecase-repository setup, so I'm confident you should be able to do it in half a day, which is better than wait for me tomorrow evening at best)
(that being said, I still reviewed it properly and left you some comments on useful things to know on our backend, for future reference)
Finally, especially for a "boilerplate-heavy" backend PRs like this where more effort goes into writing usecases and repos and adapters than actual business logic, I'd very much appreciate a rapid loop of API/DB schema/modelization validation in a draft PR (which could be done in half a day at most, probably hours) rather than risk to "lose" a couple of days of working in the wrong direction.
| func handleIsSSOEnabled(uc usecases.Usecases) func(c *gin.Context) { | ||
| return func(c *gin.Context) { | ||
| ctx := c.Request.Context() | ||
| licenseKey := os.Getenv("LICENSE_KEY") |
There was a problem hiding this comment.
The way the endpoint works is broadly ok, but I'm trying to stick as much as possible to the paradigm of "all env variables are read in the entrypoint file for the code that is run" - here, typically cmd/server.go. Any useful pre-treatment is then done and the useful value is injected to the usecases from there. See cmd/server.go:100.
The idea being that I want to avoid a situation where we start reading env variables right and left, with business logic depending on it. (and we keep open the possibility to easily switch to a config file setup option, with just one file to change).
Further, and also check cmd/server.go:100 on this, unfortunately the license key is a bit of a special case that further justifies this. Our backend also doubles as the license server (I promise I'll change this some day and run a distinct container. But for now we have to bear with it). Anyway, TL:DR our own backend should not call itself for the license (or else, all kinds of chicken-and-egg problems), so it's just validating a whitelist of GCP project ids from the metadata server.
| @@ -1,7 +1,8 @@ | |||
| services: | |||
| db: | |||
| container_name: postgres | |||
| image: europe-west1-docker.pkg.dev/marble-infra/marble/postgresql-db:latest # custom image of postgres 15 with pg_cron extension added | |||
| #image: europe-west1-docker.pkg.dev/marble-infra/marble/postgresql-db:latest # custom image of postgres 15 with pg_cron extension added | |||
| image: postgres:15.2-alpine | |||
There was a problem hiding this comment.
just heads up to roll this back before merging
| return models.Feature{}, err | ||
| } | ||
|
|
||
| tracking.TrackEvent(ctx, models.AnalyticsFeatureCreated, map[string]interface{}{ |
There was a problem hiding this comment.
generally speaking, our analytics tracking is only useful for end user actions, not admin (marble internal) user actions
| FEATURE_READ | ||
| FEATURE_CREATE | ||
| FEATURE_UPDATE | ||
| FEATURE_DELETE |
There was a problem hiding this comment.
This deserves a comment in the code (sorry), but it's important that every added entry in this list has a matching string value below on the String() method.
There was a problem hiding this comment.
Also, more structurally, permissions need to be "attached" to a role to be actually "usable". See the models/role_permissions.go file. In this case, I suppose this would be for marble admins only.
| ); | ||
| CREATE UNIQUE INDEX entitlements_unique_organization_feature ON organization_entitlements (organization_id, feature_id) WHERE deleted_at IS NULL; | ||
|
|
||
| CREATE TYPE availability_kind AS ENUM ( |
There was a problem hiding this comment.
I'm conflicted about postgres enum types. They are nice to validate DB input format, and in rare cases (definitely not here) may bring some performance/storage benefits, but they are hard to evolve (you can't, ever, remove an enum value option). So all in all, I'm not fond of them.
There was a problem hiding this comment.
If absolutely needed CHECK constraints can bring some of the same functionality while being more versatile.
No description provided.