From 5fa4fcea5e573a7da8c76eb232b7a9aa1724a018 Mon Sep 17 00:00:00 2001 From: Christie Rice <8483753+crice100@users.noreply.github.com> Date: Mon, 2 Aug 2021 10:58:13 -0400 Subject: [PATCH] fix: Handle missing user profile when setting a name in a course certificate (#28319) MICROBA-1410 --- common/djangoapps/student/models_api.py | 14 ++++++++- .../djangoapps/student/tests/test_models.py | 31 ++++++++++++++++++- lms/djangoapps/certificates/generation.py | 7 +++-- .../certificates/tests/test_generation.py | 28 +++++++++++++++++ 4 files changed, 75 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/student/models_api.py b/common/djangoapps/student/models_api.py index 20e0363f465..b7cc4d2522f 100644 --- a/common/djangoapps/student/models_api.py +++ b/common/djangoapps/student/models_api.py @@ -52,7 +52,7 @@ def get_course_enrollment(user, course_run_key): def get_phone_number(user_id): """ - Get a users phone number from the profile, if + Get a user's phone number from the profile, if one exists. Otherwise, return None. """ try: @@ -63,6 +63,18 @@ def get_phone_number(user_id): return student.phone_number or None +def get_name(user_id): + """ + Get a user's name from their profile, if one exists. Otherwise, return None. + """ + try: + student = _UserProfile.objects.get(user_id=user_id) + except _UserProfile.DoesNotExist: + log.exception(f'Could not find UserProfile for id {user_id}') + return None + return student.name or None + + def get_course_access_role(user, org, course_id, role): """ Get a specific CourseAccessRole object. Return None if diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index b6077ea11a7..8040ad7d95e 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -26,8 +26,10 @@ from common.djangoapps.student.models import ( ManualEnrollmentAudit, PendingEmailChange, PendingNameChange, - UserCelebration + UserCelebration, + UserProfile ) +from common.djangoapps.student.models_api import get_name from common.djangoapps.student.tests.factories import AccountRecoveryFactory, CourseEnrollmentFactory, UserFactory from lms.djangoapps.courseware.models import DynamicUpgradeDeadlineConfiguration from lms.djangoapps.courseware.toggles import ( @@ -728,3 +730,30 @@ class TestUserPostSaveCallback(SharedModuleStoreTestCase): course_enrollment.save() return user + + +class TestProfile(SharedModuleStoreTestCase): + """ + Tests for the user profile + """ + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.profile = UserProfile.objects.get(user_id=self.user.id) + self.name = self.profile.name + self.course = CourseFactory.create() + + def test_name(self): + """ + Test retrieval of the name + """ + assert self.name + name = get_name(self.user.id) + assert name == self.name + + def test_name_missing_profile(self): + """ + Test retrieval of the name when the user profile doesn't exist + """ + name = get_name(None) + assert not name diff --git a/lms/djangoapps/certificates/generation.py b/lms/djangoapps/certificates/generation.py index 17c4be31f63..4e5ed4d6b05 100644 --- a/lms/djangoapps/certificates/generation.py +++ b/lms/djangoapps/certificates/generation.py @@ -10,7 +10,7 @@ These methods should be called from tasks. import logging from uuid import uuid4 -from common.djangoapps.student.models import UserProfile +from common.djangoapps.student import models_api as student_api from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.certificates.utils import emit_certificate_event @@ -66,8 +66,9 @@ def _generate_certificate(user, course_key, status, enrollment_mode, course_grad # Retrieve the existing certificate for the learner if it exists existing_certificate = GeneratedCertificate.certificate_for_student(user, course_key) - profile = UserProfile.objects.get(user=user) - profile_name = profile.name + profile_name = student_api.get_name(user.id) + if not profile_name: + profile_name = '' # Retain the `verify_uuid` from an existing certificate if possible, this will make it possible for the learner to # keep the existing URL to their certificate diff --git a/lms/djangoapps/certificates/tests/test_generation.py b/lms/djangoapps/certificates/tests/test_generation.py index 0746134ce2f..8687632e23d 100644 --- a/lms/djangoapps/certificates/tests/test_generation.py +++ b/lms/djangoapps/certificates/tests/test_generation.py @@ -2,8 +2,10 @@ Tests for certificate generation """ import logging +from unittest import mock from common.djangoapps.course_modes.models import CourseMode +from common.djangoapps.student.models import UserProfile from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from common.djangoapps.util.testing import EventTestMixin from lms.djangoapps.certificates.data import CertificateStatuses @@ -15,6 +17,8 @@ from xmodule.modulestore.tests.factories import CourseFactory log = logging.getLogger(__name__) +PROFILE_NAME_METHOD = 'common.djangoapps.student.models_api.get_name' + class CertificateTests(EventTestMixin, ModuleStoreTestCase): """ @@ -26,6 +30,8 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): # Create user, a course run, and an enrollment self.u = UserFactory() + self.profile = UserProfile.objects.get(user_id=self.u.id) + self.name = self.profile.name self.cr = CourseFactory() self.key = self.cr.id # pylint: disable=no-member CourseEnrollmentFactory( @@ -65,6 +71,7 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): assert cert.status == CertificateStatuses.downloadable assert cert.mode == self.enrollment_mode assert cert.grade == self.grade + assert cert.name == self.name def test_generation_existing_unverified(self): """ @@ -159,3 +166,24 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase): self.enrollment_mode, self.grade, self.gen_mode) assert generated_cert.status, CertificateStatuses.downloadable assert generated_cert.verify_uuid != '' + + def test_generation_missing_profile(self): + """ + Test certificate generation when the user profile is missing + """ + GeneratedCertificateFactory( + user=self.u, + course_id=self.key, + mode=CourseMode.AUDIT, + status=CertificateStatuses.unverified + ) + + with mock.patch(PROFILE_NAME_METHOD, return_value=None): + generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable, self.enrollment_mode, + self.grade, self.gen_mode) + + cert = GeneratedCertificate.objects.get(user=self.u, course_id=self.key) + assert cert.status == CertificateStatuses.downloadable + assert cert.mode == self.enrollment_mode + assert cert.grade == self.grade + assert cert.name == '' -- GitLab