-
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: Create api usage function #3340
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3340 +/- ##
==========================================
- Coverage 95.93% 95.91% -0.02%
==========================================
Files 1088 1090 +2
Lines 33818 33932 +114
==========================================
+ Hits 32443 32547 +104
- Misses 1375 1385 +10 ☔ View full report in Codecov by Sentry. |
Uffizzi Ephemeral Environment
|
As discussed, I included it on the existing note in the page. |
api/organisations/templates/organisations/api_usage_notification_limit.html
Show resolved
Hide resolved
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've approved this as the comments I've left are only minor improvements that aren't strictly necessary.
api/organisations/tasks.py
Outdated
if matched_threshold < 100: | ||
message = "organisations/api_usage_notification.txt" | ||
html_message = "organisations/api_usage_notification.html" | ||
|
||
# Since threshold < 100 only include admins. | ||
recipient_list = list( | ||
FFAdminUser.objects.filter( | ||
userorganisation__organisation=organisation, | ||
userorganisation__role=OrganisationRole.ADMIN, | ||
).values_list("email", flat=True) | ||
) | ||
else: | ||
message = "organisations/api_usage_notification_limit.txt" | ||
html_message = "organisations/api_usage_notification_limit.html" | ||
|
||
# Since threshold >= 100 include everyone in the organisation. | ||
recipient_list = list( | ||
FFAdminUser.objects.filter( | ||
userorganisation__organisation=organisation, | ||
).values_list("email", flat=True) | ||
) |
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'd probably have done:
if matched_threshold < 100: | |
message = "organisations/api_usage_notification.txt" | |
html_message = "organisations/api_usage_notification.html" | |
# Since threshold < 100 only include admins. | |
recipient_list = list( | |
FFAdminUser.objects.filter( | |
userorganisation__organisation=organisation, | |
userorganisation__role=OrganisationRole.ADMIN, | |
).values_list("email", flat=True) | |
) | |
else: | |
message = "organisations/api_usage_notification_limit.txt" | |
html_message = "organisations/api_usage_notification_limit.html" | |
# Since threshold >= 100 include everyone in the organisation. | |
recipient_list = list( | |
FFAdminUser.objects.filter( | |
userorganisation__organisation=organisation, | |
).values_list("email", flat=True) | |
) | |
recipient_list = FFAdminUser.objects.filter( | |
userorganisation__organisation=organisation, | |
) | |
if matched_threshold < 100: | |
message = "organisations/api_usage_notification.txt" | |
html_message = "organisations/api_usage_notification.html" | |
# Since threshold < 100 only include admins. | |
recipient_list = recipient_list.filter( | |
userorganisation__role=OrganisationRole.ADMIN, | |
) | |
else: | |
message = "organisations/api_usage_notification_limit.txt" | |
html_message = "organisations/api_usage_notification_limit.html" |
... and then in the send_mail call you can convert to a flat list of emails:
send_mail(
...
recipient_list=list(recipient_list.values_list("email", flat=True))
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 that's no problem to update.
# Now re-run the usage to make sure the notification isn't resent. | ||
handle_api_usage_notifications() | ||
|
||
assert ( | ||
OranisationAPIUsageNotification.objects.filter( | ||
organisation=organisation, | ||
).count() | ||
== 1 | ||
) | ||
assert OranisationAPIUsageNotification.objects.first() == api_usage_notification |
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.
A neat addition here might be to use the mailoutbox
fixture available in pytest-django.
https://pytest-django.readthedocs.io/en/latest/helpers.html#mailoutbox
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.
What a cool fixture! I've updated the code to use that instead of the email send mocks.
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingI have added information todocs/
if required so people know about the feature!Changes
This PR enables our application to respond to organisations which are using the API close to or above our limits. A few considerations were necessary in order to get all the moving pieces together. First, additional information from Chargebee was required to see what part of the billing cycle the customer is at at the time of comparing their API use. This is then combined with the realtime data we store in Influxdb. And then code that is capable of treating yearly plans as smoothly as monthly ones is in place with relative date counting. Finally an additional model was created in order to keep track of sending notifications only once for a given threshold in a given month.
How did you test this code?
Influxdb related code was manually tested with ENV vars set to point to our development influxdb instance. In addition to that one large test is available to validate the rest of the work.