diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 37bc9cd97e13..489288b9fac9 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -55,12 +55,19 @@ 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 openedx.core.djangoapps.theming.helpers import ( - get_current_request, - get_current_site, +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 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 @@ -331,14 +338,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 @@ -1583,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" @@ -1593,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() - 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: - 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): @@ -2357,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: @@ -2465,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): @@ -2475,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. @@ -2493,20 +2470,16 @@ 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() - 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 + site = theming_helpers.get_current_site() - 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 + if not site: + raise User.DoesNotExist('Tahoe: Cannot get a current organization user without a site') + + organization = get_organization_by_site(site) + return get_organization_user_by_username_or_email( + username_or_email=username_or_email, + organization=organization + ) def get_user(email): 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) 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_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 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)