Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add endpoint to list (edge) identity overrides for a feature #3116

Merged

Conversation

matthewelwell
Copy link
Contributor

@matthewelwell matthewelwell commented Dec 8, 2023

Changes

Fixes #3105.

Uses the new flagsmith_environments_v2 (based on the structure defined here) to generate a view to show all the identity overrides for a particular feature.

Adds an endpoint at GET /api/v1/environments/:environment_id/edge-identity-overrides[?feature=:feature_id] which returns data in the following structure:

{
  "results": [
    {
      "identity_uuid": "59efa2a7-6a45-46d6-b953-a7073a90eacf",
      "identifier": "identity1"
      "feature_state": {
        "feature_state_value": "foo",
        "feature": 1,
        "multivariate_feature_state_values": [],
        "enabled": true,
        "featurestate_uuid": "a7495917-ee57-41d1-a64e-e0697dbc57fb",
      }
    }
  ]
}

How did you test this code?

Added tests to the codebase where necessary. Also tested the dynamo integrations manually to confirm the data is returned as expected.

I've also run my local API against staging dynamo and inserted data into the flagsmith_environments_v2 table and verified that the endpoint picks up the data as expected.

Copy link

vercel bot commented Dec 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 11, 2023 10:30am
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 11, 2023 10:30am
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 11, 2023 10:30am

Copy link
Contributor

github-actions bot commented Dec 8, 2023

Uffizzi Preview deployment-42416 was deleted.

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2a9cd7c) 95.95% compared to head (7a044be) 96.08%.
Report is 1 commits behind head on main.

Files Patch % Lines
api/edge_api/identities/permissions.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3116      +/-   ##
==========================================
+ Coverage   95.95%   96.08%   +0.13%     
==========================================
  Files        1053     1054       +1     
  Lines       31588    31693     +105     
==========================================
+ Hits        30311    30453     +142     
+ Misses       1277     1240      -37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matthewelwell matthewelwell marked this pull request as ready for review December 8, 2023 17:00
@matthewelwell matthewelwell marked this pull request as draft December 8, 2023 17:00
Comment on lines +81 to +86
identity: EdgeIdentity = self.parent.context["identity"]
environment: Environment = self.parent.context["environment"]
identity_id = identity.get_hash_key(
environment.use_identity_composite_key_for_hashing
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically out of the scope of this PR, but this was an issue in the current implementation of the views which retrieve the feature states for a given identity.

The previous logic, although wrong, wouldn't have actually given any incorrect data (at least for users that are creating identity overrides via the dashboard, not via the API) since it only would have affected multivariate features with more than one weighted option, which we don't allow for identity overrides (you have to select a single option, which will be given 100% weighting).

Since I had to also implement this functionality for the new endpoint, however, it made sense to fix it in this PR.

@@ -142,4 +143,9 @@
create_segment_override,
name="create-segment-override",
),
path(
"<int:environment_pk>/edge-identity-overrides",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether to use the pk here or the API key. I definitely prefer using the pk, and would like to move to using the pk everywhere in the future so I figured this would at least not add further tech debt. It does, however, cause some consistency issues.

@matthewelwell matthewelwell marked this pull request as ready for review December 8, 2023 17:22
@@ -183,6 +183,7 @@ def save(self, user: FFAdminUser = None, master_api_key: MasterAPIKey = None):
"environment_api_key": self.environment_api_key,
"changes": changes,
"identity_uuid": str(self.identity_uuid),
"identifier": self.identifier,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khvn26, I had to add the identifier here as I need it in the response to the FE. I believe we'll also need it to power identity override in local evaluation too so I think it's data worth having.

@matthewelwell matthewelwell added this pull request to the merge queue Dec 11, 2023
Merged via the queue into main with commit 098ab94 Dec 11, 2023
@matthewelwell matthewelwell deleted the feat/implement-get-identity-overrides-for-a-feature-in-edge branch December 11, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issue related to the REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display per-feature identity overrides from the flagsmith_environments_v2 DynamoDB table
4 participants