From cc51494910a6715ef5d01deb4a70731a9c51af82 Mon Sep 17 00:00:00 2001 From: tasawernawaz Date: Wed, 29 Aug 2018 11:58:13 +0500 Subject: [PATCH] Fix for duplicate email validation on account settings page LEARNER-6216 --- common/djangoapps/student/tests/test_email.py | 18 ------------------ common/djangoapps/student/views/management.py | 3 --- .../core/djangoapps/user_api/accounts/api.py | 12 ++++++++++++ .../user_api/accounts/tests/test_views.py | 17 +++++++++++++++++ 4 files changed, 29 insertions(+), 21 deletions(-) diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index 5146c5ce2f..032ca60060 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -349,24 +349,6 @@ class EmailChangeRequestTests(EventTestMixin, CacheIsolationTestCase): """ self.assertEqual(self.do_email_validation(self.user.email), 'Old email is the same as the new email.') - def test_duplicate_email(self): - """ - Assert the expected error message from the email validation method for an email address - that is already in use by another account. - """ - UserFactory.create(email=self.new_email) - self.assertEqual(self.do_email_validation(self.new_email), 'An account with this e-mail already exists.') - - def test_retired_email(self): - """ - Assert the expected error message from the email validation method for an email address - that corresponds with an already-retired account. - """ - user = UserFactory.create(email=self.new_email) - user.email = get_retired_email_by_email(self.new_email) - user.save() - self.assertEqual(self.do_email_validation(self.new_email), 'An account with this e-mail already exists.') - @patch('django.core.mail.send_mail') @patch('student.views.management.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) def test_email_failure(self, send_mail): diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 891c9f6d35..e1b475262d 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -1256,9 +1256,6 @@ def validate_new_email(user, new_email): if new_email == user.email: raise ValueError(_('Old email is the same as the new email.')) - if email_exists_or_retired(new_email): - raise ValueError(_('An account with this e-mail already exists.')) - def do_email_change_request(user, new_email, activation_key=None): """ diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index ef9ce56177..da2947bff4 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -179,6 +179,11 @@ def update_account_settings(requesting_user, update, username=None): "user_message": text_type(err) } + # Don't process with sending email to given new email, if it is already associated with + # an account. User must see same success message with no error. + # This is so that this endpoint cannot be used to determine if an email is valid or not. + changing_email = not _verify_email_does_not_already_exists(new_email) + # If the user asked to change full name, validate it if changing_full_name: try: @@ -268,6 +273,13 @@ def update_account_settings(requesting_user, update, username=None): ) +def _verify_email_does_not_already_exists(new_email): + """ + Return `True` if an account with given email already exists. + """ + return User.objects.filter(email=new_email).count() != 0 + + @helpers.intercept_errors(errors.UserAPIInternalError, ignore_errors=[errors.UserAPIRequestError]) @transaction.atomic def create_account(username, password, email): diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index a1c4a2d09b..6b484e9a4f 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -677,6 +677,23 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): ) self.assertEqual("Valid e-mail address required.", field_errors["email"]["user_message"]) + @mock.patch('student.views.management.do_email_change_request') + def test_patch_duplicate_email(self, do_email_change_request): + """ + Test that same success response will be sent to user even if the given email already used. + """ + existing_email = "same@example.com" + UserFactory.create(email=existing_email) + + client = self.login_client("client", "user") + + # Try changing to an existing email to make sure no error messages returned. + response = self.send_patch(client, {"email": existing_email}) + self.assertEqual(200, response.status_code) + + # Verify that no actual request made for email change + self.assertFalse(do_email_change_request.called) + def test_patch_language_proficiencies(self): """ Verify that patching the language_proficiencies field of the user