Add support for using featureKey with entitlements #1065
Conversation
chore: support fetching by key (cherry picked from commit 8964555c73ba3450df5f2d0732748f4a5d99d9a6)
GAlexIHU
added
kind/feature
| return Entitlement{}, &productcatalog.FeatureNotFoundError{ID: input.FeatureID} | ||
| func (c *entitlementConnector) CreateEntitlement(ctx context.Context, input CreateEntitlementInputs) (*Entitlement, error) { | ||
| // ID has precedence over key | ||
| idOrKey := input.FeatureID |
There was a problem hiding this comment.
I think the inputs should already get a single FeatureIDorKey field, and this should be handled in the httpdriver layer.
This would also align the PR with the GetEntitlementValue's signature
| @@ -30,6 +30,7 @@ func (Entitlement) Fields() []ent.Field { | |||
| field.String("feature_id").Immutable().SchemaType(map[string]string{ | |||
| dialect.Postgres: "char(26)", | |||
| }), | |||
| field.String("feature_key").Immutable(), | |||
There was a problem hiding this comment.
I don't think it's worth storing it here. I would rather prefer that the connector resolves the ID and we persits that data.
I wouldn't want to allow for a situation where a feature gets archived, then a new feature is created with the same key. Let's make sure that inside the DB we are not representing this convenience feature.
There was a problem hiding this comment.
So actually the problem has deeper roots than implementation details:
- If we want to allow entitlements to be accessible by featureKey, then it's necessary to limit that a user can only be entitled to a given featureKey once. Otherwise we cannot return a simple value.
- If we limit that then after archiving a feature the user can only be entitled again if the previous is deleted.
The issue is that this is not yet enforced (which i'll add) but after that these archivation scenarios are resolved. (this has some implications on the checks we do at create-time but nothing too complex)
| func (c *featureConnector) GetFeature(ctx context.Context, featureID models.NamespacedID) (Feature, error) { | ||
| feature, err := c.featureRepo.GetByID(ctx, featureID) | ||
| func (c *featureConnector) GetFeature(ctx context.Context, namespace string, idOrKey string) (*Feature, error) { | ||
| feature, err := c.featureRepo.GetByIdOrKey(ctx, namespace, idOrKey, true) |
There was a problem hiding this comment.
Please add an options struct at least for the includeArchived as true/false const arguments are extremely hard to understand what they are representing. (or you can add a type IncludeArchive bool so that the next one reading this code won't have to look up the function signature) ;)
There was a problem hiding this comment.
I agree 4 arguments is starting to get on the longer side, but signature wise a bool is no different than a string and we don't do custom types for that.
GAlexIHU
changed the title
|
implemented in another PR |
Overview
Fixes #(issue)
Notes for reviewer