feat: Identity overrides in environment document #3766
Conversation
khvn26
requested review from
a team and
novakzaballa
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3766 +/- ##
=======================================
Coverage 95.99% 95.99%
=======================================
Files 1135 1138 +3
Lines 36204 36245 +41
=======================================
+ Hits 34754 34795 +41
Misses 1450 1450 ☔ View full report in Codecov by Sentry. |
khvn26
force-pushed
the
feat/environment-document-identity-overrides
branch
from
7a02f82 to
fe4eeec
Compare
khvn26
force-pushed
the
feat/environment-document-identity-overrides
branch
from
fe4eeec to
44131c9
Compare
khvn26
force-pushed
the
feat/environment-document-identity-overrides
branch
from
44131c9 to
e108ee3
Compare
| result = map_environment_to_sdk_document( | ||
| environment, | ||
| identities_with_overrides=identities_with_overrides, | ||
| ) |
There was a problem hiding this comment.
I'd love to see an assertion on the number of queries here (along with maybe multiple identities / identity overrides).
There was a problem hiding this comment.
This is a valid request but the assertion won't belong to the mapper test as it expects already fetched data with the identities_with_overrides kwarg. I'll add multiple overrides to the environment-document view test.
There was a problem hiding this comment.
Done, and fixed an N+1 issue in the process 😬
api/environments/models.py
Outdated
| "identity", | ||
| "identity__environment", | ||
| ).prefetch_related( | ||
| "identity__identity_traits", |
There was a problem hiding this comment.
I'm unsure why we would need traits here?
There was a problem hiding this comment.
I thought I had fair reasoning for that, but, upon reviewing, I tend to agree with your question/comment. Removed both trait prefetching and trait mapping to the environment document.
api/tests/unit/environments/test_unit_environments_views_sdk_environment.py
Outdated
Show resolved
Hide resolved
docs/docs/clients/overview.md
Outdated
| - In circumstances where you need to target a specific identity, you can do this by creating a segment to target that | ||
| specific user and subsequently adding a segment override for that segment. |
There was a problem hiding this comment.
If we're removing this, shouldn't we also remove this line: https://github.com/Flagsmith/flagsmith/pull/3766/files#diff-a4321d583bb04a398f14c6f6164fe85200d8687ac5c617f8e52b631376463809L349
... but should we also add some data about the Edge API opt-in process here?
There was a problem hiding this comment.
I've removed Ben's commit as we should really update the docs as part of FE work after #3881.
khvn26
force-pushed
the
feat/environment-document-identity-overrides
branch
from
bc5d07d to
4a84e84
Compare
khvn26
force-pushed
the
feat/environment-document-identity-overrides
branch
from
4a84e84 to
55f216d
Compare
- Add `map_environment_to_sdk_document` mapper - Add `Identity.objects.with_overrides()`
- Use INNER JOIN for identity overrides - Don't fetch integrations for the SDK endpoint
khvn26
force-pushed
the
feat/environment-document-identity-overrides
branch
from
ae30fd5 to
2899fd1
Compare
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingdocs/if required so people know about the feature!Changes
This PR adds identity overrides to the environment document response for local evaluation.
How did you test this code?
Added a unit test for the new mapper.
Contributes to #1762.