diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 7f26eac7b1..30b8cde045 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -282,33 +282,6 @@ class UserProfile(models.Model): self.set_meta(meta) self.save() - @transaction.commit_on_success - def update_name(self, new_name): - """Update the user's name, storing the old name in the history. - - Implicitly saves the model. - If the new name is not the same as the old name, do nothing. - - Arguments: - new_name (unicode): The new full name for the user. - - Returns: - None - - """ - if self.name == new_name: - return - - if self.name: - meta = self.get_meta() - if 'old_names' not in meta: - meta['old_names'] = [] - meta['old_names'].append([self.name, u"", datetime.now(UTC).isoformat()]) - self.set_meta(meta) - - self.name = new_name - self.save() - class UserSignupSource(models.Model): """ diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index 3112e4c24c..25783feb87 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -4,7 +4,9 @@ import django.db import unittest from student.tests.factories import UserFactory, RegistrationFactory, PendingEmailChangeFactory -from student.views import reactivation_email_for_user, change_email_request, confirm_email_change +from student.views import ( + reactivation_email_for_user, change_email_request, do_email_change_request, confirm_email_change +) from student.models import UserProfile, PendingEmailChange from django.contrib.auth.models import User, AnonymousUser from django.test import TestCase, TransactionTestCase @@ -174,6 +176,11 @@ class EmailChangeRequestTests(TestCase): self.request.POST['new_email'] = email self.assertFailedRequest(self.run_request(), 'Valid e-mail address required.') + def test_change_email_to_existing_value(self): + """ Test the error message if user attempts to change email to the existing value. """ + self.request.POST['new_email'] = self.user.email + self.assertFailedRequest(self.run_request(), 'Old email is the same as the new email.') + def check_duplicate_email(self, email): """Test that a request to change a users email to `email` fails""" request = self.req_factory.post('unused_url', data={ @@ -192,7 +199,33 @@ class EmailChangeRequestTests(TestCase): UserFactory.create(email=self.new_email) self.check_duplicate_email(self.new_email.capitalize()) - # TODO: Finish testing the rest of change_email_request + @patch('django.core.mail.send_mail') + @patch('student.views.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.request.POST['new_email'] = "valid@email.com" + self.assertFailedRequest(self.run_request(), 'Unable to send email activation link. Please try again later.') + + @patch('django.core.mail.send_mail') + @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) + def test_email_success(self, send_mail): + """ Test email was sent if no errors encountered. """ + old_email = self.user.email + new_email = "valid@example.com" + registration_key = "test registration key" + do_email_change_request(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), + settings.DEFAULT_FROM_EMAIL, + [new_email] + ) @patch('django.contrib.auth.models.User.email_user') diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index bbb3482a9b..d2649b60c4 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -18,7 +18,7 @@ from django.contrib.auth.decorators import login_required from django.contrib.auth.views import password_reset_confirm from django.contrib import messages from django.core.context_processors import csrf -from django.core.mail import send_mail +from django.core import mail from django.core.urlresolvers import reverse from django.core.validators import validate_email, validate_slug, ValidationError from django.db import IntegrityError, transaction @@ -1546,7 +1546,7 @@ def create_account_with_params(request, params): dest_addr = settings.FEATURES['REROUTE_ACTIVATION_EMAIL'] message = ("Activation for %s (%s): %s\n" % (user, user.email, profile.name) + '-' * 80 + '\n\n' + message) - send_mail(subject, message, from_address, [dest_addr], fail_silently=False) + mail.send_mail(subject, message, from_address, [dest_addr], fail_silently=False) else: user.email_user(subject, message, from_address) except Exception: # pylint: disable=broad-except @@ -1916,6 +1916,7 @@ def reactivation_email_for_user(user): return JsonResponse({"success": True}) +# TODO: delete this method and redirect unit tests to do_email_change_request after accounts page work is done. @ensure_csrf_cookie def change_email_request(request): """ AJAX call from the profile page. User wants a new e-mail. @@ -1933,39 +1934,44 @@ def change_email_request(request): }) # TODO: this should be status code 400 # pylint: disable=fixme new_email = request.POST['new_email'] + try: + do_email_change_request(request.user, new_email) + except ValueError as err: + return JsonResponse({ + "success": False, + "error": err.message, + }) + return JsonResponse({"success": True}) + + +def do_email_change_request(user, new_email, activation_key=uuid.uuid4().hex): + """ + Given a new email for a user, does some basic verification of the new address and sends an activation message + to the new address. If any issues are encountered with verification or sending the message, a ValueError will + be thrown. + """ try: validate_email(new_email) except ValidationError: - return JsonResponse({ - "success": False, - "error": _('Valid e-mail address required.'), - }) # TODO: this should be status code 400 # pylint: disable=fixme + raise ValueError(_('Valid e-mail address required.')) + + if new_email == user.email: + raise ValueError(_('Old email is the same as the new email.')) if User.objects.filter(email=new_email).count() != 0: - ## CRITICAL TODO: Handle case sensitivity for e-mails - return JsonResponse({ - "success": False, - "error": _('An account with this e-mail already exists.'), - }) # TODO: this should be status code 400 # pylint: disable=fixme + raise ValueError(_('An account with this e-mail already exists.')) - pec_list = PendingEmailChange.objects.filter(user=request.user) + pec_list = PendingEmailChange.objects.filter(user=user) if len(pec_list) == 0: pec = PendingEmailChange() pec.user = user else: pec = pec_list[0] - pec.new_email = request.POST['new_email'] - pec.activation_key = uuid.uuid4().hex + pec.new_email = new_email + pec.activation_key = activation_key pec.save() - if pec.new_email == user.email: - pec.delete() - return JsonResponse({ - "success": False, - "error": _('Old email is the same as the new email.'), - }) # TODO: this should be status code 400 # pylint: disable=fixme - context = { 'key': pec.activation_key, 'old_email': user.email, @@ -1982,15 +1988,10 @@ def change_email_request(request): settings.DEFAULT_FROM_EMAIL ) try: - send_mail(subject, message, from_address, [pec.new_email]) + mail.send_mail(subject, message, from_address, [pec.new_email]) except Exception: # pylint: disable=broad-except log.error(u'Unable to send email activation link to user from "%s"', from_address, exc_info=True) - return JsonResponse({ - "success": False, - "error": _('Unable to send email activation link. Please try again later.') - }) - - return JsonResponse({"success": True}) + raise ValueError(_('Unable to send email activation link. Please try again later.')) @ensure_csrf_cookie @@ -2059,6 +2060,7 @@ def confirm_email_change(request, key): # pylint: disable=unused-argument raise +# TODO: DELETE AFTER NEW ACCOUNT PAGE DONE @ensure_csrf_cookie @require_POST def change_name_request(request): @@ -2087,45 +2089,7 @@ def change_name_request(request): return JsonResponse({"success": True}) -@ensure_csrf_cookie -def pending_name_changes(request): - """ Web page which allows staff to approve or reject name changes. """ - if not request.user.is_staff: - raise Http404 - - students = [] - for change in PendingNameChange.objects.all(): - profile = UserProfile.objects.get(user=change.user) - students.append({ - "new_name": change.new_name, - "rationale": change.rationale, - "old_name": profile.name, - "email": change.user.email, - "uid": change.user.id, - "cid": change.id, - }) - - return render_to_response("name_changes.html", {"students": students}) - - -@ensure_csrf_cookie -def reject_name_change(request): - """ JSON: Name change process. Course staff clicks 'reject' on a given name change """ - if not request.user.is_staff: - raise Http404 - - try: - pnc = PendingNameChange.objects.get(id=int(request.POST['id'])) - except PendingNameChange.DoesNotExist: - return JsonResponse({ - "success": False, - "error": _('Invalid ID'), - }) # TODO: this should be status code 400 # pylint: disable=fixme - - pnc.delete() - return JsonResponse({"success": True}) - - +# TODO: DELETE AFTER NEW ACCOUNT PAGE DONE def accept_name_change_by_id(uid): """ Accepts the pending name change request for the user represented @@ -2156,20 +2120,6 @@ def accept_name_change_by_id(uid): return JsonResponse({"success": True}) -@ensure_csrf_cookie -def accept_name_change(request): - """ JSON: Name change process. Course staff clicks 'accept' on a given name change - - We used this during the prototype but now we simply record name changes instead - of manually approving them. Still keeping this around in case we want to go - back to this approval method. - """ - if not request.user.is_staff: - raise Http404 - - return accept_name_change_by_id(int(request.POST['id'])) - - @require_POST @login_required @ensure_csrf_cookie diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index 1a8f6537bf..0feb69944a 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -28,6 +28,9 @@ from django.contrib.auth.decorators import login_required from django.core.mail import send_mail from openedx.core.djangoapps.user_api.api import profile as profile_api +from openedx.core.djangoapps.user_api.accounts.views import AccountView +from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH +from openedx.core.djangoapps.user_api.api.account import AccountUserNotFound, AccountUpdateError from course_modes.models import CourseMode from student.models import CourseEnrollment @@ -718,16 +721,13 @@ def submit_photos_for_verification(request): # then try to do that before creating the attempt. if request.POST.get('full_name'): try: - profile_api.update_profile( - username, - full_name=request.POST.get('full_name') - ) - except profile_api.ProfileUserNotFound: + AccountView.update_account(request.user, username, {"name": request.POST.get('full_name')}) + except AccountUserNotFound: return HttpResponseBadRequest(_("No profile found for user")) - except profile_api.ProfileInvalidField: + except AccountUpdateError: msg = _( "Name must be at least {min_length} characters long." - ).format(min_length=profile_api.FULL_NAME_MIN_LENGTH) + ).format(min_length=NAME_MIN_LENGTH) return HttpResponseBadRequest(msg) # Create the attempt diff --git a/lms/templates/name_changes.html b/lms/templates/name_changes.html deleted file mode 100644 index 72d55d3ef0..0000000000 --- a/lms/templates/name_changes.html +++ /dev/null @@ -1,45 +0,0 @@ -<%! from django.utils.translation import ugettext as _ %> - -<%inherit file="main.html" /> - - -
-
-
-

${_("Pending name changes")}

- - % for s in students: - - - - - - - % endfor -
${s['old_name']}${s['new_name']|h}${s['email']|h}${s['rationale']|h}[${_("Confirm")}] - [${_("Reject")}]
-
-
-
diff --git a/lms/urls.py b/lms/urls.py index 0b324ca9aa..acb33a4284 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -28,9 +28,6 @@ urlpatterns = ( url(r'^change_email$', 'student.views.change_email_request', name="change_email"), url(r'^email_confirm/(?P[^/]*)$', 'student.views.confirm_email_change'), url(r'^change_name$', 'student.views.change_name_request', name="change_name"), - url(r'^accept_name_change$', 'student.views.accept_name_change'), - url(r'^reject_name_change$', 'student.views.reject_name_change'), - url(r'^pending_name_changes$', 'student.views.pending_name_changes'), url(r'^event$', 'track.views.user_track'), url(r'^segmentio/event$', 'track.views.segmentio.segmentio_event'), url(r'^t/(?P