Add API limits management command #1153

TaiWilkin · 2020-10-28T14:00:44Z

Overview

Creates a management command to:

Count API requests
Create/update ApiBlock records
Send notification emails about changes to ApiBlock records

Connects #1139

Demo

Testing Instructions

Checkout this branch
Run ./scripts/update
Set a contributor's ApiLimit.monthly_limit to 0 using the admin dashboard.
Run ./scripts/manage check_api_limits. Nothing should happen.
Set the contributor's ApiLimit.monthly_limit to 10.
Use ./scripts/manage shell_plus to create a RequestLog for the contributor, setting the date within the current month the response code in the 200s.
Run ./scripts/manage check_api_limits. Nothing should happen.
Add eight more RequestLogs for the contributor (as above), for a total of nine.
Run ./scripts/manage check_api_limits. You should see an email printed to the console. Check the ContributorNotifications for the user; the api_limit_warning_sent_on should equal the current date.
Run ./scripts/manage check_api_limits again. Nothing should happen.
Add one more RequestLog for the contributor.
Run ./scripts/manage check_api_limits. You should see an email printed to the console. Check the ContributorNotifications for the user; the api_limit_exceeded_sent_on should equal the current date.
There should be an active ApiBlock set for the contributor with an end date set to the end of the current month.
Run ./scripts/manage check_api_limits. Nothing should happen.
Set the grace limit for the ApiBlock to 12 and set active to False.
Run ./scripts/manage check_api_limits. Nothing should happen.
Create two more RequestLogs for the contributor.
Run ./scripts/manage check_api_limits. Nothing should happen. You should see an email printed to the console. Check the ContributorNotifications for the user; the api_grace_limit_exceeded_sent_on should equal the current date. The ApiBlock should be active again.
Run ./scripts/manage check_api_limits. Nothing should happen.
Run ./scripts/test. All tests should pass.

Checklist

fixup! commits have been squashed
CI passes after rebase
CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

jwalgran · 2020-10-28T18:14:11Z

Looking at this now.

jwalgran · 2020-10-28T18:16:10Z

Curious about this co-authorship with vagrant

Did this happen, perhaps, by running git commands inside the VM?

TaiWilkin · 2020-10-28T19:04:56Z

Curious about this co-authorship with vagrant
Did this happen, perhaps, by running git commands inside the VM?

Yes, I think so. I should be able to correct it by editing the commit when I rebase. (Learned something new - I didn't know that signing as vagrant was possible!)

jwalgran

Nice, clear implementation. I was able to successfully exercise this using real API calls from the Swagger page and see both notifications being sent when they should.

I added a question and made some relatively small suggestions

jwalgran · 2020-10-28T18:32:16Z

docker-compose.yml

+      - DJANGO_LOG_LEVEL=INFO
+      - AWS_PROFILE=${AWS_PROFILE:-open-apparel-registry}
+      - OAR_CLIENT_KEY=
+      - NOTIFICATION_EMAIL_TO=notification@example.com


Noting for the PR history that this appears to be only content change in this file. The rest is whitespace cleanup.

jwalgran · 2020-10-28T18:43:48Z

src/django/api/limits.py

+            ).values('contributor').annotate(request_count=Count('id'))
+    for c in contributor_logs:
+        contributor = Contributor.objects.get(id=c.get('contributor'))
+        with transaction.atomic():


Consider moving the logic inside this for loop into a helper function and decorating the helper function with @transaction.atomic. For each loop iteration we want all of our reads and writes to the database to be consistent. Future editors of the logic will not have to worry about if they need introduce or move a with transaction block.

jwalgran · 2020-10-28T18:46:21Z

src/django/api/limits.py

+    return (at_datetime.replace(
+            day=1, hour=0, minute=0, second=0, microsecond=0)
+            + relativedelta(months=1) - relativedelta(seconds=1)) \
+            .replace(tzinfo=timezone.utc)


I'm curious about this final updating of the tzinfo. Does adding and removing relativedeltas strip the timezone information resulting in our need to replace it?

jwalgran · 2020-10-28T18:58:04Z

src/django/api/limits.py

+                until = get_end_of_month(at_datetime)
+                with transaction.atomic():
+                    ApiBlock.objects.create(contributor=contributor,
+                                            until=until, active=False,


If we are creating a new ApiBlock because the contributor has exceeded their limit, as I understand it that we should be setting active=True since we want to block further request to the API until we reach the until date.

jwalgran · 2020-10-28T19:03:58Z

src/django/api/templates/mail/api_limit_admin_subject.txt

@@ -0,0 +1 @@
+Open Apparel Registry contributor API limit notice


Consider changing this admin-facing email title to include the contributor name so that mail clients don't "helpfully" stack up notifications for different contributors.

One possibility

OAR API limit exceeded - {{contributor_name}}

jwalgran · 2020-10-28T19:09:07Z

src/django/api/templates/mail/api_limit_body.html

+        </p>
+        {% if grace_limit %}
+        <p>
+            The grace limit previously set was {{ grace_limit }}.


While testing I realized that we are being more detailed about our limit calculations than we absolutely have to be.

Consider this simplified version that excludes the current count

{% if grace_limit %} <p> You're receiving this email because you have exceeded your limit of {{ grace_limit }} OAR API requests for this month. </p> {% else %} <p> You're receiving this email because you have exceeded your limit of {{ limit }} OAR API requests for this month. </p> {% endif %}

jwalgran · 2020-10-28T19:32:20Z

src/django/api/templates/mail/api_limit_body.txt

+You're receiving this email because you have used
+{{ count }} of your {{ limit }} requests for this month.
+
+{% if grace_limit %}
+    The grace limit previously set was {{ grace_limit }}.
+{% endif %}


While testing I realized that we are being more detailed about our limit calculations than we absolutely have to be.

Consider this simplified version that excludes the current count

{% if grace_limit %} You're receiving this email because you have exceeded your limit of {{ grace_limit }} OAR API requests for this month. {% else %} You're receiving this email because you have exceeded your limit of {{ limit }} OAR API requests for this month. {% endif %}

TaiWilkin · 2020-10-29T15:48:58Z

@jwalgran Sincere apologies for way too many fixups! I kept thinking I was done and realizing there were additional changes I should have made.
The functionality inside the loop has been moved to a separate, @transaction.atomic-decorated function. The email templates have been updated and simplified. The ApiBlock is being set as initially active (good catch!). Initially I was creating the at_datetime with a naive datetime object, and adding the timezone afterwards - once I had fixed that, I no longer needed to do the extra work of updating it ex post facto, but I missed removing that instance. It's been cleaned up now.

jwalgran · 2020-10-29T16:29:32Z

Looking at this now.

jwalgran · 2020-10-29T16:54:40Z

I made an incomplete suggestion when I proposed simplifying the emails. The current implementation uses the same email templates for the 90% warning and the limit exceeded notifications. As a result, in my manual testing, when my test user reached 90%, the email sent said "you have exceeded your limit"

The options for handling this that I can think of are:

Restore count to the context and add logic to the templates.
Add separate templates for "warning" and "limit exceeded" emails.

I lean slightly toward using different templates. In general, "dumb" templates are easier to maintain.

jwalgran

The changes look good and I successfully tested the creation of ApiBlocks and limit and grace_limit emails.

The only remaining issue that I can see is the one that I commented on previously. We need to distinguish between limit warning and limit exceeded emails.

TaiWilkin · 2020-10-29T17:32:19Z

@jwalgran I updated to a second set of templates. The text in them is, "You're receiving this email because you have used 90% of your limit of {{ limit }} OAR API requests for this month." and the subject says "Open Apparel Registry API limit warning." Does this text seem clear for users?

jwalgran

Confirmed the new templates. We will likely get updated text from the OAR team so the existing text is a good starting point. Thanks.

TaiWilkin requested a review from jwalgran October 28, 2020 14:01

TaiWilkin assigned jwalgran Oct 28, 2020

jwalgran reviewed Oct 28, 2020

View reviewed changes

jwalgran assigned TaiWilkin Oct 28, 2020

TaiWilkin requested a review from jwalgran October 29, 2020 15:49

jwalgran reviewed Oct 29, 2020

View reviewed changes

jwalgran approved these changes Oct 29, 2020

View reviewed changes

TaiWilkin force-pushed the tw/add-api-access-command branch from e3e2a47 to b4086ea Compare October 29, 2020 18:23

TaiWilkin added 5 commits October 29, 2020 14:23

Add ContributorNotifications Model

d16e229

Add api limit email templates

7c4db31

Add notification email address

23ea312

Add api limit management command

2c25692

Add api limit tests

91e66b0

TaiWilkin force-pushed the tw/add-api-access-command branch from b4086ea to 91e66b0 Compare October 29, 2020 18:23

TaiWilkin merged commit 331a169 into develop Oct 29, 2020

TaiWilkin deleted the tw/add-api-access-command branch October 29, 2020 18:28

rbreslow mentioned this pull request Nov 2, 2020

Set up scheduled invocation of check_api_limits #1165

Closed

5 tasks

rbreslow mentioned this pull request Dec 28, 2020

Scheduled check_api_limits invocation with Step Functions #1193

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API limits management command #1153

Add API limits management command #1153

TaiWilkin commented Oct 28, 2020

jwalgran commented Oct 28, 2020

jwalgran commented Oct 28, 2020

TaiWilkin commented Oct 28, 2020

jwalgran left a comment

jwalgran Oct 28, 2020

jwalgran Oct 28, 2020

jwalgran Oct 28, 2020

jwalgran Oct 28, 2020

jwalgran Oct 28, 2020

jwalgran Oct 28, 2020

jwalgran Oct 28, 2020

TaiWilkin commented Oct 29, 2020

jwalgran commented Oct 29, 2020

jwalgran commented Oct 29, 2020

jwalgran left a comment

TaiWilkin commented Oct 29, 2020

jwalgran left a comment

		@@ -0,0 +1 @@
		Open Apparel Registry contributor API limit notice

Add API limits management command #1153

Add API limits management command #1153

Conversation

TaiWilkin commented Oct 28, 2020

Overview

Demo

Testing Instructions

Checklist

jwalgran commented Oct 28, 2020

jwalgran commented Oct 28, 2020

TaiWilkin commented Oct 28, 2020

jwalgran left a comment

Choose a reason for hiding this comment

jwalgran Oct 28, 2020

Choose a reason for hiding this comment

jwalgran Oct 28, 2020

Choose a reason for hiding this comment

jwalgran Oct 28, 2020

Choose a reason for hiding this comment

jwalgran Oct 28, 2020

Choose a reason for hiding this comment

jwalgran Oct 28, 2020

Choose a reason for hiding this comment

jwalgran Oct 28, 2020

Choose a reason for hiding this comment

jwalgran Oct 28, 2020

Choose a reason for hiding this comment

TaiWilkin commented Oct 29, 2020

jwalgran commented Oct 29, 2020

jwalgran commented Oct 29, 2020

jwalgran left a comment

Choose a reason for hiding this comment

TaiWilkin commented Oct 29, 2020

jwalgran left a comment

Choose a reason for hiding this comment