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
|
zachaysan
changed the title
github-actions
bot
added
the
front-end
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
matthewelwell
left a comment
matthewelwell
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
What a cool fixture! I've updated the code to use that instead of the email send mocks.
3 participants
Thanks for submitting a PR! Please check the boxes below:
pre-committo 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.