diff --git a/authentication/api_test.py b/authentication/api_test.py index 8ee396b556..a3ce17d896 100644 --- a/authentication/api_test.py +++ b/authentication/api_test.py @@ -17,15 +17,12 @@ def test_create_user_session(user): assert session.session_key is not None +@pytest.mark.usefixtures("mock_validate_user_registration") def test_create_user_with_generated_username(mocker, valid_address_dict): """ Integration test to assert that create_user_with_generated_username tries to find an available username and try again to save a User if there was a username collision """ - mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.return_value = {"validation_decisions": {"name": ""}} - mocker.patch("courseware.api.get_edx_api_registration_client", return_value=mock_client) - username = "testuser" # Create a user with the desired username before calling the function so we get a collision UserFactory.create(username=username) diff --git a/authentication/pipeline/user_test.py b/authentication/pipeline/user_test.py index 1d6ab63056..7177ded2df 100644 --- a/authentication/pipeline/user_test.py +++ b/authentication/pipeline/user_test.py @@ -257,6 +257,7 @@ def test_create_user_via_email_exit(mocker, backend_name, flow): @pytest.mark.django_db +@pytest.mark.usefixtures("mock_validate_user_registration") def test_create_user_via_email(mocker, mock_email_backend, mock_create_user_strategy): """ Tests that create_user_via_email creates a user via social_core.pipeline.user.create_user_via_email, @@ -272,9 +273,6 @@ def test_create_user_via_email(mocker, mock_email_backend, mock_create_user_stra "authentication.pipeline.user.create_user_with_generated_username", return_value=fake_user, ) - mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.return_value = {"validation_decisions": {"name": ""}} - mocker.patch("courseware.api.get_edx_api_registration_client", return_value=mock_client) response = user_actions.create_user_via_email( mock_create_user_strategy, @@ -384,6 +382,7 @@ def test_create_user_via_email_with_email_case_insensitive_existing_user( "create_user_return_val,create_user_exception", # noqa: PT006 [[None, None], [UserFactory.build(), ValueError("bad value")]], # noqa: PT007 ) +@pytest.mark.usefixtures("mock_validate_user_registration") def test_create_user_via_email_create_fail( mocker, mock_email_backend, @@ -397,10 +396,6 @@ def test_create_user_via_email_create_fail( return_value=create_user_return_val, side_effect=create_user_exception, ) - mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.return_value = {"validation_decisions": {"name": ""}} - mocker.patch("courseware.api.get_edx_api_registration_client", return_value=mock_client) - with pytest.raises(UserCreationFailedException): user_actions.create_user_via_email( mock_create_user_strategy, @@ -413,6 +408,7 @@ def test_create_user_via_email_create_fail( @pytest.mark.django_db +@pytest.mark.usefixtures("mock_validate_user_registration") def test_create_user_via_email_affiliate( mocker, mock_create_user_strategy, mock_email_backend ): @@ -426,9 +422,6 @@ def test_create_user_via_email_affiliate( "authentication.pipeline.user.create_user_with_generated_username", return_value=UserFactory.build(), ) - mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.return_value = {"validation_decisions": {"name": ""}} - mocker.patch("courseware.api.get_edx_api_registration_client", return_value=mock_client) user_actions.create_user_via_email( mock_create_user_strategy, mock_email_backend, diff --git a/authentication/serializers.py b/authentication/serializers.py index b04f71e03c..701e46d7c5 100644 --- a/authentication/serializers.py +++ b/authentication/serializers.py @@ -27,6 +27,7 @@ UserTryAgainLaterException, ) from authentication.utils import SocialAuthState +from users.constants import USER_REGISTRATION_FAILED_MSG PARTIAL_PIPELINE_TOKEN_KEY = "partial_pipeline_token" # noqa: S105 @@ -177,7 +178,7 @@ def save(self, **kwargs): # noqa: C901 except UserTryAgainLaterException: result = SocialAuthState( SocialAuthState.STATE_ERROR_TEMPORARY, - errors=["Unable to register at this time, please try again later"], + errors=[USER_REGISTRATION_FAILED_MSG], ) except AuthException as exc: log.exception("Received unexpected AuthException") diff --git a/courseware/api.py b/courseware/api.py index 79fbbc7111..79c7f4d642 100644 --- a/courseware/api.py +++ b/courseware/api.py @@ -542,11 +542,14 @@ def get_edx_api_registration_client(): Returns: EdxApi: edx api registration client instance """ - if settings.MITXPRO_REGISTRATION_ACCESS_TOKEN is None: - raise ImproperlyConfigured("MITXPRO_REGISTRATION_ACCESS_TOKEN is not set") # noqa: EM101 + registration_access_token = ( + settings.MITXPRO_REGISTRATION_ACCESS_TOKEN + if settings.MITXPRO_REGISTRATION_ACCESS_TOKEN + else "" + ) return EdxApi( - {"access_token": settings.MITXPRO_REGISTRATION_ACCESS_TOKEN}, + {"access_token": registration_access_token}, settings.OPENEDX_API_BASE_URL, timeout=settings.EDX_API_CLIENT_TIMEOUT, ) @@ -844,16 +847,14 @@ def validate_name_with_edx(name): name (str): The full name Raises: - EdxApiRegistrationValidationException: Raised if response status is not OK. + EdxApiRegistrationValidationException: Raised if response status is not 200. """ edx_client = get_edx_api_registration_client() try: - resp = edx_client.user_info.validate_user_registration( - data=dict( # noqa: C408 + return edx_client.user_validation.validate_user_registration( + registration_information=dict( # noqa: C408 name=name, ) - ) - except Exception as exc: # noqa: BLE001 + ).name + except Exception as exc: raise EdxApiRegistrationValidationException(name, exc.response) from exc - - return resp["validation_decisions"]["name"] diff --git a/courseware/api_test.py b/courseware/api_test.py index dcb8c02483..1a35626703 100644 --- a/courseware/api_test.py +++ b/courseware/api_test.py @@ -112,24 +112,17 @@ def update_token_response_error(settings): return SimpleNamespace(refresh_token=refresh_token, access_token=access_token) -def test_validate_name_with_edx_success(mocker): +def test_validate_name_with_edx_success(mock_validate_user_registration): """ Test that validate_name_with_edx successfully returns the validation message from edX API when the name is valid. """ name = "Test User" - mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.return_value = {"validation_decisions": {"name": ""}} - mocker.patch( - "courseware.api.get_edx_api_registration_client", - return_value=mock_client - ) - result = validate_name_with_edx(name) assert result == "" - mock_client.user_info.validate_user_registration.assert_called_once_with( - data={"name": name} + mock_validate_user_registration.user_validation.validate_user_registration.assert_called_once_with( + registration_information={"name": name} ) @@ -142,6 +135,7 @@ def test_validate_name_with_edx_failure(mocker): class MockApiException(Exception): # noqa: N818 """Mock exception for API errors with a response attribute.""" + def __init__(self, message, response): super().__init__(message) self.response = response @@ -149,20 +143,25 @@ def __init__(self, message, response): mock_response = mocker.MagicMock() mock_response.status_code = 403 mock_response.headers = {"Content-Type": "application/json"} + mock_response.text = "Some error details" mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.side_effect = MockApiException( - "API error", response=mock_response + mock_client.user_validation.validate_user_registration.side_effect = ( + MockApiException("API error", response=mock_response) ) mocker.patch( - "courseware.api.get_edx_api_registration_client", - return_value=mock_client + "courseware.api.get_edx_api_registration_client", return_value=mock_client ) - with pytest.raises(EdxApiRegistrationValidationException): + with pytest.raises(EdxApiRegistrationValidationException) as exc_info: validate_name_with_edx(name) - mock_client.user_info.validate_user_registration.assert_called_once_with( - data={"name": name} + mock_client.user_validation.validate_user_registration.assert_called_once_with( + registration_information={"name": name} + ) + assert ( + str(exc_info.value) + == f"EdX API error validating registration name {name}.\nResponse - code: {mock_response.status_code}, " + f"content: {mock_response.text}" ) diff --git a/courseware/exceptions.py b/courseware/exceptions.py index 1dd019db33..b65fbfffbb 100644 --- a/courseware/exceptions.py +++ b/courseware/exceptions.py @@ -68,7 +68,7 @@ class UserNameUpdateFailedException(Exception): # noqa: N818 class EdxApiRegistrationValidationException(Exception): # noqa: N818 """An edX Registration Validation API call resulted in an error response""" - def __init__(self, name, response, msg=None): + def __init__(self, name, error_response, msg=None): """ Sets exception properties and adds a default message @@ -77,7 +77,7 @@ def __init__(self, name, response, msg=None): response (requests.Response): edX response for name validation """ self.name = name - self.response = response + self.response = error_response if msg is None: # Set some default useful error message msg = f"EdX API error validating registration name {self.name}.\n{get_error_response_summary(self.response)}" diff --git a/fixtures/common.py b/fixtures/common.py index 2268e5f6cf..045b7e2e38 100644 --- a/fixtures/common.py +++ b/fixtures/common.py @@ -110,3 +110,19 @@ def valid_address_dict(): def nplusone_fail(settings): # noqa: PT004 """Configures the nplusone app to raise errors""" settings.NPLUSONE_RAISE = True + + +@pytest.fixture +def mock_validate_user_registration(mocker): + """Fixture to mock validate_user_registration method.""" + mock_response = mocker.MagicMock() + mock_response.name = "" + + mock_client = mocker.MagicMock() + mock_client.user_validation.validate_user_registration.return_value = mock_response + mocker.patch( + "courseware.api.get_edx_api_registration_client", + return_value=mock_client + ) + + return mock_client diff --git a/static/js/containers/pages/profile/EditProfilePage.js b/static/js/containers/pages/profile/EditProfilePage.js index 0d3e2498dd..575fe9deff 100644 --- a/static/js/containers/pages/profile/EditProfilePage.js +++ b/static/js/containers/pages/profile/EditProfilePage.js @@ -87,11 +87,8 @@ export class EditProfilePage extends React.Component { /* eslint-disable camelcase */ body: { errors }, }: { body: Object } = await editProfile(payload); - - if (errors) { - const nonFieldErrors = errors.non_field_errors || []; - const fieldErrors = { ...errors }; - delete fieldErrors.non_field_errors; + if (errors && errors.non_field_errors) { + const nonFieldErrors = errors.non_field_errors; nonFieldErrors.forEach((error) => { addUserNotification({ @@ -104,8 +101,10 @@ export class EditProfilePage extends React.Component { }, }); }); - - setErrors(fieldErrors); + } else if (errors && errors.length > 0) { + setErrors({ + email: errors[0], + }); } else { history.push(routes.profile.view); } diff --git a/static/js/containers/pages/register/RegisterDetailsPage.js b/static/js/containers/pages/register/RegisterDetailsPage.js index 42a4ec1142..06cf2177c5 100644 --- a/static/js/containers/pages/register/RegisterDetailsPage.js +++ b/static/js/containers/pages/register/RegisterDetailsPage.js @@ -21,7 +21,6 @@ import { STATE_ERROR, STATE_EXISTING_ACCOUNT, handleAuthResponse, - STATE_REGISTER_DETAILS, } from "../../../lib/auth"; import queries from "../../../lib/queries"; import { qsPartialTokenSelector } from "../../../lib/selectors"; @@ -109,13 +108,6 @@ export class RegisterDetailsPage extends React.Component { /* eslint-disable camelcase */ [STATE_ERROR]: ({ field_errors }: AuthResponse) => setErrors(field_errors), - [STATE_REGISTER_DETAILS]: ({ field_errors }: AuthResponse) => { - // Validation failures will result in a 200 API response that still points to this page but contains - // field errors. - if (field_errors) { - setErrors(field_errors); - } - }, }); } finally { setSubmitting(false); diff --git a/static/js/containers/pages/register/RegisterDetailsPage_test.js b/static/js/containers/pages/register/RegisterDetailsPage_test.js index 74c9885d9a..383d769311 100644 --- a/static/js/containers/pages/register/RegisterDetailsPage_test.js +++ b/static/js/containers/pages/register/RegisterDetailsPage_test.js @@ -13,7 +13,6 @@ import { STATE_ERROR_TEMPORARY, FLOW_REGISTER, STATE_EXISTING_ACCOUNT, - STATE_REGISTER_DETAILS, } from "../../../lib/auth"; import { routes } from "../../../lib/urls"; import { makeRegisterAuthResponse } from "../../../factories/auth"; @@ -170,31 +169,4 @@ describe("RegisterDetailsPage", () => { "You already have an xPRO account. Please .", ); }); - - it("shows field errors from the auth response if they exist", async () => { - const { inner } = await renderPage(); - - helper.handleRequestStub.returns({ - body: makeRegisterAuthResponse({ - state: STATE_REGISTER_DETAILS, - field_errors: { name: "Invalid" }, - partial_token: "new_partial_token", - }), - }); - const onSubmit = inner.find("RegisterDetailsForm").prop("onSubmit"); - - await onSubmit(detailsData, { - setSubmitting: setSubmittingStub, - setErrors: setErrorsStub, - }); - - sinon.assert.calledWith( - helper.handleRequestStub, - "/api/register/details/", - "POST", - { body, headers: undefined, credentials: undefined }, - ); - sinon.assert.calledOnce(setErrorsStub); - sinon.assert.calledWith(setErrorsStub, { name: "Invalid" }); - }); }); diff --git a/users/serializers.py b/users/serializers.py index 3234f2360a..86784b2cc4 100644 --- a/users/serializers.py +++ b/users/serializers.py @@ -34,11 +34,6 @@ """, flags=re.I | re.VERBOSE | re.MULTILINE, ) -NAME_CONTAINS_HTML_URL_MSG = ( - "Full name can not contain HTML or URL. Please try a different one." -) - -OPENEDX_NAME_VALIDATION_MSGS_MAP = {"Enter a valid name": NAME_CONTAINS_HTML_URL_MSG} class LegalAddressSerializer(serializers.ModelSerializer): @@ -278,14 +273,11 @@ def get_unused_coupons(self, instance): return [] def validate(self, data): - """Validate an existing user""" + """Validate user data""" name = data.get("name") if name: try: openedx_validation_msg = validate_name_with_edx(name) - openedx_validation_msg = OPENEDX_NAME_VALIDATION_MSGS_MAP.get( - openedx_validation_msg, openedx_validation_msg - ) except ( HTTPError, RequestsConnectionError, @@ -295,7 +287,7 @@ def validate(self, data): raise serializers.ValidationError(USER_REGISTRATION_FAILED_MSG) # noqa: B904 if openedx_validation_msg: - raise serializers.ValidationError({"name": openedx_validation_msg}) + raise serializers.ValidationError(USER_REGISTRATION_FAILED_MSG) return data diff --git a/users/views_test.py b/users/views_test.py index 667de1eca6..f6d065eba4 100644 --- a/users/views_test.py +++ b/users/views_test.py @@ -214,13 +214,11 @@ def test_update_email_change_request_invalid_token(user_drf_client): assert resp.status_code == status.HTTP_404_NOT_FOUND +@pytest.mark.usefixtures("mock_validate_user_registration") def test_update_user_name_change(mocker, user_client, user, valid_address_dict): """Test that updating user's name is properly reflected in xPRO""" new_name = fuzzy.FuzzyText(prefix="Test-").fuzz() mocker.patch("courseware.api.update_edx_user_name") - mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.return_value = {"validation_decisions": {"name": ""}} - mocker.patch("courseware.api.get_edx_api_registration_client", return_value=mock_client) payload = { "name": new_name, "email": user.email, @@ -238,13 +236,11 @@ def test_update_user_name_change(mocker, user_client, user, valid_address_dict): assert User.objects.get(pk=user.pk).name == new_name +@pytest.mark.usefixtures("mock_validate_user_registration") def test_update_user_name_change_edx(mocker, user_client, user, valid_address_dict): """Test that PATCH on user/me also calls update user's name api in edX if there is a name change in xPRO""" new_name = fuzzy.FuzzyText(prefix="Test-").fuzz() update_edx_mock = mocker.patch("courseware.api.update_edx_user_name") - mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.return_value = {"validation_decisions": {"name": ""}} - mocker.patch("courseware.api.get_edx_api_registration_client", return_value=mock_client) payload = { "name": new_name, "email": user.email, @@ -259,12 +255,10 @@ def test_update_user_name_change_edx(mocker, user_client, user, valid_address_di update_edx_mock.assert_called_once_with(user) +@pytest.mark.usefixtures("mock_validate_user_registration") def test_update_user_no_name_change_edx(mocker, user_client, user, valid_address_dict): """Test that PATCH on user/me without name change doesn't call update user's name in edX""" update_edx_mock = mocker.patch("courseware.api.update_edx_user_name") - mock_client = mocker.MagicMock() - mock_client.user_info.validate_user_registration.return_value = {"validation_decisions": {"name": ""}} - mocker.patch("courseware.api.get_edx_api_registration_client", return_value=mock_client) resp = user_client.patch( reverse("users_api-me"), content_type="application/json",