From dd817358cbddf3f1814113a68f1bea582a5daf56 Mon Sep 17 00:00:00 2001 From: Helen Bailey Date: Tue, 26 Nov 2019 16:24:46 -0500 Subject: [PATCH] =?UTF-8?q?Replace=20ElementsAPICall=20model=20with=20a=20?= =?UTF-8?q?celery=20task=20to=20patch=20update=20an=20E=E2=80=A6=20(#173)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace ElementsAPICall model with a celery task to patch update an Elements record. Delete elements models and make database migration. Update tests. --- Pipfile | 3 + Pipfile.lock | 121 ++++++ solenoid/__init__.py | 3 + solenoid/celery.py | 14 + solenoid/elements/admin.py | 50 --- solenoid/elements/errors.py | 10 + solenoid/elements/helpers.py | 24 ++ .../commands/delete_stale_api_calls.py | 21 - .../management/commands/issue_unsent_calls.py | 26 -- .../migrations/0003_delete_elementsapicall.py | 16 + solenoid/elements/models.py | 168 -------- solenoid/elements/tasks.py | 35 ++ solenoid/elements/tests.py | 379 ++++-------------- solenoid/elements/views.py | 80 +--- solenoid/settings/base.py | 7 + 15 files changed, 318 insertions(+), 639 deletions(-) create mode 100644 solenoid/celery.py delete mode 100644 solenoid/elements/admin.py create mode 100644 solenoid/elements/errors.py create mode 100644 solenoid/elements/helpers.py delete mode 100644 solenoid/elements/management/commands/delete_stale_api_calls.py delete mode 100644 solenoid/elements/management/commands/issue_unsent_calls.py create mode 100644 solenoid/elements/migrations/0003_delete_elementsapicall.py delete mode 100644 solenoid/elements/models.py create mode 100644 solenoid/elements/tasks.py diff --git a/Pipfile b/Pipfile index 58d11b6..679e229 100644 --- a/Pipfile +++ b/Pipfile @@ -9,6 +9,7 @@ python_version = "3.7" [dev-packages] coverage = ">=4.5.1" freezegun = "*" +requests-mock = "*" [packages] # Only needed for Heroku @@ -32,3 +33,5 @@ newrelic = "*" whitenoise = "*" Pillow = "*" sentry-sdk = "*" +celery = "*" +redis = "*" diff --git a/Pipfile.lock b/Pipfile.lock index 5ab1c79..fedba21 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -1,7 +1,11 @@ { "_meta": { "hash": { +<<<<<<< HEAD "sha256": "53406b373de4356533de017bd1d05a6170538ada1360b52cc632b9888c2aa649" +======= + "sha256": "59c3cbea877917a5bac2566032ad8430571da8ec66ea7790612ff38aaf548624" +>>>>>>> Replace ElementsAPICall model with a celery task to patch update an E… (#173) }, "pipfile-spec": 6, "requires": { @@ -16,12 +20,21 @@ ] }, "default": { +<<<<<<< HEAD "asgiref": { "hashes": [ "sha256:7e06d934a7718bf3975acbf87780ba678957b87c7adc056f13b6215d610695a0", "sha256:ea448f92fc35a0ef4b1508f53a04c4670255a3f33d22a81c8fc9c872036adbe5" ], "version": "==3.2.3" +======= + "amqp": { + "hashes": [ + "sha256:6e649ca13a7df3faacdc8bbb280aa9a6602d22fd9d545336077e573a1f4ff3b8", + "sha256:77f1aef9410698d20eaeac5b73a87817365f457a507d82edf292e12cbb83b08d" + ], + "version": "==2.5.2" +>>>>>>> Replace ElementsAPICall model with a celery task to patch update an E… (#173) }, "beautifulsoup4": { "hashes": [ @@ -32,6 +45,21 @@ "index": "pypi", "version": "==4.8.2" }, + "billiard": { + "hashes": [ + "sha256:01afcb4e7c4fd6480940cfbd4d9edc19d7a7509d6ada533984d0d0f49901ec82", + "sha256:b8809c74f648dfe69b973c8e660bcec00603758c9db8ba89d7719f88d5f01f26" + ], + "version": "==3.6.1.0" + }, + "celery": { + "hashes": [ + "sha256:4c4532aa683f170f40bd76f928b70bc06ff171a959e06e71bf35f2f9d6031ef9", + "sha256:528e56767ae7e43a16cfef24ee1062491f5754368d38fcfffa861cdb9ef219be" + ], + "index": "pypi", + "version": "==4.3.0" + }, "certifi": { "hashes": [ "sha256:017c25db2a153ce562900032d5bc68e9f191e44e9a0f762f373977de9df1fbb3", @@ -141,6 +169,20 @@ ], "version": "==2.8" }, + "importlib-metadata": { + "hashes": [ + "sha256:aa18d7378b00b40847790e7c27e11673d7fed219354109d0e7b9e5b25dc3ad26", + "sha256:d5f18a79777f3aa179c145737780282e27b508fc8fd688cb17c7a813e8bd39af" + ], + "version": "==0.23" + }, + "kombu": { + "hashes": [ + "sha256:1760b54b1d15a547c9a26d3598a1c8cdaf2436386ac1f5561934bc8a3cbbbd86", + "sha256:e7465aa85a1db889116819f08c5de29520d2fa103324dcdca5e90af345f01771" + ], + "version": "==4.6.6" + }, "libsass": { "hashes": [ "sha256:003a65b4facb4c5dbace53fb0f70f61c5aae056a04b4d112a198c3c9674b31f2", @@ -162,6 +204,13 @@ ], "version": "==0.19.4" }, + "more-itertools": { + "hashes": [ + "sha256:409cd48d4db7052af495b09dec721011634af3753ae1ef92d2b32f73a745f832", + "sha256:92b8c4b06dac4f0611c0729b2f2ede52b2e1bac1ab48f089c7ddc12e26bb60c4" + ], + "version": "==7.2.0" + }, "newrelic": { "hashes": [ "sha256:0e651f2ff48dd1fc538fc1297892cf726d1ad4fc0b2578aae6a47f10f16afb2c" @@ -268,6 +317,14 @@ ], "version": "==1.0.6" }, + "redis": { + "hashes": [ + "sha256:3613daad9ce5951e426f460deddd5caf469e08a3af633e9578fc77d362becf62", + "sha256:8d0fc278d3f5e1249967cba2eb4a5632d19e45ce5c09442b8422d15ee2c22cc2" + ], + "index": "pypi", + "version": "==3.3.11" + }, "requests": { "hashes": [ "sha256:11e007a8a2aa0323f5a921e9e6a2d7e4e67d9877e85773fba9ba6419025cbeb4", @@ -355,16 +412,50 @@ ], "version": "==1.25.7" }, + "vine": { + "hashes": [ + "sha256:133ee6d7a9016f177ddeaf191c1f58421a1dcc6ee9a42c58b34bed40e1d2cd87", + "sha256:ea4947cc56d1fd6f2095c8d543ee25dad966f78692528e68b4fada11ba3f98af" + ], + "version": "==1.3.0" + }, "whitenoise": { "hashes": [ "sha256:0f9137f74bd95fa54329ace88d8dc695fbe895369a632e35f7a136e003e41d73", "sha256:62556265ec1011bd87113fb81b7516f52688887b7a010ee899ff1fd18fd22700" ], "index": "pypi", +<<<<<<< HEAD "version": "==5.0.1" +======= + "version": "==4.1.4" + }, + "zipp": { + "hashes": [ + "sha256:3718b1cbcd963c7d4c5511a8240812904164b7f381b647143a89d3b98f9bcd8e", + "sha256:f06903e9f1f43b12d371004b4ac7b06ab39a44adc747266928ae6debfa7b3335" + ], + "version": "==0.6.0" +>>>>>>> Replace ElementsAPICall model with a celery task to patch update an E… (#173) } }, "develop": { + "certifi": { + "hashes": [ + "sha256:e4f3620cfea4f83eedc95b24abd9cd56f3c4b146dd0177e83a21b4eb49e21e50", + "sha256:fd7c7c74727ddcf00e9acd26bba8da604ffec95bf1c2144e67aff7a8b50e6cef" + ], + "index": "pypi", + "version": "==2019.9.11" + }, + "chardet": { + "hashes": [ + "sha256:84ab92ed1c4d4f16916e05906b6b75a6c0fb5db821cc65e70cbd64a3e2a5eaae", + "sha256:fc323ffcaeaed0e0a02bf4d117757b98aed530d9ed4531e3e15460124c106691" + ], + "index": "pypi", + "version": "==3.0.4" + }, "coverage": { "hashes": [ "sha256:189aac76d6e0d7af15572c51892e7326ee451c076c5a50a9d266406cd6c49708", @@ -410,6 +501,13 @@ "index": "pypi", "version": "==0.3.12" }, + "idna": { + "hashes": [ + "sha256:c357b3f628cf53ae2c4c05627ecc484553142ca23264e593d327bcde5e9c3407", + "sha256:ea8b7f6188e6fa117537c3df7da9fc686d485087abf6ac197f9c46432f7e4a3c" + ], + "version": "==2.8" + }, "python-dateutil": { "hashes": [ "sha256:73ebfe9dbf22e832286dafa60473e4cd239f8592f699aa5adaf10050e6e1823c", @@ -417,12 +515,35 @@ ], "version": "==2.8.1" }, + "requests": { + "hashes": [ + "sha256:11e007a8a2aa0323f5a921e9e6a2d7e4e67d9877e85773fba9ba6419025cbeb4", + "sha256:9cf5292fcd0f598c671cfc1e0d7d1a7f13bb8085e9a590f48c010551dc6c4b31" + ], + "index": "pypi", + "version": "==2.22.0" + }, + "requests-mock": { + "hashes": [ + "sha256:510df890afe08d36eca5bb16b4aa6308a6f85e3159ad3013bac8b9de7bd5a010", + "sha256:88d3402dd8b3c69a9e4f9d3a73ad11b15920c6efd36bc27bf1f701cf4a8e4646" + ], + "index": "pypi", + "version": "==1.7.0" + }, "six": { "hashes": [ "sha256:1f1b7d42e254082a9db6279deae68afb421ceba6158efa6131de7b3003ee93fd", "sha256:30f610279e8b2578cab6db20741130331735c781b56053c59c4076da27f06b66" ], "version": "==1.13.0" + }, + "urllib3": { + "hashes": [ + "sha256:a8a318824cc77d1fd4b2bec2ded92646630d7fe8619497b142c84a9e6f5a7293", + "sha256:f3c5fd51747d450d4dcf6f923c81f78f811aab8205fda64b0aba34a4e48b0745" + ], + "version": "==1.25.7" } } } diff --git a/solenoid/__init__.py b/solenoid/__init__.py index e69de29..fb989c4 100644 --- a/solenoid/__init__.py +++ b/solenoid/__init__.py @@ -0,0 +1,3 @@ +from .celery import app as celery_app + +__all__ = ('celery_app',) diff --git a/solenoid/celery.py b/solenoid/celery.py new file mode 100644 index 0000000..943d8b9 --- /dev/null +++ b/solenoid/celery.py @@ -0,0 +1,14 @@ +import os + +from celery import Celery + +os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'solenoid.settings.base') + +app = Celery('solenoid') +app.config_from_object('django.conf:settings', namespace='CELERY') +app.autodiscover_tasks() + + +@app.task(bind=True) +def debug_task(self): + print('Request: {0!r}'.format(self.request)) diff --git a/solenoid/elements/admin.py b/solenoid/elements/admin.py deleted file mode 100644 index e9f23ed..0000000 --- a/solenoid/elements/admin.py +++ /dev/null @@ -1,50 +0,0 @@ -from django.contrib import admin - -from .models import ElementsAPICall - - -class ReturnedListFilter(admin.SimpleListFilter): - title = 'Response was returned' - parameter_name = 'was_returned' - - def lookups(self, request, model_admin): - return ( - ('true', 'true'), - ('false', 'false'), - ) - - def queryset(self, request, queryset): - if self.value() == 'true': - return queryset.filter(response_content__isnull=False) - elif self.value() == 'false': - return queryset.filter(response_content__isnull=True) - else: - return queryset - - -class RetryListFilter(admin.SimpleListFilter): - title = 'Retry of other call' - parameter_name = 'is_retry' - - def lookups(self, request, model_admin): - return ( - ('true', 'true'), - ('false', 'false'), - ) - - def queryset(self, request, queryset): - if self.value() == 'true': - return queryset.filter(retry_of__isnull=False) - elif self.value() == 'false': - return queryset.filter(retry_of__isnull=True) - else: - return queryset - - -class ElementsAPICallAdmin(admin.ModelAdmin): - list_filter = (ReturnedListFilter, RetryListFilter, 'response_status') - list_display = ('pk', 'request_url', 'response_status', 'retry_of', - 'timestamp') - - -admin.site.register(ElementsAPICall, ElementsAPICallAdmin) diff --git a/solenoid/elements/errors.py b/solenoid/elements/errors.py new file mode 100644 index 0000000..fc44719 --- /dev/null +++ b/solenoid/elements/errors.py @@ -0,0 +1,10 @@ +class Error(Exception): + """Base class for exceptions in this module.""" + pass + + +class RetryError(Error): + """Exception raised for HTTP status codes that indicate a Symplectic + Elements API call should be retried (409, 500, 504). + """ + pass diff --git a/solenoid/elements/helpers.py b/solenoid/elements/helpers.py new file mode 100644 index 0000000..77d9cb1 --- /dev/null +++ b/solenoid/elements/helpers.py @@ -0,0 +1,24 @@ +from xml.etree.ElementTree import Element, SubElement + +from django.utils import timezone + + +def make_xml(username): + top = Element('update-object') + top.set('xmlns', 'http://www.symplectic.co.uk/publications/api') + oa_field = SubElement(top, 'oa') + + # Update library status field + status_field = SubElement(oa_field, 'library-status') + status_field.set('status', 'full-text-requested') + date_field = SubElement(status_field, 'last-requested-when') + date_field.text = timezone.now().isoformat() + note_field = SubElement(status_field, 'note-field') + note_field.set('clear-existing-note', 'true') + note = SubElement(note_field, 'note') + note.text = "Library status changed to Full text requested on " \ + "{date} by {username}.".format( + date=timezone.now().strftime('%-d %B %Y'), + username=username) + + return top diff --git a/solenoid/elements/management/commands/delete_stale_api_calls.py b/solenoid/elements/management/commands/delete_stale_api_calls.py deleted file mode 100644 index 18df6fa..0000000 --- a/solenoid/elements/management/commands/delete_stale_api_calls.py +++ /dev/null @@ -1,21 +0,0 @@ -from datetime import timedelta -import logging - -from django.core.management.base import BaseCommand -from django.utils import timezone - -from solenoid.elements.models import ElementsAPICall - - -logger = logging.getLogger(__name__) - - -class Command(BaseCommand): - help = ('Deletes successfully sent ElementsAPICall records from the ' - 'database if they are more than 4 weeks old') - - def handle(self, *args, **options): - last_month = timezone.now() - timedelta(weeks=4) - ElementsAPICall.objects.filter( - response_status='200' - ).filter(timestamp__lt=last_month).delete() diff --git a/solenoid/elements/management/commands/issue_unsent_calls.py b/solenoid/elements/management/commands/issue_unsent_calls.py deleted file mode 100644 index cd3ece7..0000000 --- a/solenoid/elements/management/commands/issue_unsent_calls.py +++ /dev/null @@ -1,26 +0,0 @@ -import logging - -from django.conf import settings -from django.core.management.base import BaseCommand - -from solenoid.elements.models import ElementsAPICall -from solenoid.elements.views import issue_elements_api_call - - -logger = logging.getLogger(__name__) - - -class Command(BaseCommand): - help = 'Issues all unsent ElementsAPICalls' - - def handle(self, *args, **options): - if not settings.USE_ELEMENTS: - logger.info('Not running issue_unsent_calls command because ' - 'settings.USE_ELEMENTS is False.') - return - - qs1 = ElementsAPICall.objects.filter(response_status='') - qs2 = ElementsAPICall.objects.filter(response_status__isnull=True) - - for call in (qs1 | qs2): - issue_elements_api_call(call) diff --git a/solenoid/elements/migrations/0003_delete_elementsapicall.py b/solenoid/elements/migrations/0003_delete_elementsapicall.py new file mode 100644 index 0000000..4974344 --- /dev/null +++ b/solenoid/elements/migrations/0003_delete_elementsapicall.py @@ -0,0 +1,16 @@ +# Generated by Django 2.2.7 on 2019-11-21 19:56 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('elements', '0002_auto_20180308_1812'), + ] + + operations = [ + migrations.DeleteModel( + name='ElementsAPICall', + ), + ] diff --git a/solenoid/elements/models.py b/solenoid/elements/models.py deleted file mode 100644 index 49e474b..0000000 --- a/solenoid/elements/models.py +++ /dev/null @@ -1,168 +0,0 @@ -import logging -import requests -import time -from xml.etree.ElementTree import Element, SubElement - -from django.conf import settings -from django.db import models -from django.utils import timezone - -logger = logging.getLogger(__name__) -HEADERS = {'Content-Type': 'text/xml'} -PROXIES = { - 'http': settings.QUOTAGUARD_URL, - 'https': settings.QUOTAGUARD_URL, -} - - -class ElementsAPICall(models.Model): - - """Stores records of requests made to the Elements API and responses - received. This will allow us to retry failed calls and monitor for - problems with the integration.""" - - request_data = models.TextField( - help_text='The xml sent (i.e. the "data" kwarg in the requests.patch()' - ' call.') - request_url = models.URLField( - help_text='The URL to which the call was sent (i.e. the "url" argument' - ' to requests.patch()).') - response_content = models.TextField( - blank=True, - null=True, - help_text='The content of the response. Will be blank if there was no' - 'response (i.e. due to timeout or other failed call).') - response_status = models.CharField( - max_length=3, - blank=True, - null=True, - help_text='The HTTP status code of the response. Will be blank if' - 'there was no response.') - timestamp = models.DateTimeField(auto_now_add=True) - retry_of = models.ForeignKey( - 'self', - blank=True, - null=True, - on_delete=models.CASCADE, - help_text='If this call is a retry of a previous failed call, this is' - 'a ForeignKey to that call. Otherwise it is blank.') - - # For the errors that may be returned by the Elements API, see - # https://support.symplectic.co.uk/support/solutions/articles/6000170776-api-v5-5-requests-and-responses . - STATUS_FAIL = [400, 401, 403, 404, 410] - STATUS_RETRY = [409, 500, 504] - - def __str__(self): - return "API call #{self.pk} ({self.timestamp})".format(self=self) - - @classmethod - def make_xml(cls, username): - logger.info('Making XML') - - top = Element('update-object') - top.set('xmlns', 'http://www.symplectic.co.uk/publications/api') - oa_field = SubElement(top, 'oa') - - # Update library status field - status_field = SubElement(oa_field, 'library-status') - status_field.set('status', 'full-text-requested') - date_field = SubElement(status_field, 'last-requested-when') - date_field.text = timezone.now().isoformat() - note_field = SubElement(status_field, 'note-field') - note_field.set('clear-existing-note', 'true') - note = SubElement(note_field, 'note') - note.text = "Library status changed to Full text requested on " \ - "{date} by {username}.".format( - date=timezone.now().strftime('%-d %B %Y'), - username=username) - - return top - - def _follow_redirects(self, response): - """If Elements redirected the call, follow the redirect up to five - steps, and then return the response (if not a redirect) or raise an - exception.""" - logger.info('Following redirects for call #{pk}'.format(pk=self.pk)) - tries = 5 - while tries > 0: - logger.info('Following redirect; {tries} tries remain'.format( - tries=tries)) - response = requests.Session.send(response.next) - if response.status_code != 303: - return response - tries -= 1 - - logger.warning('Max number of redirects exceeded') - raise requests.TooManyRedirects - - def issue(self): - """Issue a patch call to the Elements API for this call's request. - Return the response. - - This function follows redirects; the returned response will have a - non-redirect status code.""" - - logger.info('Issuing ElementsAPICall #{pk}'.format(pk=self.pk)) - - if not settings.QUOTAGUARD_URL: - logger.warning('No URL; not issuing call.') - return - - # Send request - response = requests.patch(self.request_url, - data=self.request_data, - headers=HEADERS, - proxies=PROXIES, - # Passing in an auth parameter makes requests handle HTTP Basic - # Auth transparently. - auth=(settings.ELEMENTS_USER, settings.ELEMENTS_PASSWORD)) - - if response.status_code == 303: - response = self._follow_redirects(response) - - logger.info('Returning response for call #{pk}'.format(pk=self.pk)) - return response - - def retry(self): - """Retry a call, using exponential backoff. Creates new call objects - ForeignKeyed back to self. Does not return anything. - """ - logger.info('Retrying ElementsAPICall #{pk}'.format(pk=self.pk)) - - tries = 4 - delay = 2 - - while tries > 0: - logger.info('Retry {num} for call {pk}'.format( - num=tries, pk=self.pk)) - - new_call = ElementsAPICall.objects.create( - request_data=self.request_data, - request_url=self.request_url, - retry_of=self - ) - response = new_call.issue() - new_call.update(response) - - if not new_call.should_retry: - logger.info('Not retrying #{pk} because should_retry is ' - 'False'.format(pk=new_call.pk)) - return - - time.sleep(delay) - tries -= 1 - delay *= 2 - - def update(self, response): - """Update an existing ElementsAPICall with response data.""" - logger.info('Updating ElementsAPICall #{pk}'.format(pk=self.pk)) - self.response_content = response.content - self.response_status = response.status_code - self.save() - - @property - def should_retry(self): - # If there is no status, this returns False, which is correct, because - # we can't retry a call we haven't issued (and we shouldn't retry a - # call that timed out). - return int(self.response_status) in self.STATUS_RETRY diff --git a/solenoid/elements/tasks.py b/solenoid/elements/tasks.py new file mode 100644 index 0000000..4075469 --- /dev/null +++ b/solenoid/elements/tasks.py @@ -0,0 +1,35 @@ +import requests + +from celery import shared_task +from celery.utils.log import get_task_logger +from django.conf import settings + +from .errors import RetryError + +logger = get_task_logger(__name__) + +AUTH = (settings.ELEMENTS_USER, + settings.ELEMENTS_PASSWORD) + +PROXIES = { + 'http': settings.QUOTAGUARD_URL, + 'https': settings.QUOTAGUARD_URL, +} + + +@shared_task(bind=True, + autoretry_for=(RetryError,), + retry_backoff=True) +def patch_elements_record(self, url, xml_data): + """Issue a patch to the Elements API for a given item record URL, with the + given update data. Return the response.""" + response = requests.patch(url, + data=xml_data, + headers={'Content-Type': 'text/xml'}, + proxies=PROXIES, + auth=AUTH) + if response.status_code in [409, 500, 504]: + raise RetryError(f'Elements response status {response.status_code} ' + 'requires retry') + response.raise_for_status() + return response diff --git a/solenoid/elements/tests.py b/solenoid/elements/tests.py index 9fc1a7c..0cdddf7 100644 --- a/solenoid/elements/tests.py +++ b/solenoid/elements/tests.py @@ -1,143 +1,117 @@ -from datetime import timedelta -from freezegun import freeze_time -import re -import requests -from unittest.mock import patch, MagicMock +from unittest.mock import patch from urllib.parse import urljoin from xml.etree.ElementTree import tostring +from requests.exceptions import HTTPError + +import requests_mock from django.conf import settings from django.core.exceptions import ImproperlyConfigured -from django.core.management import call_command from django.test import TestCase, override_settings from django.utils import timezone +from freezegun import freeze_time -from solenoid.emails.models import EmailMessage -from solenoid.emails.signals import email_sent - -from .models import ElementsAPICall -from .views import wrap_elements_api_call, issue_elements_api_call - - -def _fix_datestamp(str1, str2): - # Because expected and actual xml strings are generated at different - # times, we need to ensure they have the same time field so tests don't - # fail for a spurious reason. - now = timezone.now().isoformat() - str1 = re.sub( - '.*', - '{now}'.format(now=now), - str1 - ) - str2 = re.sub( - '.*', - '{now}'.format(now=now), - str2 - ) +from ..emails.models import EmailMessage +from ..emails.signals import email_sent +from .errors import RetryError +from .helpers import make_xml +from .tasks import patch_elements_record +from .views import wrap_elements_api_call - return str1, str2 +USERNAME = 'username' +with freeze_time('2019-01-01'): + GOOD_XML = (f'' + f'{timezone.now().isoformat()}' + f'' + f'Library status changed to Full text requested on ' + f'{timezone.now().strftime("%-d %B %Y")} ' + f'by username.' + f'') -USERNAME = 'username' -GOOD_XML = """ - - - - {requested_date} - - Library status changed to Full text requested on {note_date} by {username}. - - - - - """.format(username=USERNAME, - requested_date=timezone.now().isoformat(), - # This test suite will fail if you set this variable before - # midnight server time and get to test_make_xml after. - # Probably this will never arise but if it does, here's - # the note to save you hours of debugging. - note_date=timezone.now().strftime('%-d %B %Y') - # Yes, that's a DOUBLE space in the replace. We don't want - # to replace the single space between field names and - # attributes, just to strip the whitespace that's there for - # ease of reading. - ).replace(' ', '').replace('\n', '') +class HelpersTest(TestCase): + @freeze_time('2019-01-01') + def test_make_xml(self): + xml = make_xml('username') + assert GOOD_XML == tostring(xml, encoding='unicode') + + +@requests_mock.Mocker() +class TasksTest(TestCase): + def test_patch_elements_record_success(self, m): + m.register_uri('PATCH', 'mock://api.com', text='Success') + response = patch_elements_record('mock://api.com', GOOD_XML) + assert 200 == response.status_code + + def test_patch_elements_record_raises_retry(self, m): + with self.assertRaises(RetryError): + m.register_uri('PATCH', 'mock://api.com/409', status_code=409) + patch_elements_record('mock://api.com/409', GOOD_XML) + with self.assertRaises(RetryError): + m.register_uri('PATCH', 'mock://api.com/500', status_code=500) + patch_elements_record('mock://api.com/500', GOOD_XML) + with self.assertRaises(RetryError): + m.register_uri('PATCH', 'mock://api.com/504', status_code=504) + patch_elements_record('mock://api.com/504', GOOD_XML) + + def test_patch_elements_record_failure_raises_exception(self, m): + m.register_uri('PATCH', 'mock://api.com/400', status_code=400) + with self.assertRaises(HTTPError): + patch_elements_record('mock://api.com/400', GOOD_XML) class ViewsTests(TestCase): fixtures = ['testdata.yaml'] - def setUp(self): - self.call = ElementsAPICall.objects.create( - request_data=ElementsAPICall.make_xml('username'), - request_url='https://10.0.0.2', - ) - - # -------------------- Tests of wrap_elements_api_call -------------------- - @override_settings(USE_ELEMENTS=False) - @patch('solenoid.elements.views.issue_elements_api_call') - def test_use_elements_setting_respected(self, mock_call): + @patch('solenoid.elements.tasks.patch_elements_record') + def test_use_elements_setting_respected(self, mock_patch): retval = wrap_elements_api_call(EmailMessage) assert retval is False - mock_call.assert_not_called() + mock_patch.assert_not_called() @override_settings(USE_ELEMENTS=True, ELEMENTS_PASSWORD=None) - @patch('solenoid.elements.views.issue_elements_api_call') - def test_elements_password_setting_respected(self, mock_call): + @patch('solenoid.elements.tasks.patch_elements_record') + def test_elements_password_setting_respected(self, mock_patch): with self.assertRaises(ImproperlyConfigured): wrap_elements_api_call(EmailMessage) - mock_call.assert_not_called() + mock_patch.assert_not_called() @override_settings(USE_ELEMENTS=True, ELEMENTS_PASSWORD='foo') - @patch('solenoid.elements.views.issue_elements_api_call') - def test_checks_kwargs_for_username(self, mock_call): + @patch('solenoid.elements.tasks.patch_elements_record') + def test_checks_kwargs_for_username(self, mock_patch): with self.assertRaises(AssertionError): wrap_elements_api_call(EmailMessage, - instance=EmailMessage.objects.get(pk=1)) - mock_call.assert_not_called() + instance=EmailMessage.objects.get(pk=1)) + mock_patch.assert_not_called() @override_settings(USE_ELEMENTS=True, ELEMENTS_PASSWORD='foo') - @patch('solenoid.elements.views.issue_elements_api_call') - def test_checks_kwargs_for_instance(self, mock_call): + @patch('solenoid.elements.tasks.patch_elements_record') + def test_checks_kwargs_for_instance(self, mock_patch): with self.assertRaises(AssertionError): - wrap_elements_api_call(EmailMessage, - username='username') - mock_call.assert_not_called() - - # @override_settings(USE_ELEMENTS=True, ELEMENTS_PASSWORD='foo') - # @patch('solenoid.elements.views.issue_elements_api_call') - # @patch('solenoid.elements.models.ElementsAPICall.make_xml') - # def test_calls_make_xml_with_proper_args(self, mock_xml, mock_call): - # email = EmailMessage.objects.get(pk=1) - # wrap_elements_api_call(EmailMessage, - # username='username', - # instance=email) - # mock_xml.assert_called_once_with( - # username='username') - - @override_settings(USE_ELEMENTS=True, ELEMENTS_PASSWORD='foo') - @patch('solenoid.elements.views.issue_elements_api_call') - @patch('solenoid.elements.models.ElementsAPICall.objects.create') - def test_creates_api_call_with_proper_args(self, mock_create, mock_call): + wrap_elements_api_call(EmailMessage, username='username') + mock_patch.assert_not_called() + + @freeze_time('2019-01-01') + @override_settings(USE_ELEMENTS=True, ELEMENTS_PASSWORD='foo', + CELERY_ALWAYS_EAGER=True) + @patch('solenoid.elements.tasks.patch_elements_record.delay') + def test_calls_task_with_proper_args(self, mock_patch): email = EmailMessage.objects.get(pk=1) wrap_elements_api_call(EmailMessage, - username='username', - instance=email) + username='username', + instance=email) for record in email.record_set.all(): - xml = ElementsAPICall.make_xml(username='username') + xml = make_xml(username='username') url = urljoin(settings.ELEMENTS_ENDPOINT, 'publications/{id}'.format(id=record.paper_id)) - mock_create.assert_called() - _, kwargs = mock_create.call_args - - assert kwargs['request_url'] == url - expected, actual = _fix_datestamp( - kwargs['request_data'], tostring(xml).decode('utf-8')) - assert expected == actual + mock_patch.assert_called_once_with(url, + tostring(xml).decode('utf-8')) def test_wrap_is_registered_with_email_sent(self): # You can't mock out the wrap function and test that the mock was @@ -147,204 +121,3 @@ def test_wrap_is_registered_with_email_sent(self): # correctly. registered_functions = [r[1]() for r in email_sent.receivers] assert wrap_elements_api_call in registered_functions - - # ------------------- Tests of issue_elements_api_call ------------------- - - @patch('solenoid.elements.models.ElementsAPICall.retry') - @patch('solenoid.elements.models.ElementsAPICall.update') - @patch('solenoid.elements.models.ElementsAPICall.issue') - def test_handles_requests_TooManyRedirects(self, - mock_issue, mock_update, mock_retry): - mock_issue.side_effect = requests.TooManyRedirects() - # This should not raise an exception, but it also shouldn't do anything - # else. - issue_elements_api_call(self.call) - mock_update.assert_not_called() - mock_retry.assert_not_called() - - @patch('solenoid.elements.models.ElementsAPICall.retry') - @patch('solenoid.elements.models.ElementsAPICall.update') - @patch('solenoid.elements.models.ElementsAPICall.issue') - def test_handles_timeouts(self, - mock_issue, mock_update, mock_retry): - mock_issue.return_value = None - # This should not raise an exception, but it also shouldn't do anything - # else. - issue_elements_api_call(self.call) - mock_update.assert_not_called() - mock_retry.assert_not_called() - - @patch('solenoid.elements.models.ElementsAPICall.update') - @patch('solenoid.elements.models.ElementsAPICall.issue') - def test_updates_call(self, - mock_issue, mock_update): - - # The call won't actually be updated since we're mocking that out; - # let's make sure that should_retry returns False. - call = self.call - call.response_status = 200 - call.save() - - response = MagicMock(status_code=200, content='content') - mock_issue.return_value = response - - issue_elements_api_call(call) - - mock_update.assert_called_once_with(response) - - @patch('solenoid.elements.models.ElementsAPICall.retry') - @patch('solenoid.elements.models.ElementsAPICall.issue') - def test_retries_call(self, mock_issue, mock_retry): - response = MagicMock(status_code=409, content='content') - mock_issue.return_value = response - - issue_elements_api_call(self.call) - mock_retry.assert_called() - - assert self.call.should_retry # check assumption that 409 -> retry - - -class ElementsAPICallTest(TestCase): - def setUp(self): - self.call = ElementsAPICall.objects.create( - request_data=ElementsAPICall.make_xml('username'), - request_url='https://10.0.0.2', - ) - - def test_make_xml(self): - xml = ElementsAPICall.make_xml(USERNAME) - - actual, expected = _fix_datestamp( - tostring(xml).decode('utf-8'), GOOD_XML) - - self.assertEqual(expected, actual) - - @patch('requests.Session.send') - def test_follow_redirects_good(self, mock_send): - mock_send.return_value = MagicMock(status_code=400) - req = requests.Request('PATCH', 'http://10.0.0.2').prepare() - response = requests.Response() - response.status_code = 303 - response._next = req - resp = self.call._follow_redirects(response) - assert resp.status_code == 400 - - @patch('requests.Session.send') - def test_follow_redirects_infinite_loop(self, mock_send): - mock_send.return_value = MagicMock(status_code=303) - req = requests.Request('PATCH', 'http://10.0.0.2').prepare() - response = requests.Response() - response.status_code = 303 - response._next = req - with self.assertRaises(requests.TooManyRedirects): - self.call._follow_redirects(response) - - # @patch('solenoid.elements.models.ElementsAPICall._follow_redirects') - # @patch('requests.patch') - # def test_issue_follows_redirects(self, mock_patch, mock_follow): - # mock_patch.return_value = MagicMock(status_code=303) - # self.call.issue() - # mock_follow.assert_called_once() - - @patch('solenoid.elements.models.ElementsAPICall._follow_redirects') - @patch('requests.patch') - def test_issue_non_redirect(self, mock_patch, mock_follow): - mock_patch.return_value = MagicMock(status_code=200) - self.call.issue() - mock_follow.assert_not_called() - - @patch('solenoid.elements.models.ElementsAPICall.issue') - def test_retry_once(self, mock_issue): - # We're only going to test one retry, not the whole exponential backoff - # process, because that would be time-consuming and tests should be - # fast. - mock_issue.return_value = MagicMock(status_code='200', content='stuff') - self.call.retry() - mock_issue.assert_called_once() - - new_call = ElementsAPICall.objects.latest('pk') - assert new_call.response_status == '200' - assert new_call.response_content == 'stuff' - - def test_update(self): - response = MagicMock(status_code=200, - content='this seems fine') - self.call.update(response) - self.call.refresh_from_db() - assert self.call.response_status == '200' - assert self.call.response_content == 'this seems fine' - - def test_should_retry(self): - call = self.call - - # ~~~ failing statuses ~~~ # - call.response_status = 400 - call.save() - assert not call.should_retry - - call.response_status = 401 - call.save() - assert not call.should_retry - - call.response_status = 403 - call.save() - assert not call.should_retry - - call.response_status = 404 - call.save() - assert not call.should_retry - - call.response_status = 410 - call.save() - assert not call.should_retry - - # ~~ retryable statuses ~~ # - call.response_status = 409 - call.save() - assert call.should_retry - - call.response_status = 500 - call.save() - assert call.should_retry - - call.response_status = 504 - call.save() - assert call.should_retry - - # ~~~~ good statuses ~~~~ # - call.response_status = 200 - call.save() - assert not call.should_retry - - call.response_status = 303 - call.save() - assert not call.should_retry - - -class DeleteStaleAPICallsTest(TestCase): - fixtures = ['testdata.yaml'] - - def setUp(self): - self.call = ElementsAPICall.objects.create( - request_data=ElementsAPICall.make_xml('username'), - request_url='https://10.0.0.2', - response_status='200' - ) - - with freeze_time(timezone.now() - timedelta(weeks=5)): - self.call = ElementsAPICall.objects.create( - request_data=ElementsAPICall.make_xml('username'), - request_url='https://10.0.0.2' - ) - - with freeze_time(timezone.now() - timedelta(weeks=5)): - self.call = ElementsAPICall.objects.create( - request_data=ElementsAPICall.make_xml('username'), - request_url='https://10.0.0.2', - response_status='200' - ) - - def test_command(self): - self.assertEqual(len(ElementsAPICall.objects.all()), 3) - call_command('delete_stale_api_calls') - self.assertEqual(len(ElementsAPICall.objects.all()), 2) diff --git a/solenoid/elements/views.py b/solenoid/elements/views.py index 2579443..8dcf012 100644 --- a/solenoid/elements/views.py +++ b/solenoid/elements/views.py @@ -1,82 +1,23 @@ -# The goal: given a Record, mark it as requested in Elements. - -# Current status and next steps: -# * Authentication requires credentials + app whitelist + firewall whitelist; -# have acquired that for test, and requested firewall whitelist in production -# * The required workflow is: -# 1) GET the object -# 2) find all manual records associated with the object (type 'manual' is not -# guaranteed to be unique) and get their record IDs -# 3) issue PATCH requests for all records to update their c-requested field -# (and possibly the date and note fields) -# * Make sure xml validates; then remove validation parameter -# * Integrate with signals so that it's nonblocking -# * Consider whether you want to do a scheduler dyno later - might have a -# cost, but would let you rerun all failed updates -# * Consider whether you want any kind of logging/monitoring to notify you when -# the API fails -# * If we can get the record number from the CSV, do that (and definitely -# monitor it). -# Make sure to validate for the record ID when you ingest CSV. - import logging from urllib.parse import urljoin from xml.etree.ElementTree import tostring -import requests - -from django import db from django.conf import settings from django.core.exceptions import ImproperlyConfigured from django.dispatch import receiver - from solenoid.emails.signals import email_sent -from .models import ElementsAPICall - +from .helpers import make_xml +from .tasks import patch_elements_record logger = logging.getLogger(__name__) -# Honestly this belongs in the management command issue_unsent_calls, but it -# started its life here and it's way easier to test here. -def issue_elements_api_call(call): - """Sends a patch request to Elements about a Record. Takes a call argument, - which is an ElementsAPICall prepared with request data. Updates it with - response data. Does not return anything. - """ - - logger.info('Entering issue_elements_api_call for call #{pk}'.format( - pk=call.pk)) - - try: - response = call.issue() - except requests.TooManyRedirects: - logger.exception("Call #{pk} raised too many redirects".format( - pk=call.pk)) - return - - # Handle timeouts. - if not response: - logger.warning('No response received from Elements API call') - return - - call.update(response) - - if call.should_retry: - logger.warning("Call #{pk} must be retried".format(pk=call.pk)) - call.retry() - - @receiver(email_sent) def wrap_elements_api_call(sender, **kwargs): - """Creates an Elements API call object when an email has been sent. - - Unsent or retryable ElementsAPICalls should be sent via management - command. They are not sent as part of this function because they block the - request/response loop, which causes Heroku to time out and crash.""" + """Calls the patch_elements_record celery task when an email has been sent. + """ logger.info('email_sent signal received') - db.close_old_connections() if not settings.USE_ELEMENTS: logger.info('USE_ELEMENTS is False; not sending API call') @@ -96,18 +37,15 @@ def wrap_elements_api_call(sender, **kwargs): instance = kwargs['instance'] # Construct XML. (This is the same for all records in the email.) - xml = ElementsAPICall.make_xml(username=kwargs['username']) + xml = make_xml(username=kwargs['username']) + request_data = tostring(xml, encoding='unicode') for record in instance.record_set.all(): url = urljoin(settings.ELEMENTS_ENDPOINT, 'publications/{id}'.format( id=record.paper_id)) - logger.info('Constructing ElementsAPICall for record #{pk}'.format( - pk=record.pk)) + logger.info(f'Call patch_elements_record task for record #{record.pk}') - # Construct call - ElementsAPICall.objects.create( - request_data=tostring(xml).decode('utf-8'), - request_url=url - ) + # Call task + patch_elements_record.delay(url, request_data) diff --git a/solenoid/settings/base.py b/solenoid/settings/base.py index ec5ae5c..a23906c 100644 --- a/solenoid/settings/base.py +++ b/solenoid/settings/base.py @@ -399,3 +399,10 @@ # ----------------------------------------------------------------------------- DSPACE_SALT = os.getenv('DSPACE_AUTHOR_ID_SALT', default='salty') + + +# CELERY CONFIGURATION +# ----------------------------------------------------------------------------- + +CELERY_BROKER_URL = os.getenv('CELERY_BROKER_URL', + default='redis://localhost:6379/0')