HP-1787 | fix: do keycloak changes inside a transaction

profiles/schema.py

@@ -1260,7 +1260,7 @@ def mutate_and_get_payload(cls, root, info, **input):
             if sensitive_data:
                 update_sensitivedata(profile, sensitive_data)
 
-        profile_updated.send(sender=profile.__class__, instance=profile)
+            profile_updated.send(sender=profile.__class__, instance=profile)
 
         return UpdateMyProfileMutation(profile=profile)
 
@@ -1336,7 +1336,7 @@ def mutate_and_get_payload(cls, root, info, **input):
             if sensitive_data:
                 update_sensitivedata(profile, sensitive_data)
 
-        profile_updated.send(sender=profile.__class__, instance=profile)
+            profile_updated.send(sender=profile.__class__, instance=profile)
 
         return UpdateProfileMutation(profile=profile)
 
@@ -1369,9 +1369,9 @@ def mutate_and_get_payload(cls, root, info, **input):
                 profile_to_claim.save()
                 profile_to_claim.claim_tokens.all().delete()
 
-            profile_updated.send(
-                sender=profile_to_claim.__class__, instance=profile_to_claim
-            )
+                profile_updated.send(
+                    sender=profile_to_claim.__class__, instance=profile_to_claim
+                )
 
             return ClaimProfileMutation(profile=profile_to_claim)
 

profiles/tests/test_gql_update_my_profile_mutation.py

@@ -589,19 +589,22 @@ def test_remove_all_emails_if_they_are_not_primary(user_gql_client):
     assert executed["data"] == expected_data
 
 
-def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_produced(
+def test_when_keycloak_returns_conflict_on_update_changes_are_reverted(
     user_gql_client, keycloak_setup, mocker
 ):
+    """Correct error code is produced and local changes are reverted."""
     profile = ProfileWithPrimaryEmailFactory(user=user_gql_client.user)
-    email = profile.emails.first()
+    primary_email = profile.get_primary_email()
+    original_email = primary_email.email
+    new_email = f"new-{primary_email.email}"
 
     mocker.patch.object(
         keycloak.KeycloakAdminClient,
         "get_user",
         return_value={
             "firstName": profile.first_name,
             "lastName": profile.last_name,
-            "email": "[email protected]",
+            "email": primary_email.email,
         },
     )
 
@@ -612,13 +615,14 @@ def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_pro
     )
 
     email_updates = [
-        {"id": to_global_id("EmailNode", email.id), "email": "[email protected]"}
+        {"id": to_global_id("EmailNode", primary_email.id), "email": new_email}
     ]
     executed = user_gql_client.execute(
         EMAILS_MUTATION, variables={"profileInput": {"updateEmails": email_updates}}
     )
 
     assert_match_error_code(executed, "DATA_CONFLICT_ERROR")
+    assert profile.get_primary_email().email == original_email
 
 
 PHONES_MUTATION = """

profiles/tests/test_gql_update_profile_mutation.py

@@ -272,11 +272,14 @@ def test_changing_an_email_address_marks_it_unverified(user_gql_client, service)
     assert executed["data"] == expected_data
 
 
-def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_produced(
+def test_when_keycloak_returns_conflict_on_update_changes_are_reverted(
     user_gql_client, service, keycloak_setup, mocker
 ):
+    """Correct error code is produced and local changes are reverted."""
     profile = ProfileWithPrimaryEmailFactory(user=user_gql_client.user)
-    email = profile.emails.first()
+    primary_email = profile.get_primary_email()
+    original_email = primary_email.email
+    new_email = f"new-{primary_email.email}"
 
     setup_profile_and_staff_user_to_service(profile, user_gql_client.user, service)
 
@@ -286,7 +289,7 @@ def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_pro
         return_value={
             "firstName": profile.first_name,
             "lastName": profile.last_name,
-            "email": "[email protected]",
+            "email": primary_email.email,
         },
     )
 
@@ -301,8 +304,8 @@ def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_pro
             "id": to_global_id("ProfileNode", profile.id),
             "updateEmails": [
                 {
-                    "id": to_global_id("EmailNode", email.id),
-                    "email": "[email protected]",
+                    "id": to_global_id("EmailNode", primary_email.id),
+                    "email": new_email,
                 }
             ],
         }
@@ -312,6 +315,7 @@ def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_pro
     )
 
     assert_match_error_code(executed, "DATA_CONFLICT_ERROR")
+    assert profile.get_primary_email().email == original_email
 
 
 class TestProfileInputValidation(ExistingProfileInputValidationBase):

profiles/tests/test_profile_changes_to_keycloak.py

@@ -34,7 +34,6 @@ def test_do_nothing_if_user_is_not_found_in_keycloak(mocker):
     mocked_update_user = mocker.patch.object(
         keycloak.KeycloakAdminClient, "update_user"
     )
-
     profile = ProfileFactory()
 
     profile_updated.send(sender=profile.__class__, instance=profile)
@@ -48,7 +47,6 @@ def test_changed_names_are_sent_to_keycloak(mocker):
         "lastName": "New last name",
         "email": None,
     }
-
     mocker.patch.object(
         keycloak.KeycloakAdminClient,
         "get_user",
@@ -57,11 +55,11 @@ def test_changed_names_are_sent_to_keycloak(mocker):
     mocked_update_user = mocker.patch.object(
         keycloak.KeycloakAdminClient, "update_user"
     )
-
     profile = ProfileFactory(
         first_name=new_values["firstName"], last_name=new_values["lastName"]
     )
     user_id = profile.user.uuid
+
     profile_updated.send(sender=profile.__class__, instance=profile)
 
     mocked_update_user.assert_called_once_with(user_id, new_values)
@@ -77,7 +75,6 @@ def test_changing_email_causes_it_to_be_marked_unverified(
         "email": "[email protected]",
         "emailVerified": False,
     }
-
     mocker.patch.object(
         keycloak.KeycloakAdminClient,
         "get_user",
@@ -99,12 +96,12 @@ def test_changing_email_causes_it_to_be_marked_unverified(
             else Exception("send_verify_email failed")
         ),
     )
-
     profile = ProfileFactory(
         first_name=new_values["firstName"], last_name=new_values["lastName"]
     )
     profile.emails.create(email=new_values["email"], primary=True)
     user_id = profile.user.uuid
+
     profile_updated.send(sender=profile.__class__, instance=profile)
 
     mocked_update_user.assert_called_once_with(user_id, new_values)
@@ -113,23 +110,21 @@ def test_changing_email_causes_it_to_be_marked_unverified(
 
 def test_if_there_are_no_changes_then_nothing_is_sent_to_keycloak(mocker):
     values = {"firstName": "First name", "lastName": "Last name"}
-
     mocker.patch.object(keycloak.KeycloakAdminClient, "get_user", return_value=values)
     mocked_update_user = mocker.patch.object(
         keycloak.KeycloakAdminClient, "update_user"
     )
-
     profile = ProfileFactory(
         first_name=values["firstName"], last_name=values["lastName"]
     )
+
     profile_updated.send(sender=profile.__class__, instance=profile)
 
     mocked_update_user.assert_not_called()
 
 
 def test_when_update_causes_a_conflict_then_data_conflict_error_is_raised(mocker):
     profile = ProfileWithPrimaryEmailFactory()
-
     mocker.patch.object(
         keycloak.KeycloakAdminClient,
         "get_user",
@@ -139,7 +134,6 @@ def test_when_update_causes_a_conflict_then_data_conflict_error_is_raised(mocker
             "email": "[email protected]",
         },
     )
-
     mocker.patch.object(
         keycloak.KeycloakAdminClient,
         "update_user",