feat: Add segment types, segment metadata, identity override segments support #198
Conversation
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against de8a1ba |
| @@ -20,3 +20,5 @@ | |||
| IS_SET: ConditionOperator = "IS_SET" | |||
| IS_NOT_SET: ConditionOperator = "IS_NOT_SET" | |||
| IN: ConditionOperator = "IN" | |||
|
|
|||
| SEGMENT_IDENTIFIER_PROPERTY_NAME: str = "_$identity.identifier" | |||
There was a problem hiding this comment.
I kind of feel like we could generalise this to just the prefix? So e.g. _$identity.identity_uuid would also work? Maybe that's a bit YAGNI?
There was a problem hiding this comment.
_$identity.identity_uuid would also work?
In the future, maybe we'll want it to. Maybe we'll have other entities participating in the matching, e.g. $_contexts.org.location.
| @@ -39,3 +39,4 @@ class SegmentModel(BaseModel): | |||
| name: str | |||
| rules: typing.List[SegmentRuleModel] = Field(default_factory=list) | |||
| feature_states: typing.List[FeatureStateModel] = Field(default_factory=list) | |||
| meta: typing.Optional[typing.Dict[str, str]] = None | |||
There was a problem hiding this comment.
I wonder if this should be at least a typed dict? Maybe one for a little further down the line when we know exactly what we need in the meta dict?
There was a problem hiding this comment.
There's two reasons I don't want to impose a typed dict here yet:
- The metadata schema is not settled yet.
- The engine does not need this metadata, APIs do. It's on Core/Edge to maintain the schema (could be a common library, just not the engine.)
tests/unit/segments/fixtures.py
Outdated
| @@ -148,3 +149,23 @@ | |||
| ) | |||
| ], | |||
| ) | |||
| segment_identity_override = SegmentModel( | |||
| id=7, | |||
| name="segment_identity_override", | |||
There was a problem hiding this comment.
We talked about this being unique in someway, right? Should we illustrate that even in this test data?
tests/unit/segments/fixtures.py
Outdated
| meta={ | ||
| "identity_identifier": identifier, | ||
| "type": "IDENTITY_OVERRIDE", | ||
| }, |
There was a problem hiding this comment.
Do we want to add identity_uuid here too?
There was a problem hiding this comment.
@khvn26 I think this PR is still valuable but probably needs some reworking to fit in with the changes we made to the data structures?
Closes #197.
This PR adds the following:
meta: dict[str, str] | Nonefield to the Segment model. It's intended to be used by Core and Edge API to store data specific for their Flagsmith implementations, unrelated to the engine and SDKs."_$identity.identifier"string literal can be now assigned toSegmentCondition.property_to make the engine match segment condition rules against the identity identifier.