feat: Block access after seven days notice of API overage #3714
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Uffizzi Preview |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3714 +/- ##
========================================
Coverage 95.87% 95.88%
========================================
Files 1133 1134 +1
Lines 35873 36025 +152
========================================
+ Hits 34393 34541 +148
- Misses 1480 1484 +4 ☔ View full report in Codecov by Sentry. |
| @@ -189,7 +197,81 @@ def handle_api_usage_notifications(): | |||
| ) | |||
|
|
|||
|
|
|||
| def restrict_use_due_to_api_limit_grace_period_over() -> None: | |||
There was a problem hiding this comment.
I think we should perhaps have a flag to control this behaviour similar to the one that we added for the usage notifications task?
I wonder if the flag could also include a value that defines which fields to update on the organisation in case we want to start by just blocking access to admin for example, rather than immediately stopping serving flags.
There was a problem hiding this comment.
I think I understand what you're asking for here, but just to be clear:
Once this task has been run, but before the individual filtered organisations have been modified we would have to run something like this:
for organisation in organisations:
stop_serving_flags_enabled = flagsmith_client.get_identity_flags(
f"org.{organisation.id}.{organisation.name}",
traits={"organisation_id": organisation.id},
).is_feature_enabled("api_limiting_stop_serving_flags")
blog_access_to_admin_enabled = flagsmith_client.get_identity_flags(
f"org.{organisation.id}.{organisation.name}",
traits={"organisation_id": organisation.id},
).is_feature_enabled("api_limiting_block_access_to_admin")And then if either of the features are enabled, then the bulk update would include those files. Am I understanding this correctly?
There was a problem hiding this comment.
I was more thinking something along the lines of:
for organisation in organisations:
flags = flagsmith_client.get_identity_flags(...)
if flags.is_feature_enabled("api_limiting"):
fields_to_update = json.looads(flags.get_feature_value("api_limiting"))["fields_to_update"]
for field, value in fields_to_update.items():
setattr(organisation, field, value)
organisation.save()... but maybe that's a bit over complicated / error prone. Perhaps your solution is better (although we should at least make sure that we separate out the call to get the flags and call is_feature_enabled() on the resultant Flags object rather than calling get_identity_flags twice).
There was a problem hiding this comment.
I think the biggest deciding factor should be how onerous it will be to update the flags once we're ready in production. My gut is slightly in favour of using individual flags over the feature values because it seems simpler to set them to enabled for the identity flags, but that may just be an incorrect understanding of how much time it will take to individually set the fields to update in your method.
If you were in change of the Flagsmith flags for this change which approach would be more efficient?
There was a problem hiding this comment.
I think you're right tbh, individual flags are easier to manage.
There was a problem hiding this comment.
@zachaysan I think there's still work to be done here?
There was a problem hiding this comment.
Ok I've added the flagsmith flags and updated how the update logic works.
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingdocs/if required so people know about the feature!Changes
Building upon my previous PR which itself builds upon the use of API usage notification models this PR provides the following improvements:
stop_serving_flagsandblock_access_to_admin, toTruefor organisations that have received an API usage warning at least 7 days old. This only applies to free accounts as paid accounts are going to be automatically charged in a subsequent PR.How did you test this code?
Two large tests and a decent amount of time with a debugger.