From caedc68061c0a12b74cbe9c5359cc9da5b665349 Mon Sep 17 00:00:00 2001
From: bmedx <bmesick@edx.org>
Date: Tue, 10 Jul 2018 12:54:15 -0400
Subject: [PATCH] PLAT-2217 - fix partner reporting queue 500 errors, reduce
log spam
---
openedx/core/djangoapps/credit/models.py | 33 +++++--------
.../djangoapps/credit/tests/test_models.py | 44 +++++++++--------
.../accounts/tests/test_retirement_views.py | 48 ++++++++++++++++---
.../djangoapps/user_api/accounts/views.py | 11 +++--
4 files changed, 86 insertions(+), 50 deletions(-)
diff --git a/openedx/core/djangoapps/credit/models.py b/openedx/core/djangoapps/credit/models.py
index ec956eb2858..4b5de262011 100644
--- a/openedx/core/djangoapps/credit/models.py
+++ b/openedx/core/djangoapps/credit/models.py
@@ -510,27 +510,20 @@ class CreditRequirementStatus(TimeStampedModel):
return
@classmethod
- def retire_user(cls, username_to_retire):
+ def retire_user(cls, retirement):
"""
Retire a user by anonymizing
Args:
- username_to_retire(str): Username of the user
- """
- requirement_statuses = cls.objects.filter(username=username_to_retire)
- retirement_username = get_retired_username_by_username(username_to_retire)
- if requirement_statuses.exists():
- requirement_statuses.update(
- username=retirement_username,
- reason={}
- )
- return True
- else:
- log.info(
- u'Can not retire requirement statuses for user "%s" because the user could not be found',
- username_to_retire
- )
- return False
+ retirement: UserRetirementStatus of the user being retired
+ """
+ requirement_statuses = cls.objects.filter(
+ username=retirement.original_username
+ ).update(
+ username=retirement.retired_username,
+ reason={},
+ )
+ return requirement_statuses > 0
def default_deadline_for_credit_eligibility(): # pylint: disable=invalid-name
@@ -682,16 +675,16 @@ class CreditRequest(TimeStampedModel):
get_latest_by = 'created'
@classmethod
- def retire_user(cls, original_username, retired_username):
+ def retire_user(cls, retirement):
"""
Obfuscates CreditRecord instances associated with `original_username`.
Empties the records' `parameters` field and replaces username with its
anonymized value, `retired_username`.
"""
num_updated_credit_requests = cls.objects.filter(
- username=original_username
+ username=retirement.original_username
).update(
- username=retired_username,
+ username=retirement.retired_username,
parameters={},
)
return num_updated_credit_requests > 0
diff --git a/openedx/core/djangoapps/credit/tests/test_models.py b/openedx/core/djangoapps/credit/tests/test_models.py
index ed6bd7ae4dc..da957c1041d 100644
--- a/openedx/core/djangoapps/credit/tests/test_models.py
+++ b/openedx/core/djangoapps/credit/tests/test_models.py
@@ -8,12 +8,17 @@ from django.test import TestCase
from nose.plugins.attrib import attr
from opaque_keys.edx.keys import CourseKey
-from openedx.core.djangoapps.credit.models import CreditCourse, CreditRequirement, CreditRequirementStatus
-from student.models import get_retired_username_by_username
+from openedx.core.djangoapps.credit.models import (
+ CreditCourse,
+ CreditProvider,
+ CreditRequest,
+ CreditRequirement,
+ CreditRequirementStatus
+)
+from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import RetirementTestCase
+from openedx.core.djangoapps.user_api.models import UserRetirementStatus
from student.tests.factories import UserFactory
-from ..models import CreditRequest, CreditProvider, CreditCourse
-
def add_credit_course(course_key):
""" Add the course as a credit
@@ -100,8 +105,10 @@ class CreditRequirementStatusTests(TestCase):
def setUp(self):
super(CreditRequirementStatusTests, self).setUp()
self.course_key = CourseKey.from_string("edX/DemoX/Demo_Course")
+ RetirementTestCase.setup_states()
self.old_username = "username"
- self.retired_username = get_retired_username_by_username(self.old_username)
+ self.user = UserFactory(username=self.old_username)
+ self.retirement = UserRetirementStatus.create_retirement(self.user)
self.credit_course = add_credit_course(self.course_key)
def add_course_requirements(self):
@@ -145,7 +152,7 @@ class CreditRequirementStatusTests(TestCase):
def test_retire_user(self):
self.add_course_requirements()
- retirement_succeeded = CreditRequirementStatus.retire_user(self.old_username)
+ retirement_succeeded = CreditRequirementStatus.retire_user(self.retirement)
self.assertTrue(retirement_succeeded)
old_username_records_exist = CreditRequirementStatus.objects.filter(
@@ -153,11 +160,13 @@ class CreditRequirementStatusTests(TestCase):
).exists()
self.assertFalse(old_username_records_exist)
- new_username_records_exist = CreditRequirementStatus.objects.filter(username=self.retired_username).exists()
+ new_username_records_exist = CreditRequirementStatus.objects.filter(
+ username=self.retirement.retired_username
+ ).exists()
self.assertTrue(new_username_records_exist)
- def test_retire_user_with_data(self):
- retirement_succeeded = CreditRequirementStatus.retire_user(self.retired_username)
+ def test_retire_user_without_data(self):
+ retirement_succeeded = CreditRequirementStatus.retire_user(self.retirement)
self.assertFalse(retirement_succeeded)
@@ -168,7 +177,9 @@ class CreditRequestTest(TestCase):
def setUp(self):
super(CreditRequestTest, self).setUp()
+ RetirementTestCase.setup_states()
self.user = UserFactory.create()
+ self.retirement = UserRetirementStatus.create_retirement(self.user)
self.credit_course = CreditCourse.objects.create()
self.provider = CreditProvider.objects.create()
@@ -187,13 +198,10 @@ class CreditRequestTest(TestCase):
self.assertEqual(credit_request_before_retire.parameters, test_parameters)
- user_was_retired = CreditRequest.retire_user(
- original_username=self.user.username,
- retired_username=get_retired_username_by_username(self.user.username)
- )
+ user_was_retired = CreditRequest.retire_user(self.retirement)
credit_request_before_retire.refresh_from_db()
credit_requests_after_retire = CreditRequest.objects.filter(
- username=self.user.username
+ username=self.retirement.original_username
)
self.assertTrue(user_was_retired)
@@ -209,15 +217,13 @@ class CreditRequestTest(TestCase):
parameters=test_parameters,
)
another_user = UserFactory.create()
+ another_retirement = UserRetirementStatus.create_retirement(another_user)
credit_request_before_retire = CreditRequest.objects.filter(
- username=self.user.username
+ username=self.retirement.original_username
)[0]
- was_retired = CreditRequest.retire_user(
- original_username=another_user.username,
- retired_username=get_retired_username_by_username(another_user.username)
- )
+ was_retired = CreditRequest.retire_user(another_retirement)
credit_request_before_retire.refresh_from_db()
self.assertFalse(was_retired)
diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py
index b2fe846067b..0802c3cd3af 100644
--- a/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py
+++ b/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py
@@ -51,6 +51,8 @@ from openedx.core.djangoapps.user_api.models import (
UserRetirementPartnerReportingStatus,
UserOrgTag
)
+from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import fake_retirement
+from openedx.core.djangoapps.user_api.accounts.views import AccountRetirementPartnerReportView
from openedx.core.lib.token_utils import JwtBuilder
from student.models import (
CourseEnrollment,
@@ -441,7 +443,7 @@ class TestPartnerReportingPut(RetirementTestCase, ModuleStoreTestCase):
self.maxDiff = None
self.partner_queue_state = RetirementState.objects.get(state_name='ADDING_TO_PARTNER_QUEUE')
- def post_and_assert_status(self, data, expected_status=status.HTTP_204_NO_CONTENT):
+ def put_and_assert_status(self, data, expected_status=status.HTTP_204_NO_CONTENT):
"""
Helper function for making a request to the retire subscriptions endpoint, and asserting the status.
"""
@@ -458,7 +460,7 @@ class TestPartnerReportingPut(RetirementTestCase, ModuleStoreTestCase):
for course in self.courses:
CourseEnrollment.enroll(user=retirement.user, course_key=course.id)
- self.post_and_assert_status({'username': retirement.original_username})
+ self.put_and_assert_status({'username': retirement.original_username})
self.assertTrue(UserRetirementPartnerReportingStatus.objects.filter(user=retirement.user).exists())
def test_idempotent(self):
@@ -469,9 +471,14 @@ class TestPartnerReportingPut(RetirementTestCase, ModuleStoreTestCase):
for course in self.courses:
CourseEnrollment.enroll(user=retirement.user, course_key=course.id)
- # We really do want this twice.
- self.post_and_assert_status({'username': retirement.original_username})
- self.post_and_assert_status({'username': retirement.original_username})
+ # Do our step
+ self.put_and_assert_status({'username': retirement.original_username})
+
+ # Do our basic other retirement step fakery
+ fake_retirement(retirement.user)
+
+ # Try running our step again
+ self.put_and_assert_status({'username': retirement.original_username})
self.assertTrue(UserRetirementPartnerReportingStatus.objects.filter(user=retirement.user).exists())
def test_unknown_user(self):
@@ -482,7 +489,36 @@ class TestPartnerReportingPut(RetirementTestCase, ModuleStoreTestCase):
for course in self.courses:
CourseEnrollment.enroll(user=user, course_key=course.id)
- self.post_and_assert_status({'username': user.username}, status.HTTP_404_NOT_FOUND)
+ self.put_and_assert_status({'username': user.username}, status.HTTP_404_NOT_FOUND)
+
+ def test_nonexistent_course(self):
+ """
+ Checks that if a user has been enrolled in a course that does not exist
+ (we allow this!) we can still get their orgs for partner reporting. This
+ prevents regressions of a bug we found in prod where users in this state
+ were throwing 500 errors when _get_orgs_for_user hit the database to find
+ the enrollment.course.org. We now just use the enrollment.course_id.org
+ since for this purpose we don't care if the course exists.
+ """
+ retirement = self._create_retirement(self.partner_queue_state)
+ user = retirement.user
+ enrollment = CourseEnrollment.enroll(user=user, course_key=CourseKey.from_string('edX/Test201/2018_Fall'))
+
+ # Make sure the enrollment was created
+ self.assertTrue(enrollment.is_active)
+
+ # Make sure the correct org is found and returned from the low-level call. We don't get back
+ # the orgs from our PUT operation, so this is the best way to make sure it's doing the right
+ # thing.
+ orgs = AccountRetirementPartnerReportView._get_orgs_for_user(user) # pylint: disable=protected-access
+ self.assertTrue(len(orgs) == 1)
+ self.assertTrue('edX' in orgs)
+
+ # PUT should succeed
+ self.put_and_assert_status({'username': user.username})
+
+ # Row should exist
+ self.assertTrue(UserRetirementPartnerReportingStatus.objects.filter(user=retirement.user).exists())
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS')
diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py
index 959acb1ef45..d0797ff3e77 100644
--- a/openedx/core/djangoapps/user_api/accounts/views.py
+++ b/openedx/core/djangoapps/user_api/accounts/views.py
@@ -529,17 +529,18 @@ class AccountRetirementPartnerReportView(ViewSet):
parser_classes = (JSONParser,)
serializer_class = UserRetirementStatusSerializer
- def _get_orgs_for_user(self, user):
+ @staticmethod
+ def _get_orgs_for_user(user):
"""
Returns a set of orgs that the user has enrollments with
"""
orgs = set()
for enrollment in user.courseenrollment_set.all():
- org = enrollment.course.org
+ org = enrollment.course_id.org
# Org can concievably be blank or this bogus default value
if org and org != 'outdated_entry':
- orgs.add(enrollment.course.org)
+ orgs.add(org)
return orgs
def retirement_partner_report(self, request): # pylint: disable=unused-argument
@@ -761,9 +762,9 @@ class LMSAccountRetirementView(ViewSet):
course_enrollments = CourseEnrollment.objects.filter(user=retirement.user)
ManualEnrollmentAudit.retire_manual_enrollments(course_enrollments, retirement.retired_email)
- CreditRequest.retire_user(retirement.original_username, retirement.retired_username)
+ CreditRequest.retire_user(retirement)
ApiAccessRequest.retire_user(retirement.user)
- CreditRequirementStatus.retire_user(retirement.user.username)
+ CreditRequirementStatus.retire_user(retirement)
# This signal allows code in higher points of LMS to retire the user as necessary
USER_RETIRE_LMS_MISC.send(sender=self.__class__, user=retirement.user)
--
GitLab