diff --git a/common/djangoapps/student/message_types.py b/common/djangoapps/student/message_types.py index da89a2a3f5..5829557067 100644 --- a/common/djangoapps/student/message_types.py +++ b/common/djangoapps/student/message_types.py @@ -10,3 +10,10 @@ class PasswordReset(BaseMessageType): super(PasswordReset, self).__init__(*args, **kwargs) self.options['transactional'] = True + + +class EmailChange(BaseMessageType): + def __init__(self, *args, **kwargs): + super(EmailChange, self).__init__(*args, **kwargs) + + self.options['transactional'] = True diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index c6ff803e4e..babf69167b 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -1,4 +1,4 @@ - +# coding=utf-8 import json import unittest @@ -14,8 +14,10 @@ from mock import Mock, patch from six import text_type from edxmako.shortcuts import render_to_string +from openedx.core.djangoapps.ace_common.tests.mixins import EmailTemplateTagMixin from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme +from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, SYSTEM_MAINTENANCE_MSG, waffle from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, CacheIsolationMixin from student.models import ( @@ -275,13 +277,14 @@ class ReactivationEmailTests(EmailTestMixin, CacheIsolationTestCase): self.assertTrue(response_data['success']) -class EmailChangeRequestTests(EventTestMixin, CacheIsolationTestCase): +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', "Test only valid in LMS") +class EmailChangeRequestTests(EventTestMixin, EmailTemplateTagMixin, CacheIsolationTestCase): """ Test changing a user's email address """ - def setUp(self): - super(EmailChangeRequestTests, self).setUp('student.views.management.tracker') + def setUp(self, tracker='student.views.management.tracker'): + super(EmailChangeRequestTests, self).setUp(tracker) self.user = UserFactory.create() self.new_email = 'new.email@edx.org' self.req_factory = RequestFactory() @@ -305,10 +308,8 @@ class EmailChangeRequestTests(EventTestMixin, CacheIsolationTestCase): """ Executes do_email_change_request, returning any resulting error message. """ - try: + with patch('crum.get_current_request', return_value=self.fake_request): do_email_change_request(user, email, activation_key) - except ValueError as err: - return text_type(err) def assertFailedRequest(self, response_data, expected_error): """ @@ -332,8 +333,8 @@ class EmailChangeRequestTests(EventTestMixin, CacheIsolationTestCase): user2 = UserFactory.create(email=self.new_email, password="test2") # Send requests & ensure no error was thrown - self.assertIsNone(self.do_email_change(self.user, user1_new_email)) - self.assertIsNone(self.do_email_change(user2, user2_new_email)) + self.do_email_change(self.user, user1_new_email) + self.do_email_change(user2, user2_new_email) def test_invalid_emails(self): """ @@ -350,43 +351,64 @@ class EmailChangeRequestTests(EventTestMixin, CacheIsolationTestCase): self.assertEqual(self.do_email_validation(self.user.email), 'Old email is the same as the new email.') @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): """ Test the return value if sending the email for the user to click fails. """ send_mail.side_effect = [Exception, None] - self.assertEqual( - self.do_email_change(self.user, "valid@email.com"), - 'Unable to send email activation link. Please try again later.' - ) + with self.assertRaisesRegexp(ValueError, 'Unable to send email activation link. Please try again later.'): + self.do_email_change(self.user, "valid@email.com") + self.assert_no_events_were_emitted() - @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_success(self, send_mail): + def test_email_success(self): """ Test email was sent if no errors encountered. """ old_email = self.user.email new_email = "valid@example.com" - registration_key = "test registration key" - self.assertIsNone(self.do_email_change(self.user, new_email, registration_key)) - context = { - 'key': registration_key, - 'old_email': old_email, - 'new_email': new_email - } - send_mail.assert_called_with( - mock_render_to_string('emails/email_change_subject.txt', context), - mock_render_to_string('emails/email_change.txt', context), - configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL), - [new_email] + registration_key = "test-registration-key" + + self.do_email_change(self.user, new_email, registration_key) + + self._assert_email( + subject=u'Request to change édX account e-mail', + body_fragments=[ + u'We received a request to change the e-mail associated with', + u'your édX account from {old_email} to {new_email}.'.format( + old_email=old_email, + new_email=new_email, + ), + u'If this is correct, please confirm your new e-mail address by visiting:', + u'http://edx.org/email_confirm/{key}'.format(key=registration_key), + u'If you didn\'t request this, you don\'t need to do anything;', + u'you won\'t receive any more email from us.', + u'Please do not reply to this e-mail; if you require assistance,', + u'check the help section of the édX web site.', + ], ) + self.assert_event_emitted( SETTING_CHANGE_INITIATED, user_id=self.user.id, setting=u'email', old=old_email, new=new_email ) + def _assert_email(self, subject, body_fragments): + """ + Verify that the email was sent. + """ + assert len(mail.outbox) == 1 + assert len(body_fragments) > 1, 'Should provide at least two body fragments' + + message = mail.outbox[0] + text = message.body + html = message.alternatives[0][0] + + assert message.subject == subject + + for body in text, html: + for fragment in body_fragments: + assert fragment in body + @patch('django.contrib.auth.models.User.email_user') @patch('student.views.management.render_to_response', Mock(side_effect=mock_render_to_response, autospec=True)) diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 07c4dda5b1..e01f0fb2a2 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -14,6 +14,7 @@ from django.contrib import messages from django.contrib.auth.decorators import login_required from django.contrib.auth.models import AnonymousUser, User from django.contrib.auth.views import password_reset_confirm +from django.contrib.sites.models import Site from django.core import mail from django.urls import reverse from django.core.validators import ValidationError, validate_email @@ -29,6 +30,8 @@ from django.utils.http import base36_to_int, urlsafe_base64_encode from django.utils.translation import ugettext as _ from django.views.decorators.csrf import csrf_exempt, ensure_csrf_cookie from django.views.decorators.http import require_GET, require_POST, require_http_methods +from edx_ace import ace +from edx_ace.recipient import Recipient from edx_django_utils import monitoring as monitoring_utils from eventtracking import tracker from ipware.ip import get_ip @@ -38,7 +41,6 @@ from opaque_keys.edx.keys import CourseKey from pytz import UTC from six import text_type from xmodule.modulestore.django import modulestore - import track.views from course_modes.models import CourseMode from edx_ace import ace @@ -49,6 +51,7 @@ from entitlements.models import CourseEntitlement from openedx.core.djangoapps.ace_common.template_context import get_base_template_context from openedx.core.djangoapps.catalog.utils import get_programs_with_type from openedx.core.djangoapps.embargo import api as embargo_api +from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.oauth_dispatch.api import destroy_oauth_tokens from openedx.core.djangoapps.programs.models import ProgramsApiConfig from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -71,7 +74,7 @@ from student.helpers import ( generate_activation_email_context, get_next_url_for_login_page ) -from student.message_types import PasswordReset +from student.message_types import EmailChange, PasswordReset from student.models import ( CourseEnrollment, PasswordHistory, @@ -920,24 +923,32 @@ def do_email_change_request(user, new_email, activation_key=None): pec.activation_key = activation_key pec.save() - context = { - 'key': pec.activation_key, + use_https = theming_helpers.get_current_request().is_secure() + + site = Site.objects.get_current() + message_context = get_base_template_context(site) + message_context.update({ 'old_email': user.email, - 'new_email': pec.new_email - } + 'new_email': pec.new_email, + 'confirm_link': '{protocol}://{site}{link}'.format( + protocol='https' if use_https else 'http', + site=configuration_helpers.get_value('SITE_NAME', settings.SITE_NAME), + link=reverse('confirm_email_change', kwargs={ + 'key': pec.activation_key, + }), + ), + }) - subject = render_to_string('emails/email_change_subject.txt', context) - subject = ''.join(subject.splitlines()) - - message = render_to_string('emails/email_change.txt', context) - - from_address = configuration_helpers.get_value( - 'email_from_address', - settings.DEFAULT_FROM_EMAIL + msg = EmailChange().personalize( + recipient=Recipient(user.username, pec.new_email), + language=preferences_api.get_user_preference(user, LANGUAGE_KEY), + user_context=message_context, ) + try: - mail.send_mail(subject, message, from_address, [pec.new_email]) + ace.send(msg) except Exception: + from_address = configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL) log.error(u'Unable to send email activation link to user from "%s"', from_address, exc_info=True) raise ValueError(_('Unable to send email activation link. Please try again later.')) @@ -948,8 +959,8 @@ def do_email_change_request(user, new_email, activation_key=None): SETTING_CHANGE_INITIATED, { "setting": "email", - "old": context['old_email'], - "new": context['new_email'], + "old": message_context['old_email'], + "new": message_context['new_email'], "user_id": user.id, } ) diff --git a/common/templates/student/edx_ace/emailchange/email/body.html b/common/templates/student/edx_ace/emailchange/email/body.html new file mode 100644 index 0000000000..93cc842e14 --- /dev/null +++ b/common/templates/student/edx_ace/emailchange/email/body.html @@ -0,0 +1,28 @@ +{% extends 'ace_common/edx_ace/common/base_body.html' %} + +{% load i18n %} +{% load static %} +{% block content %} + + + + +
+

+ {% trans "Email Change" %} +

+

+ {% blocktrans %}We received a request to change the e-mail associated with your {{ platform_name }} account from {{ old_email }} to {{ new_email }}. If this is correct, please confirm your new e-mail address by visiting:{% endblocktrans %} +
+

+ + {% trans "Confirm Email Change" as course_cta_text %} + {% include "ace_common/edx_ace/common/return_to_course_cta.html" with course_cta_text=course_cta_text course_cta_url=confirm_link %} + +

+ {% blocktrans %}If you didn't request this, you don't need to do anything; you won't receive any more email from us. Please do not reply to this e-mail; if you require assistance, check the help section of the {{ platform_name }} web site.{% endblocktrans %} +
+

+ +
+{% endblock %} diff --git a/common/templates/student/edx_ace/emailchange/email/body.txt b/common/templates/student/edx_ace/emailchange/email/body.txt new file mode 100644 index 0000000000..34c86d59bf --- /dev/null +++ b/common/templates/student/edx_ace/emailchange/email/body.txt @@ -0,0 +1,6 @@ +{% load i18n %}{% autoescape off %} +{% blocktrans %}We received a request to change the e-mail associated with your {{ platform_name }} account from {{ old_email }} to {{ new_email }}. If this is correct, please confirm your new e-mail address by visiting:{% endblocktrans %} + +{{ confirm_link }} + +{% blocktrans %}If you didn't request this, you don't need to do anything; you won't receive any more email from us. Please do not reply to this e-mail; if you require assistance, check the help section of the {{ platform_name }} web site.{% endblocktrans %}{% endautoescape %} \ No newline at end of file diff --git a/common/templates/student/edx_ace/emailchange/email/from_name.txt b/common/templates/student/edx_ace/emailchange/email/from_name.txt new file mode 100644 index 0000000000..dcbc23c004 --- /dev/null +++ b/common/templates/student/edx_ace/emailchange/email/from_name.txt @@ -0,0 +1 @@ +{{ platform_name }} diff --git a/common/templates/student/edx_ace/emailchange/email/head.html b/common/templates/student/edx_ace/emailchange/email/head.html new file mode 100644 index 0000000000..366ada7ad9 --- /dev/null +++ b/common/templates/student/edx_ace/emailchange/email/head.html @@ -0,0 +1 @@ +{% extends 'ace_common/edx_ace/common/base_head.html' %} diff --git a/common/templates/student/edx_ace/emailchange/email/subject.txt b/common/templates/student/edx_ace/emailchange/email/subject.txt new file mode 100644 index 0000000000..54313b5c8f --- /dev/null +++ b/common/templates/student/edx_ace/emailchange/email/subject.txt @@ -0,0 +1,4 @@ +{% load i18n %} +{% autoescape off %} +{% blocktrans trimmed %}Request to change {{ platform_name }} account e-mail{% endblocktrans %} +{% endautoescape %} diff --git a/lms/templates/emails/email_change.txt b/lms/templates/emails/email_change.txt deleted file mode 100644 index a589369c6f..0000000000 --- a/lms/templates/emails/email_change.txt +++ /dev/null @@ -1,23 +0,0 @@ -## mako -<%! from django.utils.translation import ugettext as _ %> -<%! from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers %> -${_("We received a request to change the e-mail associated with your " - "{platform_name} account from {old_email} to {new_email}. " - "If this is correct, please confirm your new e-mail address by " - "visiting:").format( - platform_name=configuration_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME), - old_email=old_email, - new_email=new_email - ) -} - -% if is_secure: - https://${ site }/email_confirm/${ key } -% else: - http://${ site }/email_confirm/${ key } -% endif - -${_("If you didn't request this, you don't need to do anything; you won't " - "receive any more email from us. Please do not reply to this e-mail; " - "if you require assistance, check the help section of the " - "{platform_name} web site.").format(platform_name=configuration_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME))} 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 74fdd571c5..5c81225196 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -17,6 +17,7 @@ from django.test.client import RequestFactory from mock import Mock, patch from six import iteritems +from openedx.core.djangoapps.ace_common.tests.mixins import EmailTemplateTagMixin 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 ( @@ -64,7 +65,7 @@ def mock_render_to_string(template_name, context): @attr(shard=2) @skip_unless_lms -class TestAccountApi(UserSettingsEventTestMixin, RetirementTestCase): +class TestAccountApi(UserSettingsEventTestMixin, EmailTemplateTagMixin, RetirementTestCase): """ These tests specifically cover the parts of the API methods that are not covered by test_views.py. This includes the specific types of error raised, and default behavior when optional arguments @@ -215,8 +216,10 @@ class TestAccountApi(UserSettingsEventTestMixin, RetirementTestCase): "name": "Mickey Mouse", "email": "seems_ok@sample.com" } - with self.assertRaises(AccountUpdateError) as context_manager: - update_account_settings(self.user, less_naughty_update) + + with patch('crum.get_current_request', return_value=self.fake_request): + with self.assertRaises(AccountUpdateError) as context_manager: + update_account_settings(self.user, less_naughty_update) self.assertIn("Error thrown from do_email_change_request", context_manager.exception.developer_message) # Verify that the name change happened, even though the attempt to send the email failed. @@ -229,6 +232,7 @@ class TestAccountApi(UserSettingsEventTestMixin, RetirementTestCase): Test that email address changes are rejected when ALLOW_EMAIL_ADDRESS_CHANGE is not set. """ disabled_update = {"email": "valid@example.com"} + with self.assertRaises(AccountUpdateError) as context_manager: update_account_settings(self.user, disabled_update) self.assertIn("Email address changes have been disabled", context_manager.exception.developer_message)