fix(github-4555): use api_key name for changed_by #4561
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: api/edge_api/identities/serializers.py
Did you find this useful? React with a 👍 or 👎 |
Docker builds report
|
|
Uffizzi Preview |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4561 +/- ##
=======================================
Coverage 97.14% 97.14%
=======================================
Files 1154 1154
Lines 39811 39846 +35
=======================================
+ Hits 38674 38709 +35
Misses 1137 1137 ☔ View full report in Codecov by Sentry. |
gagantrivedi
force-pushed
the
fix/4555
branch
from
0797ff6 to
3b5561c
Compare
| mocker, identity, feature, admin_user | ||
| mocker, | ||
| identity, | ||
| feature, | ||
| user, | ||
| rf: RequestFactory, |
| @@ -189,7 +186,7 @@ def save(self, **kwargs): | |||
| "environment_api_key": identity.environment_api_key, | |||
| "identity_id": identity.id, | |||
| "identity_identifier": identity.identifier, | |||
| "changed_by_user_id": request.user.id, | |||
| "changed_by": str(request.user), | |||
There was a problem hiding this comment.
@gagantrivedi changing the signature here will be a problem during the deployment because the old task processor instances will not be able to execute the function with the updated arguments. We need to resolve this please in a new PR.
I suggest that we just add a new argument changed_by to the task function and update the calling code to only send changed_by_user_id if request.user is an FFAdminUser and then ignore that field in the actual task handler.
Note: we should probably consider adding some kind of versioning logic to the task processor to handle this in the future too.
| kwargs = { | ||
| "environment_api_key": self.environment_api_key, | ||
| "identifier": self.identifier, | ||
| "user_id": getattr(user, "id", None), |
There was a problem hiding this comment.
This will start to fail in the future if someone adds the id attribute to the APIKeyUser class. Will a test pick it up if so? Or should we be more explicit with this check? e.g.
"user_id": user.id if not getattr(user, "is_master_api_key_user", False) else None
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
Fix the issue with updating feature states for edge identities using the API key by setting the changed_by webhook field to the name of the key.
How did you test this code?
Updates unit tests