Remove UserOrganizationMapping from common; tests for enrollment multi-tenant helpers

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

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
 

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)

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

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)

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

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

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'