diff --git a/ws/tasks.py b/ws/tasks.py index 36c31f67..8abc50da 100644 --- a/ws/tasks.py +++ b/ws/tasks.py @@ -5,6 +5,7 @@ from functools import wraps from time import monotonic +import requests from celery import group, shared_task from django.core.cache import cache from django.db import transaction @@ -145,11 +146,18 @@ def update_all_discount_sheets(): group([update_discount_sheet.s(pk) for pk in discount_pks])() -@shared_task # Harmless if we run it twice -def update_participant_affiliation(participant_id): +@shared_task( + auto_retry_for=(requests.exceptions.HTTPError,), + # Account for brief outages by retrying after 1 minute, then 2, then 4, then 8 + retry_backoff=60, + max_retries=4, +) +def update_participant_affiliation(participant_id: int): """Use the participant's affiliation to update the gear database.""" participant = models.Participant.objects.get(pk=participant_id) - geardb.update_affiliation(participant) + response = geardb.update_affiliation(participant) + if response: # `None` implies nothing to do + response.raise_for_status() @shared_task # Locking done at db level to ensure idempotency diff --git a/ws/tests/utils/test_geardb.py b/ws/tests/utils/test_geardb.py index d7b58318..f85e6fc7 100644 --- a/ws/tests/utils/test_geardb.py +++ b/ws/tests/utils/test_geardb.py @@ -7,6 +7,7 @@ from django.test import SimpleTestCase from freezegun import freeze_time +from ws.tests import TestCase, factories from ws.utils import geardb @@ -102,6 +103,64 @@ def test_pagination_not_handled(self): ) +class UpdateAffiliationTest(TestCase): + def test_old_student_status(self): + participant = factories.ParticipantFactory.create(affiliation='S') + + with mock.patch.object(requests, 'put') as fake_put: + response = geardb.update_affiliation(participant) + self.assertIsNone(response) + fake_put.assert_not_called() + + @staticmethod + def test_reports_affiliation(): + """We report affiliation for a simple user with just one email.""" + participant = factories.ParticipantFactory.create( + affiliation='NA', email='tim@mit.edu' + ) + + with mock.patch.object(requests, 'put') as fake_put: + geardb.update_affiliation(participant) + fake_put.assert_called_once_with( + 'https://mitoc-gear.mit.edu/api-auth/v1/affiliation/', + headers={'Authorization': mock.ANY}, + json={ + 'email': 'tim@mit.edu', + 'affiliation': 'NA', + 'other_verified_emails': [], + }, + ) + + @staticmethod + def test_reports_affiliation_with_other_emails(): + """We report all known verified emails.""" + tim = factories.ParticipantFactory.create(affiliation='NA', email='tim@mit.edu') + + factories.EmailFactory.create( + user_id=tim.user_id, + verified=False, + primary=False, + # Tim clearly doesn't own this email + email='tim@whitehouse.gov', + ) + for verified_email in ['tim@example.com', 'tim+two@mit.edu']: + factories.EmailFactory.create( + user_id=tim.user_id, verified=True, primary=False, email=verified_email + ) + + with mock.patch.object(requests, 'put') as fake_put: + geardb.update_affiliation(tim) + fake_put.assert_called_once_with( + 'https://mitoc-gear.mit.edu/api-auth/v1/affiliation/', + headers={'Authorization': mock.ANY}, + json={ + 'email': 'tim@mit.edu', + 'affiliation': 'NA', + 'other_verified_emails': ['tim+two@mit.edu', 'tim@example.com'], + }, + ) + + class NoUserTests(SimpleTestCase): """Convenience methods neatly handle missing or anonymous users.""" diff --git a/ws/utils/geardb.py b/ws/utils/geardb.py index cc9705a0..87c9a1db 100644 --- a/ws/utils/geardb.py +++ b/ws/utils/geardb.py @@ -17,24 +17,15 @@ from django.db import connections from django.db.models import Case, Count, IntegerField, Sum, When from django.db.models.functions import Lower -from mitoc_const import affiliations from ws import models, settings from ws.utils import api as api_util from ws.utils.dates import local_date logger = logging.getLogger(__name__) -# In all cases, we should use the MITOC Trips affiliation instead -DEPRECATED_GEARDB_AFFILIATIONS = {'Student'} -AFFILIATION_MAPPING = {aff.CODE: aff.VALUE for aff in affiliations.ALL} -# Deprecated statuses, but we can still map them -AFFILIATION_MAPPING['M'] = affiliations.MIT_AFFILIATE.VALUE -AFFILIATION_MAPPING['N'] = affiliations.NON_AFFILIATE.VALUE - - -API_BASE = 'https://mitoc-gear.mit.edu/api-auth/v1/' +API_BASE = 'https://mitoc-gear.mit.edu/' JsonDict = Dict[str, Any] @@ -351,7 +342,7 @@ def outstanding_items(emails: List[str]) -> Iterator[Rental]: today = local_date() - for result in query_api('rentals/', email=emails): # One row per item + for result in query_api('api-auth/v1/rentals/', email=emails): # One row per item person, gear = result['person'], result['gear'] # Map from the person record back to the requested email address @@ -386,9 +377,7 @@ def user_rentals(user) -> List[Rental]: return list(outstanding_items(verified_emails(user))) -# NOTE: This is the last (frequently-called) method which hits the db directly. -# We should replace this with an API call. -def update_affiliation(participant): +def update_affiliation(participant: models.Participant) -> Optional[requests.Response]: """Update the gear db if the affiliation of a participant has changed. This is useful in three scenarios: @@ -412,63 +401,29 @@ def update_affiliation(participant): """ if participant.affiliation == 'S': # Deprecated status, participant hasn't logged on in years - return + return None - emails = models.EmailAddress.objects.filter( + all_verified_emails = models.EmailAddress.objects.filter( verified=True, user_id=participant.user_id ).values_list('email', flat=True) - matches = list(_matching_info_for(emails)) - if not matches: - return - - def last_updated_dates(info): - """Yield the date (not timestamp!) of various updates.""" - if info['membership_expires']: - yield info['membership_expires'] - timedelta(days=365) - if info['waiver_expires']: - yield info['waiver_expires'] - timedelta(days=365) - if info['date_inserted']: # Null for a few users! - yield info['date_inserted'] - - def last_updated(info): - """Use the membership that was most recently updated.""" - dates = list(last_updated_dates(info)) - return max(dates) if dates else datetime.min.date() - - most_recent = max(matches, key=last_updated) - geardb_affiliation = AFFILIATION_MAPPING[participant.affiliation] + other_verified_emails = set(all_verified_emails) - {participant.email} - # If the database already has the same affiliation, no need to update - if geardb_affiliation == most_recent['affiliation']: - return - - # We update in a few conditions: - # - the person has an affiliation that's less specific than this one - # - the gear database has no known affiliation - # - the affiliation was updated more recently than the gear database - should_update = ( - most_recent['affiliation'] in DEPRECATED_GEARDB_AFFILIATIONS - or not most_recent['affiliation'] - or participant.profile_last_updated.date() >= last_updated(most_recent) + payload = { + 'email': participant.email, + 'affiliation': participant.affiliation, + 'other_verified_emails': sorted(other_verified_emails), + } + response = requests.put( + urljoin(API_BASE, 'api-auth/v1/affiliation/'), + # NOTE: We sign the payload here, even though current implementations just use the body. + # This does technically mean that anyone with a valid token can use the token to query any data. + # However, tokens aren't given to end users, only used on the systems which already have the secret. + headers={'Authorization': gear_bearer_jwt(**payload)}, + json=payload, ) - - if should_update: - cursor = connections['geardb'].cursor() - logger.info( - "Updating affiliation for %s from %s to %s", - participant.name, - most_recent['affiliation'], - geardb_affiliation, - ) - cursor.execute( - ''' - update people - set affiliation = %(affiliation)s - where id = %(person_id)s - ''', - {'affiliation': geardb_affiliation, 'person_id': most_recent['person_id']}, - ) + # Note that this may be a 400! + return response def _stats_only_all_active_members():