From 5e3df7aed4869e44ea98745c8ba38eb5e83dd3a7 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Sun, 8 Dec 2019 16:49:52 -0500 Subject: [PATCH] user_api: Remove unneeded test-only activate_account --- .../student/tests/test_activate_account.py | 11 +++- .../core/djangoapps/user_api/accounts/api.py | 29 --------- .../user_api/accounts/tests/test_api.py | 62 ------------------- .../user_authn/views/tests/test_password.py | 11 ---- .../user_authn/views/tests/test_views.py | 27 +++----- 5 files changed, 17 insertions(+), 123 deletions(-) diff --git a/common/djangoapps/student/tests/test_activate_account.py b/common/djangoapps/student/tests/test_activate_account.py index 7db4e674ac..53953b5cfb 100644 --- a/common/djangoapps/student/tests/test_activate_account.py +++ b/common/djangoapps/student/tests/test_activate_account.py @@ -5,6 +5,7 @@ import unittest from uuid import uuid4 from django.conf import settings +from django.contrib.auth.models import User from django.test import TestCase, override_settings from django.urls import reverse from mock import patch @@ -103,6 +104,10 @@ class TestActivateAccount(TestCase): response = self.client.get(reverse('dashboard')) self.assertNotContains(response, expected_message) + def _assert_user_active_state(self, expected_active_state): + user = User.objects.get(username=self.user.username) + self.assertEqual(user.is_active, expected_active_state) + def test_account_activation_notification_on_logistration(self): """ Verify that logistration page displays success/error/info messages @@ -112,15 +117,19 @@ class TestActivateAccount(TestCase): login_url=reverse('signin_user'), redirect_url=reverse('dashboard'), ) + self._assert_user_active_state(expected_active_state=False) + # Access activation link, message should say that account has been activated. response = self.client.get(reverse('activate', args=[self.registration.activation_key]), follow=True) self.assertRedirects(response, login_page_url) self.assertContains(response, 'Success! You have activated your account.') + self._assert_user_active_state(expected_active_state=True) # Access activation link again, message should say that account is already active. response = self.client.get(reverse('activate', args=[self.registration.activation_key]), follow=True) self.assertRedirects(response, login_page_url) self.assertContains(response, 'This account has already been activated.') + self._assert_user_active_state(expected_active_state=True) # Open account activation page with an invalid activation link, # there should be an error message displayed. @@ -137,4 +146,4 @@ class TestActivateAccount(TestCase): response = self.client.get(reverse('activate', args=[self.registration.activation_key]), follow=True) self.assertRedirects(response, login_page_url) self.assertContains(response, SYSTEM_MAINTENANCE_MSG) - assert not self.user.is_active + self._assert_user_active_state(expected_active_state=False) diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 4518233ed9..8568e0e781 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -331,35 +331,6 @@ def _send_email_change_requests_if_needed(data, user): ) -@helpers.intercept_errors(errors.UserAPIInternalError, ignore_errors=[errors.UserAPIRequestError]) -def activate_account(activation_key): - """Activate a user's account. - - Args: - activation_key (unicode): The activation key the user received via email. - - Returns: - None - - Raises: - errors.UserNotAuthorized - errors.UserAPIInternalError: the operation failed due to an unexpected error. - - """ - # TODO: Confirm this `activate_account` is only used for tests. If so, this should not be used for tests, and we - # should instead use the `activate_account` used for /activate. - set_custom_metric('user_api_activate_account', 'True') - if waffle().is_enabled(PREVENT_AUTH_USER_WRITES): - raise errors.UserAPIInternalError(SYSTEM_MAINTENANCE_MSG) - try: - registration = Registration.objects.get(activation_key=activation_key) - except Registration.DoesNotExist: - raise errors.UserNotAuthorized - else: - # This implicitly saves the registration - registration.activate() - - def get_name_validation_error(name): """Get the built-in validation error message for when the user's real name is invalid in some way (we wonder how). diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index eeb45b0372..b9b48fbe3c 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -28,7 +28,6 @@ from openedx.core.djangoapps.ace_common.tests.mixins import EmailTemplateTagMixi from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory from openedx.core.djangoapps.user_api.accounts import PRIVATE_VISIBILITY, USERNAME_MAX_LENGTH from openedx.core.djangoapps.user_api.accounts.api import ( - activate_account, get_account_settings, update_account_settings ) @@ -530,64 +529,3 @@ class AccountSettingsOnCreationTest(CreateAccountMixin, TestCase): expected_user_password = make_password(unicodedata.normalize('NFKC', u'Ṗŕệṿïệẅ Ṯệẍt'), salt_val) self.assertEqual(expected_user_password, user.password) - - -@ddt.ddt -class AccountActivationAndPasswordChangeTest(CreateAccountMixin, TestCase): - """ - Test cases to cover the account initialization workflow - """ - USERNAME = u'claire-underwood' - PASSWORD = u'ṕáśśẃőŕd' - EMAIL = u'claire+underwood@example.com' - - IS_SECURE = False - - def get_activation_key(self, user): - registration = Registration.objects.get(user=user) - return registration.activation_key - - @skip_unless_lms - def test_activate_account(self): - # Create the account, which is initially inactive - self.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) - user = User.objects.get(username=self.USERNAME) - activation_key = self.get_activation_key(user) - - request = RequestFactory().get("/api/user/v1/accounts/") - request.user = user - account = get_account_settings(request)[0] - self.assertEqual(self.USERNAME, account["username"]) - self.assertEqual(self.EMAIL, account["email"]) - self.assertFalse(account["is_active"]) - - # Activate the account and verify that it is now active - activate_account(activation_key) - account = get_account_settings(request)[0] - self.assertTrue(account['is_active']) - - def test_activate_account_invalid_key(self): - with pytest.raises(UserNotAuthorized): - activate_account(u'invalid') - - def test_activate_account_prevent_auth_user_writes(self): - self.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) - user = User.objects.get(username=self.USERNAME) - activation_key = self.get_activation_key(user) - - with pytest.raises(UserAPIInternalError, message=SYSTEM_MAINTENANCE_MSG): - with waffle().override(PREVENT_AUTH_USER_WRITES, True): - activate_account(activation_key) - - def _assert_is_datetime(self, timestamp): - """ - Internal helper to validate the type of the provided timestamp - """ - if not timestamp: - return False - try: - parse_datetime(timestamp) - except ValueError: - return False - else: - return True diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_password.py b/openedx/core/djangoapps/user_authn/views/tests/test_password.py index 34f5ad2180..b1cb6a2df6 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_password.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_password.py @@ -12,9 +12,6 @@ from django.test.client import RequestFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory from openedx.core.djangoapps.user_api.accounts.tests.test_api import CreateAccountMixin -from openedx.core.djangoapps.user_api.accounts.api import ( - activate_account, -) from openedx.core.djangoapps.user_api.errors import UserNotFound from openedx.core.djangoapps.user_authn.views.password_reset import request_password_change from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -33,20 +30,12 @@ class TestRequestPasswordChange(CreateAccountMixin, TestCase): IS_SECURE = False - def get_activation_key(self, user): - registration = Registration.objects.get(user=user) - return registration.activation_key - @skip_unless_lms def test_request_password_change(self): # Create and activate an account self.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) self.assertEqual(len(mail.outbox), 1) - user = User.objects.get(username=self.USERNAME) - activation_key = self.get_activation_key(user) - activate_account(activation_key) - request = RequestFactory().post('/password') request.user = Mock() request.site = SiteFactory() diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_views.py b/openedx/core/djangoapps/user_authn/views/tests/test_views.py index d116eef6b7..6649243d8c 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_views.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_views.py @@ -38,7 +38,6 @@ from course_modes.models import CourseMode from openedx.core.djangoapps.oauth_dispatch.tests import factories as dot_factories from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme_context -from openedx.core.djangoapps.user_api.accounts.api import activate_account from openedx.core.djangoapps.user_api.accounts.utils import ENABLE_SECONDARY_EMAIL_FEATURE_SWITCH from openedx.core.djangoapps.user_api.errors import UserAPIInternalError from openedx.core.djangoapps.user_authn.views.login_form import login_and_registration_form @@ -61,7 +60,7 @@ FEATURES_WITH_FAILED_PASSWORD_RESET_EMAIL['ENABLE_PASSWORD_RESET_FAILURE_EMAIL'] @skip_unless_lms @ddt.ddt class UserAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): - """ Tests for views that update the user's account information. """ + """ Tests for views that change the user's password. """ USERNAME = u"heisenberg" ALTERNATE_USERNAME = u"walt" @@ -91,22 +90,10 @@ class UserAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): def setUp(self): super(UserAccountUpdateTest, self).setUp() - # Create/activate a new account self._create_account(self.USERNAME, self.OLD_PASSWORD, self.OLD_EMAIL) - mail.outbox = [] - user = User.objects.get(username=self.USERNAME) - registration = Registration.objects.get(user=user) - activate_account(registration.activation_key) - - self.account_recovery = AccountRecoveryFactory.create(user=User.objects.get(email=self.OLD_EMAIL)) - self.enable_account_recovery_switch = Switch.objects.create( - name=ENABLE_SECONDARY_EMAIL_FEATURE_SWITCH, - active=True - ) - - # Login result = self.client.login(username=self.USERNAME, password=self.OLD_PASSWORD) self.assertTrue(result) + mail.outbox = [] @skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in LMS') def test_password_change(self): @@ -223,7 +210,7 @@ class UserAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): self._create_dot_tokens(user) response = self._change_password(email=self.OLD_EMAIL) self.assertEqual(response.status_code, 200) - self.assert_access_token_destroyed(user) + self._assert_access_token_destroyed(user) def test_access_token_invalidation_logged_in(self): user = User.objects.get(email=self.OLD_EMAIL) @@ -231,7 +218,7 @@ class UserAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): self._create_dot_tokens(user) response = self._change_password() self.assertEqual(response.status_code, 200) - self.assert_access_token_destroyed(user) + self._assert_access_token_destroyed(user) def test_password_change_inactive_user(self): # Log out the user created during test setup @@ -309,7 +296,7 @@ class UserAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): RefreshTokenFactory(user=user, client=client, access_token=access_token) def _create_dot_tokens(self, user=None): - """Create dop access token for given user if user provided else for default user.""" + """Create dot access token for given user if user provided else for default user.""" if not user: user = User.objects.get(email=self.OLD_EMAIL) @@ -317,7 +304,7 @@ class UserAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): access_token = dot_factories.AccessTokenFactory(user=user, application=application) dot_factories.RefreshTokenFactory(user=user, application=application, access_token=access_token) - def assert_access_token_destroyed(self, user): + def _assert_access_token_destroyed(self, user): """Assert all access tokens are destroyed.""" self.assertFalse(dot_access_token.objects.filter(user=user).exists()) self.assertFalse(dot_refresh_token.objects.filter(user=user).exists()) @@ -328,7 +315,7 @@ class UserAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): @skip_unless_lms @ddt.ddt class LoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMixin, ModuleStoreTestCase): - """ Tests for the student account views that update the user's account information. """ + """ Tests for Login and Registration. """ USERNAME = "bob" EMAIL = "bob@example.com" PASSWORD = u"password"