From b0e9b5fd1246c1f7ccc3cfbb7f264d6022d259c4 Mon Sep 17 00:00:00 2001 From: Shadi Naif Date: Thu, 24 Mar 2022 12:37:17 +0300 Subject: [PATCH 1/3] Remove UserOrganizationMapping from common --- common/djangoapps/student/models.py | 39 +++++++++++-------- common/djangoapps/student/views/management.py | 3 +- common/djangoapps/third_party_auth/utils.py | 3 +- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 37bc9cd97e13..8dc00c22fb65 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -55,8 +55,18 @@ from six.moves.urllib.parse import urlencode from slumber.exceptions import HttpClientError, HttpServerError from user_util import user_util -from organizations.models import UserOrganizationMapping, OrganizationCourse, Organization -from tahoe_sites.api import get_organization_by_site, get_organization_for_user +from organizations.models import ( + OrganizationCourse, + Organization, +) +from tahoe_sites.api import ( + deprecated_is_existing_email_but_not_linked_yet, + get_organization_by_site, + get_organization_for_user, + get_organization_user_by_email, + get_organization_user_by_username_or_email, + is_exist_organization_user_by_email, +) from openedx.core.djangoapps.theming.helpers import ( get_current_request, get_current_site, @@ -331,14 +341,11 @@ def email_exists_or_retired(email, check_for_new_site=False): """ if settings.FEATURES.get('APPSEMBLER_MULTI_TENANT_EMAILS', False): if check_for_new_site: - exists = User.objects.filter( - email=email, - userorganizationmapping__isnull=True, # Allow learners to signup for trial site, but ensure the trial - # workflow is completed. - ).exists() + # Allow learners to signup for trial site, but ensure the trial workflow is completed. + exists = deprecated_is_existing_email_but_not_linked_yet(email=email) else: current_org = get_current_organization() - exists = current_org.userorganizationmapping_set.filter(user__email=email).exists() + exists = is_exist_organization_user_by_email(email=email, organization=current_org) else: exists = User.objects.filter(email=email).exists() check_within_organization = not check_for_new_site # Allow existing learners to spin their new Tahoe trial @@ -1629,10 +1636,10 @@ def enroll_by_email_in_organization(cls, email, course_id, mode=None, ignore_err """ try: site = get_current_site() - organization = get_organization_by_site(site) - user = organization.userorganizationmapping_set.get(user__email=email).user - return cls.enroll(user, course_id, mode) - except UserOrganizationMapping.DoesNotExist: + site_org = get_organization_by_site(site) + user_to_enroll = get_organization_user_by_email(email=email, organization=site_org) + return cls.enroll(user_to_enroll, course_id, mode) + except User.DoesNotExist: err_msg = u"Tried to enroll email {} into course {}, but user not found" log.error(err_msg.format(email, course_id)) if ignore_errors: @@ -2497,10 +2504,10 @@ def get_user_by_username_or_email_inside_organization(username_or_email): # there should be one user with either username or email equal to username_or_email site = get_current_site() organization = get_organization_by_site(site) - try: - user = organization.userorganizationmapping_set.get(Q(user__email=username_or_email) | Q(user__username=username_or_email)).user - except UserOrganizationMapping.DoesNotExist: - raise User.DoesNotExist + user = get_organization_user_by_username_or_email( + username_or_email=username_or_email, + organization=organization + ) if user.username == username_or_email: UserRetirementRequest = apps.get_model('user_api', 'UserRetirementRequest') diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 6f8b9f38a9f4..610f088e2206 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -34,6 +34,7 @@ from opaque_keys.edx.keys import CourseKey from pytz import UTC from six import text_type +from tahoe_sites.api import is_exist_organization_user_by_email import track.views from bulk_email.models import Optout @@ -753,7 +754,7 @@ def confirm_email_change(request, key): if settings.FEATURES.get('APPSEMBLER_MULTI_TENANT_EMAILS', False): current_org = get_current_organization() - email_exists = len(current_org.userorganizationmapping_set.filter(user__email=pec.new_email)) != 0 + email_exists = is_exist_organization_user_by_email(email=pec.new_email, organization=current_org) else: email_exists = len(User.objects.filter(email=pec.new_email)) != 0 diff --git a/common/djangoapps/third_party_auth/utils.py b/common/djangoapps/third_party_auth/utils.py index 4462aa350968..0d9554d1df57 100644 --- a/common/djangoapps/third_party_auth/utils.py +++ b/common/djangoapps/third_party_auth/utils.py @@ -6,6 +6,7 @@ from django.conf import settings from django.contrib.auth.models import User +from tahoe_sites.api import is_exist_organization_user_by_email from openedx.core.djangoapps.appsembler.sites.utils import get_current_organization @@ -35,7 +36,7 @@ def user_exists(details): current_org = get_current_organization() if email: - return current_org.userorganizationmapping_set.filter(user__email=email).exists() + return is_exist_organization_user_by_email(email=email, organization=current_org) elif username: try: user = User.objects.get(username=username) From d45c9265ca7cd455ddb1dba0dea49f177407da20 Mon Sep 17 00:00:00 2001 From: Omar Al-Ithawi Date: Thu, 30 Jun 2022 01:58:02 +0300 Subject: [PATCH 2/3] tests and refactor for CourseEnrollment.enroll_by_email and get_user_by_username_or_email - Added tests for `get_user_by_username_or_email_inside_organization` - Consolidated `get_user_by_username_or_email_inside_organization` inside `get_user_by_username_or_email` - Added tests for `CourseEnrollment.enroll_by_email_in_organization` - Consolidated `CourseEnrollment.enroll_by_email_in_organization` inside `enroll_by_email` - Fixed RED-2007 and https://github.com/appsembler/edx-platform/pull/903 - Fixed RED-2009 as well --- common/djangoapps/student/models.py | 86 ++++++------------- lms/djangoapps/instructor/enrollment.py | 7 +- lms/djangoapps/instructor/views/api.py | 15 +--- lms/djangoapps/instructor/views/tools.py | 9 +- .../tests/test_enroll_by_email.py | 60 +++++++++++++ .../test_get_user_by_username_or_email.py | 74 ++++++++++++++++ 6 files changed, 164 insertions(+), 87 deletions(-) create mode 100644 openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_enroll_by_email.py create mode 100644 openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_get_user_by_username_or_email.py diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 8dc00c22fb65..489288b9fac9 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -67,10 +67,7 @@ get_organization_user_by_username_or_email, is_exist_organization_user_by_email, ) -from openedx.core.djangoapps.theming.helpers import ( - get_current_request, - get_current_site, -) +from openedx.core.djangoapps.theming import helpers as theming_helpers import openedx.core.djangoapps.django_comment_common.comment_client as cc from course_modes.models import CourseMode, get_cosmetic_verified_display_price @@ -1590,7 +1587,10 @@ def enroll_by_email(cls, email, course_id, mode=None, ignore_errors=True): verified the user authentication and access. """ try: - user = User.objects.get(email=email) + if settings.FEATURES.get('APPSEMBLER_MULTI_TENANT_EMAILS', False): + user = CourseEnrollment.get_user_by_email_within_organization(email) + else: + user = User.objects.get(email=email) return cls.enroll(user, course_id, mode) except User.DoesNotExist: err_msg = u"Tried to enroll email {} into course {}, but user not found" @@ -1600,51 +1600,21 @@ def enroll_by_email(cls, email, course_id, mode=None, ignore_errors=True): raise @classmethod - def enroll_by_email_in_organization(cls, email, course_id, mode=None, ignore_errors=True): + def get_user_by_email_within_organization(cls, email): """ - Appsembler Specific: This method is a copy of enroll_by_email written - above. It does mostly the same, with the difference that looks for the - user by email, but inside the organization. If the user is registered - but in a different organization, it won't be enrolled. - - Enroll a user in a course given their email and looking by the current - organization. This saves immediately. - - Note that enrolling by email is generally done in big batches and the - error rate is high. For that reason, we supress User lookup errors by - default. - - Returns a CourseEnrollment object. If the User does not exist and - `ignore_errors` is set to `True`, it will return None. + Gets a user given their email and looking by the current organization. `email` Email address of the User to add to enroll in the course. - `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) - - `mode` is a string specifying what kind of enrollment this is. The - default is the default course mode, 'audit'. Other options - include 'professional', 'verified', 'honor', - 'no-id-professional' and 'credit'. - See CourseMode in common/djangoapps/course_modes/models.py. - - `ignore_errors` is a boolean indicating whether we should suppress - `User.DoesNotExist` errors (returning None) or let it - bubble up. - It is expected that this method is called from a method which has already verified the user authentication and access. """ - try: - site = get_current_site() - site_org = get_organization_by_site(site) - user_to_enroll = get_organization_user_by_email(email=email, organization=site_org) - return cls.enroll(user_to_enroll, course_id, mode) - except User.DoesNotExist: - err_msg = u"Tried to enroll email {} into course {}, but user not found" - log.error(err_msg.format(email, course_id)) - if ignore_errors: - return None - raise + site = theming_helpers.get_current_site() + if not site: + raise User.DoesNotExist('Tahoe: Cannot get a current organization user without a site') + site_org = get_organization_by_site(site) + user_to_enroll = get_organization_user_by_email(email=email, organization=site_org) + return user_to_enroll @classmethod def unenroll(cls, user, course_id, skip_refund=False): @@ -2364,7 +2334,7 @@ def for_user(cls, user): queryset = cls.objects.filter(email=user.email).filter(Q(user__isnull=True) | Q(user=user)) if settings.FEATURES.get('APPSEMBLER_MULTI_TENANT_EMAILS', False): - if not is_request_for_new_amc_site(get_current_request()): + if not is_request_for_new_amc_site(theming_helpers.get_current_request()): try: user_organization = get_organization_for_user(user) except Organization.DoesNotExist: @@ -2472,7 +2442,12 @@ def get_user_by_username_or_email(username_or_email): """ username_or_email = strip_if_string(username_or_email) # there should be one user with either username or email equal to username_or_email - user = User.objects.get(Q(email=username_or_email) | Q(username=username_or_email)) + if settings.FEATURES.get('APPSEMBLER_MULTI_TENANT_EMAILS', False): + # Tahoe: Use custom `get_user_by_username_or_email_inside_organization` helper to only get user in the + # current organization. + user = get_user_by_username_or_email_inside_organization(username_or_email) + else: + user = User.objects.get(Q(email=username_or_email) | Q(username=username_or_email)) if user.username == username_or_email: UserRetirementRequest = apps.get_model('user_api', 'UserRetirementRequest') if UserRetirementRequest.has_user_requested_retirement(user): @@ -2482,11 +2457,6 @@ def get_user_by_username_or_email(username_or_email): def get_user_by_username_or_email_inside_organization(username_or_email): """ - Appsembler Specific: This funtion is a copy of - the get_user_by_username_or_email written above, it basically does the same - with the difference that the user search is done inside the organization, - making sure to not return users that exists in other organizations. - Return a User object by looking up a user against username_or_email but inside a certain organization. @@ -2500,21 +2470,17 @@ def get_user_by_username_or_email_inside_organization(username_or_email): MultipleObjectsReturned if more than one user has same email or username """ - username_or_email = strip_if_string(username_or_email) - # there should be one user with either username or email equal to username_or_email - site = get_current_site() + site = theming_helpers.get_current_site() + + if not site: + raise User.DoesNotExist('Tahoe: Cannot get a current organization user without a site') + organization = get_organization_by_site(site) - user = get_organization_user_by_username_or_email( + return get_organization_user_by_username_or_email( username_or_email=username_or_email, organization=organization ) - if user.username == username_or_email: - UserRetirementRequest = apps.get_model('user_api', 'UserRetirementRequest') - if UserRetirementRequest.has_user_requested_retirement(user): - raise User.DoesNotExist - return user - def get_user(email): user = User.objects.get(email=email) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 3b846de46f24..38f29d824bdb 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -168,12 +168,7 @@ def enroll_email(course_id, student_email, auto_enroll=False, email_students=Fal if previous_state.enrollment: course_mode = previous_state.mode - # Appsembler Specific: We call our custom method instead the default one - if settings.FEATURES.get('TAHOE_MULTITENANT_BULK_ENROLLMENT', False): - enrollment_obj = CourseEnrollment.enroll_by_email_in_organization(student_email, course_id, course_mode) - else: - enrollment_obj = CourseEnrollment.enroll_by_email(student_email, course_id, course_mode) - + enrollment_obj = CourseEnrollment.enroll_by_email(student_email, course_id, course_mode) if email_students: email_params['message_type'] = 'enrolled_enroll' email_params['email_address'] = student_email diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index e411f1e61f65..023bb0dc9a1a 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -122,7 +122,6 @@ UserProfile, anonymous_id_for_user, get_user_by_username_or_email, - get_user_by_username_or_email_inside_organization, is_email_retired, unique_id_for_user ) @@ -3249,12 +3248,7 @@ def get_student(username_or_email, course_key): :return: User object """ try: - # Appsembler Specific: We call our custom method insted the default one, - # to make sure the user is get inside the org. - if settings.FEATURES.get('TAHOE_MULTITENANT_BULK_ENROLLMENT', False): - student = get_user_by_username_or_email_inside_organization(username_or_email) - else: - student = get_user_by_username_or_email(username_or_email) + student = get_user_by_username_or_email(username_or_email) except ObjectDoesNotExist: raise ValueError(_(u"{user} does not exist in the LMS. Please check your spelling and retry.").format( user=username_or_email @@ -3369,12 +3363,7 @@ def build_row_errors(key, _user, row_count): user = student[user_index] try: - # Appsembler Specific: We call our custom method insted the default one, - # to make sure the user is get inside the org. - if settings.FEATURES.get('TAHOE_MULTITENANT_BULK_ENROLLMENT', False): - user = get_user_by_username_or_email_inside_organization(user) - else: - user = get_user_by_username_or_email(user) + user = get_user_by_username_or_email(user) except ObjectDoesNotExist: build_row_errors('user_not_exist', user, row_num) log.info(u'student %s does not exist', user) diff --git a/lms/djangoapps/instructor/views/tools.py b/lms/djangoapps/instructor/views/tools.py index 648656a901fb..70c8f7a0a87b 100644 --- a/lms/djangoapps/instructor/views/tools.py +++ b/lms/djangoapps/instructor/views/tools.py @@ -8,7 +8,6 @@ import dateutil import six -from django.conf import settings from django.contrib.auth.models import User from django.http import HttpResponseBadRequest from django.utils.translation import ugettext as _ @@ -18,7 +17,6 @@ from six import string_types, text_type from six.moves import zip -from student.models import get_user_by_username_or_email_inside_organization from student.models import get_user_by_username_or_email, CourseEnrollment @@ -69,12 +67,7 @@ def get_student_from_identifier(unique_student_identifier): DEPRECATED: use student.models.get_user_by_username_or_email instead. """ - # Appsembler Specific: We call our custom method insted the default one, - # to make sure the user is get inside the org. - if settings.FEATURES.get('TAHOE_MULTITENANT_BULK_ENROLLMENT', False): - return get_user_by_username_or_email_inside_organization(unique_student_identifier) - else: - return get_user_by_username_or_email(unique_student_identifier) + return get_user_by_username_or_email(unique_student_identifier) def require_student_from_identifier(unique_student_identifier): diff --git a/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_enroll_by_email.py b/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_enroll_by_email.py new file mode 100644 index 000000000000..d16511648486 --- /dev/null +++ b/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_enroll_by_email.py @@ -0,0 +1,60 @@ +""" +Tests for the `CourseEnrollment.enroll_by_email` method with `APPSEMBLER_MULTI_TENANT_EMAILS`. +""" + +import pytest + +from django.contrib.auth import get_user_model +from student.models import CourseEnrollment + +from openedx.core.djangoapps.appsembler.api.tests.factories import CourseOverviewFactory +from openedx.core.djangoapps.appsembler.multi_tenant_emails.tests.test_utils import ( + create_org_user, + with_organization_context, +) + +User = get_user_model() + + +@pytest.mark.django_db +def test_enroll_by_email_single_tenant(settings): + """ + Ensure `enroll_by_email` works as upstream intended if APPSEMBLER_MULTI_TENANT_EMAILS is disabled. + """ + settings.FEATURES = {'APPSEMBLER_MULTI_TENANT_EMAILS': False} + course = CourseOverviewFactory.create() + course_key = course.id + + with with_organization_context(site_color='blue1') as blue_org: + blue_user = create_org_user(blue_org) + + with with_organization_context(site_color='red1') as red_org: + red_user = create_org_user(red_org) + + assert CourseEnrollment.enroll_by_email(red_user.email, course_key), 'Should enroll in same site' + assert CourseEnrollment.is_enrolled(red_user, course_key) + + assert CourseEnrollment.enroll_by_email(blue_user.email, course_key), 'Should enroll in other sites' + assert CourseEnrollment.is_enrolled(blue_user, course_key) + + +@pytest.mark.django_db +def test_enroll_by_email_multi_tenant(settings): + """ + Ensure `enroll_by_email` works with APPSEMBLER_MULTI_TENANT_EMAILS is enabled. + """ + settings.FEATURES = {'APPSEMBLER_MULTI_TENANT_EMAILS': True} + course = CourseOverviewFactory.create() + course_key = course.id + + with with_organization_context(site_color='blue1') as blue_org: + blue_user = create_org_user(blue_org) + + with with_organization_context(site_color='red1') as red_org: + red_user = create_org_user(red_org) + + assert CourseEnrollment.enroll_by_email(red_user.email, course_key), 'Should enroll in same site' + assert CourseEnrollment.is_enrolled(red_user, course_key) + + assert not CourseEnrollment.enroll_by_email(blue_user.email, course_key), 'Should not enroll in other sites' + assert not CourseEnrollment.is_enrolled(blue_user, course_key), 'Should not enroll in other sites' diff --git a/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_get_user_by_username_or_email.py b/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_get_user_by_username_or_email.py new file mode 100644 index 000000000000..c3487c746790 --- /dev/null +++ b/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_get_user_by_username_or_email.py @@ -0,0 +1,74 @@ +""" +Tests for the `get_user_by_username_or_email` helper with `APPSEMBLER_MULTI_TENANT_EMAILS`. +""" + +import pytest + +from django.contrib.auth import get_user_model +from student.models import get_user_by_username_or_email + +from openedx.core.djangoapps.appsembler.multi_tenant_emails.tests.test_utils import ( + create_org_user, + with_organization_context, +) + +User = get_user_model() + + +@pytest.mark.django_db +def test_get_user_by_username_or_email_single_tenant(settings): + """ + Ensure `get_user_by_username_or_email` works as upstream intended if APPSEMBLER_MULTI_TENANT_EMAILS is disabled. + """ + settings.FEATURES = {'APPSEMBLER_MULTI_TENANT_EMAILS': False} + + with with_organization_context(site_color='blue1') as blue_org: + blue_user = create_org_user(blue_org) + + with with_organization_context(site_color='red1') as red_org: + red_user = create_org_user(red_org) + + # Use red-site context i.e. with `get_current_request` + assert blue_user == get_user_by_username_or_email(blue_user.username), 'Gets other site user by username' + assert blue_user == get_user_by_username_or_email(blue_user.email), 'Gets other site user by email' + assert red_user == get_user_by_username_or_email(red_user.username), 'Gets same site user by username' + assert red_user == get_user_by_username_or_email(red_user.email), 'Gets same site user by email' + + # Use non-site context i.e. no `get_current_request` + assert blue_user == get_user_by_username_or_email(blue_user.username), 'works for username without current request' + assert blue_user == get_user_by_username_or_email(blue_user.email), 'works for email without current request' + + +@pytest.mark.django_db +def test_get_user_by_username_or_email_multi_tenant(settings): + """ + Ensure `get_user_by_username_or_email` works with APPSEMBLER_MULTI_TENANT_EMAILS is enabled. + """ + settings.FEATURES = {'APPSEMBLER_MULTI_TENANT_EMAILS': True} + + with with_organization_context(site_color='blue1') as blue_org: + blue_user = create_org_user(blue_org) + + with with_organization_context(site_color='red1') as red_org: + red_user = create_org_user(red_org) + + with pytest.raises(User.DoesNotExist): + # Use red-site context i.e. with `get_current_request` + # Cannot get other site user by username + get_user_by_username_or_email(blue_user.username) + + with pytest.raises(User.DoesNotExist): + # Cannot get other site user by email + get_user_by_username_or_email(blue_user.email) + + assert red_user == get_user_by_username_or_email(red_user.username), 'Gets same site user by username' + assert red_user == get_user_by_username_or_email(red_user.email), 'Gets same site user by email' + + # Use non-site context i.e. no `get_current_request` + with pytest.raises(User.DoesNotExist): + # should throw exception for username with no current request + get_user_by_username_or_email(blue_user.username) + + with pytest.raises(User.DoesNotExist): + # should throw exception for email with no current request + get_user_by_username_or_email(blue_user.email) From 506c84f7001c7cffe1d1e09bdbd49d46ce2c7812 Mon Sep 17 00:00:00 2001 From: Omar Al-Ithawi Date: Thu, 30 Jun 2022 02:03:34 +0300 Subject: [PATCH 3/3] clean up for test_course_enrollment_allowed.py --- .../multi_tenant_emails/tests/test_course_enrollment_allowed.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_course_enrollment_allowed.py b/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_course_enrollment_allowed.py index d87098cf19cc..1ca90ab42cd4 100644 --- a/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_course_enrollment_allowed.py +++ b/openedx/core/djangoapps/appsembler/multi_tenant_emails/tests/test_course_enrollment_allowed.py @@ -5,12 +5,10 @@ import json from mock import patch from rest_framework import status -from unittest import skipIf from xmodule.modulestore.tests.factories import CourseFactory from django.urls import reverse from django.contrib.auth.models import User -from django.conf import settings from student.models import CourseEnrollment, CourseEnrollmentAllowed from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from student.tests.factories import UserFactory