Skip to content

Commit

Permalink
Report member affiliation using endpoint
Browse files Browse the repository at this point in the history
We previously reached directly into the database to achieve this, but we
should instead be invoking an API endpoint which does all the logic
directly in the gear database!
  • Loading branch information
DavidCain committed Jun 5, 2022
1 parent c335490 commit 1760266
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 68 deletions.
14 changes: 11 additions & 3 deletions ws/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
59 changes: 59 additions & 0 deletions ws/tests/utils/test_geardb.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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."""

Expand Down
85 changes: 20 additions & 65 deletions ws/utils/geardb.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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():
Expand Down

0 comments on commit 1760266

Please sign in to comment.