PLAT-2217 - fix partner reporting queue 500 errors, reduce log spam

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

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)

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')

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)