-
Notifications
You must be signed in to change notification settings - Fork 429
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: Block access after seven days notice of API overage #3714
Merged
matthewelwell
merged 11 commits into
main
from
feat/block_access_after_seven_days_notice
May 7, 2024
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
e3402e7
Add APILimitAccessBlock and create after save hook on plan change
zachaysan 96eda1b
Add grace period
zachaysan 6bf270a
Add restrict and unrestrict API limit breached tasks
zachaysan 5464273
Add tests for restrict and unrestrict API usage limit breaches
zachaysan 992631c
Merge branch 'main' into feat/block_access_after_seven_days_notice
zachaysan 17f7972
Add docstrings and update variable names
zachaysan 7787b95
Add api_limit_access_block checks for test
zachaysan b73f4ae
Merge branch 'main' into feat/block_access_after_seven_days_notice
zachaysan 87e13b9
Add mocks for flagsmith client
zachaysan 9d28b82
Enable testing for flags and switch update logic
zachaysan 245cc37
Merge branch 'main' into feat/block_access_after_seven_days_notice
zachaysan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
23 changes: 23 additions & 0 deletions
23
api/organisations/migrations/0053_create_api_limit_access_block.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Generated by Django 3.2.25 on 2024-04-03 14:11 | ||
|
||
from django.db import migrations, models | ||
import django.db.models.deletion | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('organisations', '0052_create_hubspot_organisation'), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name='APILimitAccessBlock', | ||
fields=[ | ||
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
('created_at', models.DateTimeField(auto_now_add=True, null=True)), | ||
('updated_at', models.DateTimeField(auto_now=True, null=True)), | ||
('organisation', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='api_limit_access_block', to='organisations.organisation')), | ||
], | ||
), | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more thinking something along the lines of:
... 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 callingget_identity_flags
twice).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right tbh, individual flags are easier to manage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zachaysan I think there's still work to be done here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've added the flagsmith flags and updated how the update logic works.