From fc7bcd82f8d8048638fc7bfd2087e2f1ab8f9f4c Mon Sep 17 00:00:00 2001 From: Collin Preston Date: Fri, 17 Mar 2023 16:47:32 -0400 Subject: [PATCH 1/3] Issue occurs when User and OpenEdxUser records exist in mitxonline, but no user record exists in edx. --- openedx/api.py | 26 ++++++++++++++----------- openedx/api_test.py | 47 ++++++++++++++++++++++++++++++++++++++------- openedx/models.py | 2 +- 3 files changed, 56 insertions(+), 19 deletions(-) diff --git a/openedx/api.py b/openedx/api.py index 4a5bd3055a..fcfd51ca92 100644 --- a/openedx/api.py +++ b/openedx/api.py @@ -94,12 +94,20 @@ def create_edx_user(user): ) with transaction.atomic(): - _, created = OpenEdxUser.objects.select_for_update().get_or_create( + open_edx_user, created = OpenEdxUser.objects.select_for_update().get_or_create( user=user, platform=PLATFORM_EDX ) - if not created: - return + if not created and open_edx_user.has_been_synced: + # Here we should check with edx that the user exists on that end. + try: + get_edx_api_client(user) + except: + pass + else: + open_edx_user.has_been_synced = True + open_edx_user.save() + return created # a non-200 status here will ensure we rollback creation of the OpenEdxUser and try again req_session = requests.Session() @@ -125,6 +133,9 @@ def create_edx_user(user): raise OpenEdxUserCreateError( f"Error creating Open edX user. {get_error_response_summary(resp)}" ) + open_edx_user.has_been_synced = True + open_edx_user.save() + return created @transaction.atomic @@ -290,14 +301,7 @@ def repair_faulty_edx_user(user): """ created_user, created_auth_token = False, False try: - if ( - find_object_with_matching_attr( - user.openedx_users.all(), "platform", value=PLATFORM_EDX - ) - is None - ): - create_edx_user(user) - created_user = True + created_user = create_edx_user(user) except Exception as e: # 409 means we have a username conflict - pass in that case so we can # try to create the api auth tokens; re-raise otherwise diff --git a/openedx/api_test.py b/openedx/api_test.py index 6551fd9040..55110f8214 100644 --- a/openedx/api_test.py +++ b/openedx/api_test.py @@ -182,6 +182,42 @@ def test_create_edx_user_conflict(settings, user): assert OpenEdxUser.objects.count() == 0 +@responses.activate +@pytest.mark.parametrize( + "open_edx_user_record_exists,open_edx_user_record_has_been_synced", + itertools.product([True, False], [True, False]), +) +def test_create_edx_user_for_user_not_synced_with_edx( + mocker, + settings, + user, + open_edx_user_record_exists, + open_edx_user_record_has_been_synced, +): + """Test that create_edx_user validates the user record on Edx if an OpenEdxUser record already exists.""" + + responses.add( + responses.POST, + f"{settings.OPENEDX_API_BASE_URL}/user_api/v1/account/registration/", + json=dict(success=True), + status=status.HTTP_200_OK, + ) + OpenEdxUserFactory.create( + user=user, has_been_synced=open_edx_user_record_has_been_synced + ) + mocker.patch( + "openedx.api.get_edx_api_client", + side_effect=ValueError("Unexpected error") + if not open_edx_user_record_exists + else None, + ) + + user_created_in_edx = create_edx_user(user) + + assert OpenEdxUser.objects.get(user=user).has_been_synced is True + assert user_created_in_edx is False if open_edx_user_record_exists else True + + @responses.activate def test_validate_edx_username_conflict(settings, user): """Test that validate_username_with_edx handles a username validation conflict""" @@ -502,19 +538,16 @@ def test_repair_faulty_edx_user(mocker, user, no_openedx_user, no_edx_auth): Tests that repair_faulty_edx_user creates OpenEdxUser/OpenEdxApiAuth objects as necessary and returns flags that indicate what was created """ - patched_create_edx_user = mocker.patch("openedx.api.create_edx_user") patched_create_edx_auth_token = mocker.patch("openedx.api.create_edx_auth_token") - openedx_user = OpenEdxUserFactory.create(user=user) - patched_find_object = mocker.patch( - "openedx.api.find_object_with_matching_attr", - return_value=None if no_openedx_user else openedx_user, + OpenEdxUserFactory.create(user=user) + mocker.patch( + "openedx.api.create_edx_user", + return_value=True if no_openedx_user else False, ) openedx_api_auth = None if no_edx_auth else OpenEdxApiAuthFactory.build() user.openedx_api_auth = openedx_api_auth created_user, created_auth_token = repair_faulty_edx_user(user) - patched_find_object.assert_called() - assert patched_create_edx_user.called is no_openedx_user assert patched_create_edx_auth_token.called is no_edx_auth assert created_user is no_openedx_user assert created_auth_token is no_edx_auth diff --git a/openedx/models.py b/openedx/models.py index 9f7d6e6e76..179ffd4144 100644 --- a/openedx/models.py +++ b/openedx/models.py @@ -18,7 +18,7 @@ class OpenEdxUser(TimestampedModel): max_length=20, choices=OPENEDX_PLATFORM_CHOICES, default=PLATFORM_EDX ) has_been_synced = models.BooleanField( - default=True, + default=False, help_text="Indicates whether a corresponding user has been created on the openedx platform", ) From 5d80f246777bb5344b260328751580af64b4ae26 Mon Sep 17 00:00:00 2001 From: Collin Preston Date: Fri, 17 Mar 2023 16:49:32 -0400 Subject: [PATCH 2/3] Add migration --- .../0005_alter_openedxuser_has_been_synced.py | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 openedx/migrations/0005_alter_openedxuser_has_been_synced.py diff --git a/openedx/migrations/0005_alter_openedxuser_has_been_synced.py b/openedx/migrations/0005_alter_openedxuser_has_been_synced.py new file mode 100644 index 0000000000..38523d9a38 --- /dev/null +++ b/openedx/migrations/0005_alter_openedxuser_has_been_synced.py @@ -0,0 +1,21 @@ +# Generated by Django 3.2.18 on 2023-03-17 20:49 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("openedx", "0004_add_openedx_related_names"), + ] + + operations = [ + migrations.AlterField( + model_name="openedxuser", + name="has_been_synced", + field=models.BooleanField( + default=False, + help_text="Indicates whether a corresponding user has been created on the openedx platform", + ), + ), + ] From fe1146f5640c031dbf34f150085af0de7a395c1c Mon Sep 17 00:00:00 2001 From: Collin Preston Date: Mon, 20 Mar 2023 14:08:38 -0400 Subject: [PATCH 3/3] Address code review comments --- openedx/api.py | 7 ++++--- openedx/api_test.py | 13 +++++++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/openedx/api.py b/openedx/api.py index fcfd51ca92..237667b86e 100644 --- a/openedx/api.py +++ b/openedx/api.py @@ -101,13 +101,14 @@ def create_edx_user(user): if not created and open_edx_user.has_been_synced: # Here we should check with edx that the user exists on that end. try: - get_edx_api_client(user) + client = get_edx_api_client(user) + client.user_info.get_user_info() except: pass else: open_edx_user.has_been_synced = True open_edx_user.save() - return created + return False # a non-200 status here will ensure we rollback creation of the OpenEdxUser and try again req_session = requests.Session() @@ -135,7 +136,7 @@ def create_edx_user(user): ) open_edx_user.has_been_synced = True open_edx_user.save() - return created + return True @transaction.atomic diff --git a/openedx/api_test.py b/openedx/api_test.py index 55110f8214..cb7a5665a4 100644 --- a/openedx/api_test.py +++ b/openedx/api_test.py @@ -205,17 +205,18 @@ def test_create_edx_user_for_user_not_synced_with_edx( OpenEdxUserFactory.create( user=user, has_been_synced=open_edx_user_record_has_been_synced ) - mocker.patch( - "openedx.api.get_edx_api_client", - side_effect=ValueError("Unexpected error") - if not open_edx_user_record_exists - else None, + mock_client = mocker.MagicMock() + mock_client.user_info.get_user_info = mocker.Mock( + side_effect=Exception if not open_edx_user_record_exists else None, ) + mocker.patch("openedx.api.get_edx_api_client", return_value=mock_client) user_created_in_edx = create_edx_user(user) assert OpenEdxUser.objects.get(user=user).has_been_synced is True - assert user_created_in_edx is False if open_edx_user_record_exists else True + assert user_created_in_edx is not ( + open_edx_user_record_exists and open_edx_user_record_has_been_synced + ) @responses.activate