Fix create_user and usages to be more robust (#3330)

Author: rhysyngsun

b2b/serializers/v0/serializers_test.py

@@ -9,6 +9,7 @@
 
 
 @pytest.mark.django_db
+@pytest.mark.disable_responses
 def test_contract_page_serializer_expands_embeds():
     """Test that welcome_message_extra expands embed tags and preserves HTML."""
     contract = ContractPageFactory.create(
@@ -26,4 +27,6 @@ def test_contract_page_serializer_expands_embeds():
         r"</iframe>\s*"
         r"</div>"
     )
-    assert re.match(pattern, result, re.DOTALL)
+    assert re.match(pattern, result, re.DOTALL), (
+        f"Content did not match expectation: {result}"
+    )

courses/api.py

@@ -69,6 +69,7 @@
 from main import features
 from openedx.api import (
     create_edx_course_mode,
+    create_user,
     enroll_in_edx_course_runs,
     get_edx_api_course_detail_client,
     get_edx_api_course_list_client,
@@ -133,12 +134,33 @@ def get_user_relevant_program_course_run_qset(
     return enrollable_run_qset.order_by("enrollment_start")
 
 
-def create_run_enrollments(  # noqa: C901
+def _enroll_learner_into_associated_programs(run, user):
+    """
+    Enrolls the learner into all programs for which the course they are enrolling into
+    is associated as a requirement or elective.  If a program enrollment already exists
+    then the change_status of that program_enrollment is checked to ensure it equals None.
+    """
+    for program in run.course.programs:
+        if not program.live:
+            continue
+        program_enrollment, _ = ProgramEnrollment.objects.get_or_create(
+            user=user,
+            program=program,
+            defaults=dict(  # noqa: C408
+                change_status=None,
+            ),
+        )
+        if program_enrollment.change_status is not None:
+            program_enrollment.reactivate_and_save()
+
+
+def create_run_enrollments(  # noqa: C901,PLR0913
     user,
     runs,
     *,
     change_status=None,
     keep_failed_enrollments=None,
+    create_courseware_user=False,
     mode=EDX_DEFAULT_ENROLLMENT_MODE,
 ):
     """
@@ -170,30 +192,14 @@ def create_run_enrollments(  # noqa: C901
             features.IGNORE_EDX_FAILURES, False
         )
 
-    successful_enrollments = []
+    if create_courseware_user:
+        create_user(user)
+        user.refresh_from_db()
 
-    def send_enrollment_emails():
+    def _subscribe_to_edx_course_emails():
         subscribe_edx_course_emails.delay(enrollment.id)
 
-    def _enroll_learner_into_associated_programs():
-        """
-        Enrolls the learner into all programs for which the course they are enrolling into
-        is associated as a requirement or elective.  If a program enrollment already exists
-        then the change_status of that program_enrollment is checked to ensure it equals None.
-        """
-        for program in run.course.programs:
-            if not program.live:
-                continue
-            program_enrollment, _ = ProgramEnrollment.objects.get_or_create(
-                user=user,
-                program=program,
-                defaults=dict(  # noqa: C408
-                    change_status=None,
-                ),
-            )
-            if program_enrollment.change_status is not None:
-                program_enrollment.reactivate_and_save()
-
+    successful_enrollments = []
     edx_request_success = True
     if not runs[0].is_fake_course_run:
         # Make the API call to enroll the user in edX only if the run is not a fake course run
@@ -233,7 +239,7 @@ def _enroll_learner_into_associated_programs():
                 ),
             )
 
-            _enroll_learner_into_associated_programs()
+            _enroll_learner_into_associated_programs(run, user)
 
             # If the run is associated with a B2B contract, add the contract
             # to the user's contract list and update their org memberships
@@ -267,7 +273,7 @@ def _enroll_learner_into_associated_programs():
                     if enrollment_mode_changed:
                         enrollment.enrollment_mode = mode
                     enrollment.reactivate_and_save()
-                    transaction.on_commit(send_enrollment_emails)
+                    transaction.on_commit(_subscribe_to_edx_course_emails)
         except:  # pylint: disable=bare-except  # noqa: PERF203, E722
             mail_api.send_enrollment_failure_message(user, run, details=format_exc())
             log.exception(

courses/views/v1/__init__.py

@@ -77,7 +77,6 @@
 from main.utils import encode_json_cookie_value, redirect_with_user_message
 from openapi.utils import extend_schema_get_queryset
 from openedx.api import (
-    create_user,
     subscribe_to_edx_course_emails,
     sync_enrollments_with_edx,
     unsubscribe_from_edx_course_emails,
@@ -345,13 +344,10 @@ def post(self, request):
         if resp is not None:
             return resp
 
-        if not user.openedx_user_exists:
-            create_user(user)
-            user.refresh_from_db()
-
         _, edx_request_success = create_run_enrollments(
             user=user,
             runs=[run],
+            create_courseware_user=True,
             keep_failed_enrollments=settings.FEATURES.get(
                 features.IGNORE_EDX_FAILURES, False
             ),

courses/views/v1/views_test.py

@@ -615,10 +615,7 @@ def test_user_enrollment_delete_other_fail(mocker, settings, user_drf_client, us
 
 @pytest.mark.parametrize("api_request", [True, False])
 @pytest.mark.parametrize("product_exists", [True, False])
-@pytest.mark.parametrize("has_openedx_user", [True, False])
-def test_create_enrollments(
-    mocker, user, api_request, product_exists, has_openedx_user
-):
+def test_create_enrollments(mocker, user, api_request, product_exists):
     """
     Create enrollment view should create an enrollment and include a user message in the response cookies.
     Unless api_request is set to True, in which case we should get a string back.
@@ -627,17 +624,11 @@ def test_create_enrollments(
         "courses.views.v1.create_run_enrollments",
         return_value=(None, True),
     )
-    mock_fulfilled_order_filter = mocker.patch(  # noqa: F841
-        "ecommerce.models.FulfilledOrder.objects.filter", return_value=None
-    )
-    mock_create_user = mocker.patch("courses.views.v1.create_user")
+    mocker.patch("ecommerce.models.FulfilledOrder.objects.filter", return_value=None)
     run = CourseRunFactory.create()
     if product_exists:
         with reversion.create_revision():
-            product = ProductFactory.create(purchasable_object=run)  # noqa: F841
-
-    if not has_openedx_user:
-        user.openedx_users.all().delete()
+            ProductFactory.create(purchasable_object=run)
 
     client = Client()
     client.force_login(user)
@@ -665,7 +656,6 @@ def test_create_enrollments(
             }
         )
     patched_create_enrollments.assert_called_once()
-    mock_create_user.assert_called_once() if not has_openedx_user else mock_create_user.assert_not_called()
 
 
 def test_create_enrollments_failed(mocker, settings, user_client):

fixtures/common.py

@@ -70,10 +70,11 @@ def admin_drf_client(admin_user):
 
 
 @pytest.fixture
-def mocked_responses():
+def mocked_responses(request):
     """Mocked responses for requests library"""
-    with responses.RequestsMock() as rsps:
-        yield rsps
+    if not request.node.get_closest_marker("disable_responses"):
+        with responses.RequestsMock() as rsps:
+            yield rsps
 
 
 @pytest.fixture

main/exceptions.py

@@ -12,3 +12,7 @@ def exception_handler(exc, context):
             detail={"errors": exc.detail}, code=exc.status_code
         )
     return views.exception_handler(exc, context)
+
+
+class UnexpectedTransactionAtomicError(Exception):
+    """We attempted to run a function marked invalid for transactions while in a transaction"""

main/utils.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import contextlib
 import json
 from datetime import date, datetime
 from typing import TYPE_CHECKING, Set, Tuple, TypeVar, Union  # noqa: UP035
@@ -13,11 +14,13 @@
 from django.conf import settings
 from django.core.cache import caches
 from django.core.serializers import serialize
+from django.db import connection
 from django.http import HttpRequest, HttpResponseRedirect
 from mitol.common.utils.urls import remove_password_from_url
 from rest_framework import status
 
 from main.constants import USER_MSG_COOKIE_MAX_AGE, USER_MSG_COOKIE_NAME
+from main.exceptions import UnexpectedTransactionAtomicError
 from main.settings import TIME_ZONE
 
 if TYPE_CHECKING:
@@ -212,3 +215,34 @@ def get_redis_lock(name, **kwargs):
     redis_cache = caches["redis"]
     client = redis_cache.client.get_client()
     return redis_lock.Lock(client, name, **kwargs)
+
+
+def is_in_transaction() -> bool:
+    """Return True if we're in a transaction"""
+    if settings.ENVIRONMENT == "pytest":
+        # if we're running under pytest, tests that use django_db get wrapped in a
+        # transaction.atomic(), so the logic for whether we're in a transaction inside
+        # these tests is actually dependent on there being more than that one wrapper atomic block
+        return len(connection.atomic_blocks) > 1
+
+    return connection.in_atomic_block
+
+
+class raise_on_transaction_atomic(contextlib.ContextDecorator):  # noqa: N801
+    """
+    Context manager / decorator that will raise the passed exception
+    if this wrapped function or context executes while a
+    transaction.atomic() is active.
+
+    If an exception type isn't passed UnexpectedTransactionAtomicError is used.
+    """
+
+    def __init__(self, exc=UnexpectedTransactionAtomicError):
+        self.exc = exc
+
+    def __enter__(self):
+        if is_in_transaction():
+            raise self.exc
+
+    def __exit__(self, *_exc):
+        pass

main/utils_test.py

@@ -4,16 +4,20 @@
 from zoneinfo import ZoneInfo
 
 import pytest
+from django.db import transaction
 from mitol.common.utils.urls import remove_password_from_url
 
+from main.exceptions import UnexpectedTransactionAtomicError
 from main.models import AuditModel
 from main.settings import TIME_ZONE
 from main.utils import (
     date_to_datetime,
     get_field_names,
     get_js_settings,
     get_partitioned_set_difference,
+    is_in_transaction,
     parse_supplied_date,
+    raise_on_transaction_atomic,
 )
 
 
@@ -97,3 +101,32 @@ def test_date_to_datetime():
 
     with pytest.raises(AttributeError):
         date_to_datetime("this date isn't a date at all", TIME_ZONE)
+
+
+@pytest.mark.django_db
+def test_is_in_transaction(settings):
+    """Test that is_in_transaction functions correctly"""
+    assert is_in_transaction() is False
+
+    with transaction.atomic():
+        assert is_in_transaction() is True
+
+    settings.ENVIRONMENT = "dev"
+
+    assert is_in_transaction() is True
+
+
+@pytest.mark.django_db
+def test_raise_on_transaction_atomic():
+    """Test that raise_on_transaction_atomic correctly raises exceptions when in a transaction"""
+
+    with raise_on_transaction_atomic():
+        pass
+
+    with pytest.raises(UnexpectedTransactionAtomicError), transaction.atomic():  # noqa: SIM117
+        with raise_on_transaction_atomic():
+            pass
+
+    with pytest.raises(AttributeError), transaction.atomic():  # noqa: SIM117
+        with raise_on_transaction_atomic(AttributeError):
+            pass

openedx/api.py

@@ -30,7 +30,11 @@
 from authentication import api as auth_api
 from courses.constants import ENROLL_CHANGE_STATUS_UNENROLLED
 from main import features
-from main.utils import get_partitioned_set_difference, get_redis_lock
+from main.utils import (
+    get_partitioned_set_difference,
+    get_redis_lock,
+    raise_on_transaction_atomic,
+)
 from openedx.constants import (
     EDX_DEFAULT_ENROLLMENT_MODE,
     OPENEDX_REPAIR_GRACE_PERIOD_MINS,
@@ -47,6 +51,7 @@
     OpenEdxUserMissingError,
     UnknownEdxApiEmailSettingsException,
     UnknownEdxApiEnrollException,
+    UserCreateInTransactionError,
     UserNameUpdateFailedException,
 )
 from openedx.models import OpenEdxApiAuth, OpenEdxUser
@@ -387,6 +392,7 @@ def _create_edx_user_request(open_edx_user, user, access_token):  # noqa: C901,
         lock.release()
 
 
+@raise_on_transaction_atomic(UserCreateInTransactionError)
 def create_edx_user(user, edx_username=None):
     """
     Makes a request to create an equivalent user in Open edX