Skip to content

E
edx-platform
Commit 3ed31c2b by cahrens
Options

Add name change and email change support to accounts API. 

Name change is immediate, email change is multi-step.
parent 99e85556
Showing with 325 additions and 275 deletions
common/djangoapps/student/models.py
...@@ -282,33 +282,6 @@ class UserProfile(models.Model): ...@@ -282,33 +282,6 @@ class UserProfile(models.Model):
self.set_meta(meta) self.set_meta(meta)
self.save() 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): class UserSignupSource(models.Model):
""" """
......
common/djangoapps/student/tests/test_email.py
...@@ -4,7 +4,9 @@ import django.db ...@@ -4,7 +4,9 @@ import django.db
import unittest import unittest
from student.tests.factories import UserFactory, RegistrationFactory, PendingEmailChangeFactory 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 student.models import UserProfile, PendingEmailChange
from django.contrib.auth.models import User, AnonymousUser from django.contrib.auth.models import User, AnonymousUser
from django.test import TestCase, TransactionTestCase from django.test import TestCase, TransactionTestCase
...@@ -174,6 +176,11 @@ class EmailChangeRequestTests(TestCase): ...@@ -174,6 +176,11 @@ class EmailChangeRequestTests(TestCase):
self.request.POST['new_email'] = email self.request.POST['new_email'] = email
self.assertFailedRequest(self.run_request(), 'Valid e-mail address required.') 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): def check_duplicate_email(self, email):
"""Test that a request to change a users email to `email` fails""" """Test that a request to change a users email to `email` fails"""
request = self.req_factory.post('unused_url', data={ request = self.req_factory.post('unused_url', data={
...@@ -192,7 +199,33 @@ class EmailChangeRequestTests(TestCase): ...@@ -192,7 +199,33 @@ class EmailChangeRequestTests(TestCase):
UserFactory.create(email=self.new_email) UserFactory.create(email=self.new_email)
self.check_duplicate_email(self.new_email.capitalize()) 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') @patch('django.contrib.auth.models.User.email_user')
......
common/djangoapps/student/views.py
...@@ -18,7 +18,7 @@ from django.contrib.auth.decorators import login_required ...@@ -18,7 +18,7 @@ from django.contrib.auth.decorators import login_required
from django.contrib.auth.views import password_reset_confirm from django.contrib.auth.views import password_reset_confirm
from django.contrib import messages from django.contrib import messages
from django.core.context_processors import csrf 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.urlresolvers import reverse
from django.core.validators import validate_email, validate_slug, ValidationError from django.core.validators import validate_email, validate_slug, ValidationError
from django.db import IntegrityError, transaction from django.db import IntegrityError, transaction
...@@ -1546,7 +1546,7 @@ def create_account_with_params(request, params): ...@@ -1546,7 +1546,7 @@ def create_account_with_params(request, params):
dest_addr = settings.FEATURES['REROUTE_ACTIVATION_EMAIL'] dest_addr = settings.FEATURES['REROUTE_ACTIVATION_EMAIL']
message = ("Activation for %s (%s): %s\n" % (user, user.email, profile.name) + message = ("Activation for %s (%s): %s\n" % (user, user.email, profile.name) +
'-' * 80 + '\n\n' + message) '-' * 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: else:
user.email_user(subject, message, from_address) user.email_user(subject, message, from_address)
except Exception: # pylint: disable=broad-except except Exception: # pylint: disable=broad-except
...@@ -1916,6 +1916,7 @@ def reactivation_email_for_user(user): ...@@ -1916,6 +1916,7 @@ def reactivation_email_for_user(user):
return JsonResponse({"success": True}) 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 @ensure_csrf_cookie
def change_email_request(request): def change_email_request(request):
""" AJAX call from the profile page. User wants a new e-mail. """ AJAX call from the profile page. User wants a new e-mail.
...@@ -1934,38 +1935,43 @@ def change_email_request(request): ...@@ -1934,38 +1935,43 @@ def change_email_request(request):
new_email = request.POST['new_email'] new_email = request.POST['new_email']
try: try:
validate_email(new_email) do_email_change_request(request.user, new_email)
except ValidationError: except ValueError as err:
return JsonResponse({ return JsonResponse({
"success": False, "success": False,
"error": _('Valid e-mail address required.'), "error": err.message,
}) # TODO: this should be status code 400 # pylint: disable=fixme })
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:
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: if User.objects.filter(email=new_email).count() != 0:
## CRITICAL TODO: Handle case sensitivity for e-mails raise ValueError(_('An account with this e-mail already exists.'))
return JsonResponse({
"success": False,
"error": _('An account with this e-mail already exists.'),
}) # TODO: this should be status code 400 # pylint: disable=fixme
pec_list = PendingEmailChange.objects.filter(user=request.user) pec_list = PendingEmailChange.objects.filter(user=user)
if len(pec_list) == 0: if len(pec_list) == 0:
pec = PendingEmailChange() pec = PendingEmailChange()
pec.user = user pec.user = user
else: else:
pec = pec_list[0] pec = pec_list[0]
pec.new_email = request.POST['new_email'] pec.new_email = new_email
pec.activation_key = uuid.uuid4().hex pec.activation_key = activation_key
pec.save() 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 = { context = {
'key': pec.activation_key, 'key': pec.activation_key,
'old_email': user.email, 'old_email': user.email,
...@@ -1982,15 +1988,10 @@ def change_email_request(request): ...@@ -1982,15 +1988,10 @@ def change_email_request(request):
settings.DEFAULT_FROM_EMAIL settings.DEFAULT_FROM_EMAIL
) )
try: 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 except Exception: # pylint: disable=broad-except
log.error(u'Unable to send email activation link to user from "%s"', from_address, exc_info=True) log.error(u'Unable to send email activation link to user from "%s"', from_address, exc_info=True)
return JsonResponse({ raise ValueError(_('Unable to send email activation link. Please try again later.'))
"success": False,
"error": _('Unable to send email activation link. Please try again later.')
})
return JsonResponse({"success": True})
@ensure_csrf_cookie @ensure_csrf_cookie
...@@ -2059,6 +2060,7 @@ def confirm_email_change(request, key): # pylint: disable=unused-argument ...@@ -2059,6 +2060,7 @@ def confirm_email_change(request, key): # pylint: disable=unused-argument
raise raise
# TODO: DELETE AFTER NEW ACCOUNT PAGE DONE
@ensure_csrf_cookie @ensure_csrf_cookie
@require_POST @require_POST
def change_name_request(request): def change_name_request(request):
...@@ -2087,45 +2089,7 @@ def change_name_request(request): ...@@ -2087,45 +2089,7 @@ def change_name_request(request):
return JsonResponse({"success": True}) return JsonResponse({"success": True})
@ensure_csrf_cookie # TODO: DELETE AFTER NEW ACCOUNT PAGE DONE
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})
def accept_name_change_by_id(uid): def accept_name_change_by_id(uid):
""" """
Accepts the pending name change request for the user represented Accepts the pending name change request for the user represented
...@@ -2156,20 +2120,6 @@ def accept_name_change_by_id(uid): ...@@ -2156,20 +2120,6 @@ def accept_name_change_by_id(uid):
return JsonResponse({"success": True}) 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 @require_POST
@login_required @login_required
@ensure_csrf_cookie @ensure_csrf_cookie
......
lms/djangoapps/verify_student/views.py
...@@ -28,6 +28,9 @@ from django.contrib.auth.decorators import login_required ...@@ -28,6 +28,9 @@ from django.contrib.auth.decorators import login_required
from django.core.mail import send_mail 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.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 course_modes.models import CourseMode
from student.models import CourseEnrollment from student.models import CourseEnrollment
...@@ -718,16 +721,13 @@ def submit_photos_for_verification(request): ...@@ -718,16 +721,13 @@ def submit_photos_for_verification(request):
# then try to do that before creating the attempt. # then try to do that before creating the attempt.
if request.POST.get('full_name'): if request.POST.get('full_name'):
try: try:
profile_api.update_profile( AccountView.update_account(request.user, username, {"name": request.POST.get('full_name')})
username, except AccountUserNotFound:
full_name=request.POST.get('full_name')
)
except profile_api.ProfileUserNotFound:
return HttpResponseBadRequest(_("No profile found for user")) return HttpResponseBadRequest(_("No profile found for user"))
except profile_api.ProfileInvalidField: except AccountUpdateError:
msg = _( msg = _(
"Name must be at least {min_length} characters long." "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) return HttpResponseBadRequest(msg)
# Create the attempt # Create the attempt
......
lms/templates/name_changes.html deleted 100644 → 0
<%! from django.utils.translation import ugettext as _ %>
<%inherit file="main.html" />
<script>
function name_confirm(id) {
$.post('/accept_name_change',{"id":id},
function(data){
if(data.success){
$("#div"+id).html("${_('Accepted')}");
} else {
alert("${_('Error')}");
}
});
}
function name_deny(id) {
$.post('/reject_name_change',{"id":id},
function(data){
if(data.success){
$("#div"+id).html("${_('Rejected')}");
} else {
alert("${_('Error')}");
}
});
}
</script>
<section class="container">
<div class="gradebook-wrapper">
<section class="gradebook-content">
<h1>${_("Pending name changes")}</h1>
<table>
% for s in students:
<tr>
<td><a href="/profile/${s['uid']}"/>${s['old_name']}</td>
<td>${s['new_name']|h}</td>
<td>${s['email']|h}</td>
<td>${s['rationale']|h}</td>
<td><span id="div${s['cid']}"><span onclick="name_confirm(${s['cid']});">[${_("Confirm")}]</span>
<span onclick="name_deny(${s['cid']});">[${_("Reject")}]</span></span></td></tr>
% endfor
</table>
</section>
</div>
</section>
lms/urls.py
...@@ -26,9 +26,6 @@ urlpatterns = ('', # nopep8 ...@@ -26,9 +26,6 @@ urlpatterns = ('', # nopep8
url(r'^change_email$', 'student.views.change_email_request', name="change_email"), url(r'^change_email$', 'student.views.change_email_request', name="change_email"),
url(r'^email_confirm/(?P<key>[^/]*)$', 'student.views.confirm_email_change'), url(r'^email_confirm/(?P<key>[^/]*)$', 'student.views.confirm_email_change'),
url(r'^change_name$', 'student.views.change_name_request', name="change_name"), 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'^event$', 'track.views.user_track'),
url(r'^segmentio/event$', 'track.views.segmentio.segmentio_event'), url(r'^segmentio/event$', 'track.views.segmentio.segmentio_event'),
url(r'^t/(?P<template>[^/]*)$', 'static_template_view.views.index'), # TODO: Is this used anymore? What is STATIC_GRAB? url(r'^t/(?P<template>[^/]*)$', 'static_template_view.views.index'), # TODO: Is this used anymore? What is STATIC_GRAB?
......
openedx/core/djangoapps/user_api/accounts/__init__.py
# The minimum acceptable length for the name account field
NAME_MIN_LENGTH = 2
openedx/core/djangoapps/user_api/accounts/serializers.py
from rest_framework import serializers from rest_framework import serializers
from django.contrib.auth.models import User from django.contrib.auth.models import User
from student.models import UserProfile from student.models import UserProfile
from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH
class AccountUserSerializer(serializers.HyperlinkedModelSerializer): class AccountUserSerializer(serializers.HyperlinkedModelSerializer):
...@@ -22,7 +23,20 @@ class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer): ...@@ -22,7 +23,20 @@ class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer):
fields = ( fields = (
"name", "gender", "goals", "year_of_birth", "level_of_education", "language", "country", "mailing_address" "name", "gender", "goals", "year_of_birth", "level_of_education", "language", "country", "mailing_address"
) )
read_only_fields = ("name",) # Currently no read-only field, but keep this so view code doesn't need to know.
read_only_fields = ()
def validate_name(self, attrs, source):
""" Enforce minimum length for name. """
if source in attrs:
new_name = attrs[source].strip()
if len(new_name) < NAME_MIN_LENGTH:
raise serializers.ValidationError(
"The name field must be at least {} characters long.".format(NAME_MIN_LENGTH)
)
attrs[source] = new_name
return attrs
def transform_gender(self, obj, value): def transform_gender(self, obj, value):
""" Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """ """ Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """
......
openedx/core/djangoapps/user_api/accounts/tests/test_views.py
# -*- coding: utf-8 -*-
import unittest import unittest
import ddt import ddt
import json import json
...@@ -9,7 +10,8 @@ from django.conf import settings ...@@ -9,7 +10,8 @@ from django.conf import settings
from rest_framework.test import APITestCase, APIClient from rest_framework.test import APITestCase, APIClient
from student.tests.factories import UserFactory from student.tests.factories import UserFactory
from student.models import UserProfile from student.models import UserProfile, PendingEmailChange
from student.views import confirm_email_change
TEST_PASSWORD = "test" TEST_PASSWORD = "test"
...@@ -45,6 +47,19 @@ class TestAccountAPI(APITestCase): ...@@ -45,6 +47,19 @@ class TestAccountAPI(APITestCase):
self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
self.send_get(self.different_client, expected_status=404) self.send_get(self.different_client, expected_status=404)
@ddt.data(
("client", "user"),
("staff_client", "staff_user"),
)
@ddt.unpack
def test_get_account_unknown_user(self, api_client, user):
"""
Test that requesting a user who does not exist returns a 404.
"""
client = self.login_client(api_client, user)
response = client.get(reverse("accounts_api", kwargs={'username': "does_not_exist"}))
self.assertEqual(404, response.status_code)
def test_get_account_default(self): def test_get_account_default(self):
""" """
Test that a client (logged in) can get her own account information (using default legacy profile information, Test that a client (logged in) can get her own account information (using default legacy profile information,
...@@ -116,6 +131,35 @@ class TestAccountAPI(APITestCase): ...@@ -116,6 +131,35 @@ class TestAccountAPI(APITestCase):
for empty_field in ("level_of_education", "gender", "country"): for empty_field in ("level_of_education", "gender", "country"):
self.assertIsNone(response.data[empty_field]) self.assertIsNone(response.data[empty_field])
def test_patch_account_anonymous_user(self):
"""
Test that an anonymous client (not logged in) cannot call patch.
"""
self.send_patch(self.anonymous_client, {}, expected_status=401)
def test_patch_account_different_user(self):
"""
Test that a client (logged in) cannot update the account information for a different client.
"""
self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD)
self.send_patch(self.different_client, {}, expected_status=404)
@ddt.data(
("client", "user"),
("staff_client", "staff_user"),
)
@ddt.unpack
def test_patch_account_unknown_user(self, api_client, user):
"""
Test that trying to update a user who does not exist returns a 404.
"""
client = self.login_client(api_client, user)
response = client.patch(
reverse("accounts_api", kwargs={'username': "does_not_exist"}),
data=json.dumps({}), content_type="application/merge-patch+json"
)
self.assertEqual(404, response.status_code)
@ddt.data( @ddt.data(
( (
"client", "user", "gender", "f", "not a gender", "client", "user", "gender", "f", "not a gender",
...@@ -127,12 +171,15 @@ class TestAccountAPI(APITestCase): ...@@ -127,12 +171,15 @@ class TestAccountAPI(APITestCase):
), ),
("client", "user", "country", "GB", "XY", "Select a valid choice. XY is not one of the available choices."), ("client", "user", "country", "GB", "XY", "Select a valid choice. XY is not one of the available choices."),
("client", "user", "year_of_birth", 2009, "not_an_int", "Enter a whole number."), ("client", "user", "year_of_birth", 2009, "not_an_int", "Enter a whole number."),
("client", "user", "name", "bob", "z"*256, "Ensure this value has at most 255 characters (it has 256)."),
("client", "user", "name", u"ȻħȺɍłɇs", "z ", "The name field must be at least 2 characters long."),
("client", "user", "language", "Creole"), ("client", "user", "language", "Creole"),
("client", "user", "goals", "Smell the roses"), ("client", "user", "goals", "Smell the roses"),
("client", "user", "mailing_address", "Sesame Street"), ("client", "user", "mailing_address", "Sesame Street"),
# All of the fields can be edited by is_staff, but iterating through all of them again seems like overkill. # All of the fields can be edited by is_staff, but iterating through all of them again seems like overkill.
# Just test a representative field. # Just test a representative field.
("staff_client", "staff_user", "goals", "Smell the roses"), ("staff_client", "staff_user", "goals", "Smell the roses"),
# Note that email is tested below, as it is not immediately updated.
) )
@ddt.unpack @ddt.unpack
def test_patch_account( def test_patch_account(
...@@ -183,7 +230,7 @@ class TestAccountAPI(APITestCase): ...@@ -183,7 +230,7 @@ class TestAccountAPI(APITestCase):
"Field '{0}' cannot be edited.".format(field_name), data["field_errors"][field_name]["user_message"] "Field '{0}' cannot be edited.".format(field_name), data["field_errors"][field_name]["user_message"]
) )
for field_name in ["username", "email", "date_joined", "name"]: for field_name in ["username", "date_joined"]:
response = self.send_patch(client, {field_name: "will_error", "gender": "f"}, expected_status=400) response = self.send_patch(client, {field_name: "will_error", "gender": "f"}, expected_status=400)
verify_error_response(field_name, response.data) verify_error_response(field_name, response.data)
...@@ -192,10 +239,10 @@ class TestAccountAPI(APITestCase): ...@@ -192,10 +239,10 @@ class TestAccountAPI(APITestCase):
self.assertEqual("m", response.data["gender"]) self.assertEqual("m", response.data["gender"])
# Test error message with multiple read-only items # Test error message with multiple read-only items
response = self.send_patch(client, {"username": "will_error", "email": "xx"}, expected_status=400) response = self.send_patch(client, {"username": "will_error", "date_joined": "xx"}, expected_status=400)
self.assertEqual(2, len(response.data["field_errors"])) self.assertEqual(2, len(response.data["field_errors"]))
verify_error_response("username", response.data) verify_error_response("username", response.data)
verify_error_response("email", response.data) verify_error_response("date_joined", response.data)
def test_patch_bad_content_type(self): def test_patch_bad_content_type(self):
""" """
...@@ -223,6 +270,84 @@ class TestAccountAPI(APITestCase): ...@@ -223,6 +270,84 @@ class TestAccountAPI(APITestCase):
response = self.send_get(self.client) response = self.send_get(self.client)
self.assertIsNone(response.data[field_name]) self.assertIsNone(response.data[field_name])
def test_patch_name_metadata(self):
"""
Test the metadata stored when changing the name field.
"""
def get_name_change_info(expected_entries):
legacy_profile = UserProfile.objects.get(id=self.user.id)
name_change_info = legacy_profile.get_meta()["old_names"]
self.assertEqual(expected_entries, len(name_change_info))
return name_change_info
def verify_change_info(change_info, old_name, requester, new_name):
self.assertEqual(3, len(change_info))
self.assertEqual(old_name, change_info[0])
self.assertEqual("Name change requested through account API by {}".format(requester), change_info[1])
self.assertIsNotNone(change_info[2])
# Verify the new name was also stored.
get_response = self.send_get(self.client)
self.assertEqual(new_name, get_response.data["name"])
self.client.login(username=self.user.username, password=TEST_PASSWORD)
legacy_profile = UserProfile.objects.get(id=self.user.id)
self.assertEqual({}, legacy_profile.get_meta())
old_name = legacy_profile.name
# First change the name as the user and verify meta information.
self.send_patch(self.client, {"name": "Mickey Mouse"})
name_change_info = get_name_change_info(1)
verify_change_info(name_change_info[0], old_name, self.user.username, "Mickey Mouse")
# Now change the name as a different (staff) user and verify meta information.
self.staff_client.login(username=self.staff_user.username, password=TEST_PASSWORD)
self.send_patch(self.staff_client, {"name": "Donald Duck"})
name_change_info = get_name_change_info(2)
verify_change_info(name_change_info[0], old_name, self.user.username, "Donald Duck",)
verify_change_info(name_change_info[1], "Mickey Mouse", self.staff_user.username, "Donald Duck")
@ddt.data(
("client", "user"),
("staff_client", "staff_user"),
)
@ddt.unpack
def test_patch_email(self, api_client, user):
"""
Test that the user (and anyone with an is_staff account) can request an email change through the accounts API.
Full testing of the helper method used (do_email_change_request) exists in the package with the code.
Here just do minimal smoke testing.
"""
client = self.login_client(api_client, user)
old_email = self.user.email
new_email = "newemail@example.com"
self.send_patch(client, {"email": new_email, "goals": "change my email"})
# Since request is multi-step, the email won't change on GET immediately (though goals will update).
get_response = self.send_get(client)
self.assertEqual(old_email, get_response.data["email"])
self.assertEqual("change my email", get_response.data["goals"])
# Now call the method that will be invoked with the user clicks the activation key in the received email.
# First we must get the activation key that was sent.
pending_change = PendingEmailChange.objects.filter(user=self.user)
self.assertEqual(1, len(pending_change))
activation_key = pending_change[0].activation_key
confirm_change_url = reverse(
"student.views.confirm_email_change", kwargs={'key': activation_key}
)
response = self.client.post(confirm_change_url)
self.assertEqual(200, response.status_code)
get_response = self.send_get(client)
self.assertEqual(new_email, get_response.data["email"])
# Finally, try changing to an invalid email just to make sure error messages are appropriately returned.
error_response = self.send_patch(client, {"email": "not_an_email"}, expected_status=400)
self.assertEqual(
"Error thrown from do_email_change_request: 'Valid e-mail address required.'",
error_response.data["developer_message"]
)
self.assertEqual("Valid e-mail address required.", error_response.data["user_message"])
def login_client(self, api_client, user): def login_client(self, api_client, user):
"""Helper method for getting the client and user and logging in. Returns client. """ """Helper method for getting the client and user and logging in. Returns client. """
client = getattr(self, api_client) client = getattr(self, api_client)
......
openedx/core/djangoapps/user_api/accounts/views.py
...@@ -7,6 +7,8 @@ https://openedx.atlassian.net/wiki/display/TNL/User+API ...@@ -7,6 +7,8 @@ https://openedx.atlassian.net/wiki/display/TNL/User+API
from django.core.exceptions import ObjectDoesNotExist from django.core.exceptions import ObjectDoesNotExist
from django.contrib.auth.models import User from django.contrib.auth.models import User
from django.utils.translation import ugettext as _ from django.utils.translation import ugettext as _
import datetime
from pytz import UTC
from rest_framework.views import APIView from rest_framework.views import APIView
from rest_framework.response import Response from rest_framework.response import Response
...@@ -16,9 +18,12 @@ from rest_framework import permissions ...@@ -16,9 +18,12 @@ from rest_framework import permissions
from rest_framework import parsers from rest_framework import parsers
from student.models import UserProfile from student.models import UserProfile
from student.views import do_email_change_request
from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH
from openedx.core.djangoapps.user_api.accounts.serializers import AccountLegacyProfileSerializer, AccountUserSerializer from openedx.core.djangoapps.user_api.accounts.serializers import AccountLegacyProfileSerializer, AccountUserSerializer
from openedx.core.lib.api.permissions import IsUserInUrlOrStaff from openedx.core.lib.api.permissions import IsUserInUrlOrStaff
from openedx.core.lib.api.parsers import MergePatchParser from openedx.core.lib.api.parsers import MergePatchParser
from openedx.core.djangoapps.user_api.api.account import AccountUserNotFound, AccountUpdateError
class AccountView(APIView): class AccountView(APIView):
...@@ -37,9 +42,10 @@ class AccountView(APIView): ...@@ -37,9 +42,10 @@ class AccountView(APIView):
* username: username associated with the account (not editable) * username: username associated with the account (not editable)
* name: full name of the user (not editable through this API) * name: full name of the user (must be at least two characters)
* email: email for the user (not editable through this API) * email: email for the user (the new email address must be confirmed via a confirmation email, so GET will
not reflect the change until the address has been confirmed)
* date_joined: date this account was created (not editable), in the string format provided by * date_joined: date this account was created (not editable), in the string format provided by
datetime (for example, "2014-08-26T17:52:11Z") datetime (for example, "2014-08-26T17:52:11Z")
...@@ -82,7 +88,11 @@ class AccountView(APIView): ...@@ -82,7 +88,11 @@ class AccountView(APIView):
""" """
GET /api/user/v0/accounts/{username}/ GET /api/user/v0/accounts/{username}/
""" """
existing_user, existing_user_profile = self._get_user_and_profile(username) try:
existing_user, existing_user_profile = self._get_user_and_profile(username)
except AccountUserNotFound:
return Response(status=status.HTTP_404_NOT_FOUND)
user_serializer = AccountUserSerializer(existing_user) user_serializer = AccountUserSerializer(existing_user)
legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile) legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile)
...@@ -96,10 +106,50 @@ class AccountView(APIView): ...@@ -96,10 +106,50 @@ class AccountView(APIView):
https://tools.ietf.org/html/rfc7396. The content_type must be "application/merge-patch+json" or https://tools.ietf.org/html/rfc7396. The content_type must be "application/merge-patch+json" or
else an error response with status code 415 will be returned. else an error response with status code 415 will be returned.
""" """
existing_user, existing_user_profile = self._get_user_and_profile(username) try:
AccountView.update_account(request.user, username, request.DATA)
except AccountUserNotFound:
return Response(status=status.HTTP_404_NOT_FOUND)
except AccountUpdateError as err:
return Response(err.error_info, status=status.HTTP_400_BAD_REQUEST)
return Response(status=status.HTTP_204_NO_CONTENT)
@staticmethod
def update_account(requesting_user, username, update):
"""Update the account for the given username.
Note:
No authorization or permissions checks are done in this method. It is up to the caller
of this method to enforce the contract that this method is only called if
requesting_user.username == username or requesting_user.is_staff == True.
Arguments:
requesting_user (User): the user who initiated the request
username (string): the username associated with the account to change
update (dict): the updated account field values
Raises:
AccountUserNotFound: no user exists with the specified username
AccountUpdateError: the update could not be completed, usually due to validation errors
(for example, read-only fields were specified or field values are not legal)
"""
existing_user, existing_user_profile = AccountView._get_user_and_profile(username)
# If user has requested to change email, we must call the multi-step process to handle this.
# It is not handled by the serializer (which considers email to be read-only).
new_email = None
if "email" in update:
new_email = update["email"]
del update["email"]
# If user has requested to change name, store old name because we must update associated metadata
# after the save process is complete.
old_name = None
if "name" in update:
old_name = existing_user_profile.name
# Check for fields that are not editable. Marking them read-only causes them to be ignored, but we wish to 400. # Check for fields that are not editable. Marking them read-only causes them to be ignored, but we wish to 400.
update = request.DATA
read_only_fields = set(update.keys()).intersection( read_only_fields = set(update.keys()).intersection(
AccountUserSerializer.Meta.read_only_fields + AccountLegacyProfileSerializer.Meta.read_only_fields AccountUserSerializer.Meta.read_only_fields + AccountLegacyProfileSerializer.Meta.read_only_fields
) )
...@@ -110,33 +160,57 @@ class AccountView(APIView): ...@@ -110,33 +160,57 @@ class AccountView(APIView):
"developer_message": "This field is not editable via this API", "developer_message": "This field is not editable via this API",
"user_message": _("Field '{field_name}' cannot be edited.".format(field_name=read_only_field)) "user_message": _("Field '{field_name}' cannot be edited.".format(field_name=read_only_field))
} }
response_data = {"field_errors": field_errors} raise AccountUpdateError({"field_errors": field_errors})
return Response(response_data, status=status.HTTP_400_BAD_REQUEST)
# If the user asked to change email, send the request now.
if new_email:
try:
do_email_change_request(existing_user, new_email)
except ValueError as err:
response_data = {
"developer_message": "Error thrown from do_email_change_request: '{}'".format(err.message),
"user_message": err.message
}
raise AccountUpdateError(response_data)
user_serializer = AccountUserSerializer(existing_user, data=update) user_serializer = AccountUserSerializer(existing_user, data=update)
legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile, data=update) legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile, data=update)
for serializer in user_serializer, legacy_profile_serializer: for serializer in user_serializer, legacy_profile_serializer:
validation_errors = self._get_validation_errors(update, serializer) validation_errors = AccountView._get_validation_errors(update, serializer)
if validation_errors: if validation_errors:
return Response(validation_errors, status=status.HTTP_400_BAD_REQUEST) raise AccountUpdateError(validation_errors)
serializer.save() serializer.save()
return Response(status=status.HTTP_204_NO_CONTENT) # If the name was changed, store information about the change operation. This is outside of the
# serializer so that we can store who requested the change.
def _get_user_and_profile(self, username): if old_name:
meta = existing_user_profile.get_meta()
if 'old_names' not in meta:
meta['old_names'] = []
meta['old_names'].append([
old_name,
"Name change requested through account API by {0}".format(requesting_user.username),
datetime.datetime.now(UTC).isoformat()
])
existing_user_profile.set_meta(meta)
existing_user_profile.save()
@staticmethod
def _get_user_and_profile(username):
""" """
Helper method to return the legacy user and profile objects based on username. Helper method to return the legacy user and profile objects based on username.
""" """
try: try:
existing_user = User.objects.get(username=username) existing_user = User.objects.get(username=username)
existing_user_profile = UserProfile.objects.get(user=existing_user)
except ObjectDoesNotExist: except ObjectDoesNotExist:
return Response({}, status=status.HTTP_404_NOT_FOUND) raise AccountUserNotFound()
existing_user_profile = UserProfile.objects.get(user=existing_user)
return existing_user, existing_user_profile return existing_user, existing_user_profile
def _get_validation_errors(self, update, serializer): @staticmethod
def _get_validation_errors(update, serializer):
""" """
Helper method that returns any validation errors that are present. Helper method that returns any validation errors that are present.
""" """
...@@ -157,4 +231,4 @@ class AccountView(APIView): ...@@ -157,4 +231,4 @@ class AccountView(APIView):
} }
validation_errors['field_errors'] = field_errors validation_errors['field_errors'] = field_errors
return validation_errors return validation_errors
\ No newline at end of file
openedx/core/djangoapps/user_api/api/account.py
...@@ -40,11 +40,6 @@ class AccountUserAlreadyExists(AccountRequestError): ...@@ -40,11 +40,6 @@ class AccountUserAlreadyExists(AccountRequestError):
pass pass
class AccountUsernameAlreadyExists(AccountUserAlreadyExists):
"""An account already exists with the requested username. """
pass
class AccountUsernameInvalid(AccountRequestError): class AccountUsernameInvalid(AccountRequestError):
"""The requested username is not in a valid format. """ """The requested username is not in a valid format. """
pass pass
...@@ -70,6 +65,15 @@ class AccountNotAuthorized(AccountRequestError): ...@@ -70,6 +65,15 @@ class AccountNotAuthorized(AccountRequestError):
pass pass
class AccountUpdateError(AccountRequestError):
"""
An update to the account failed. More detailed information is present in error_info (a dict
with at least a developer_message, though possibly also a nested field_errors dict).
"""
def __init__(self, error_info):
self.error_info = error_info
@intercept_errors(AccountInternalError, ignore_errors=[AccountRequestError]) @intercept_errors(AccountInternalError, ignore_errors=[AccountRequestError])
@transaction.commit_on_success @transaction.commit_on_success
def create_account(username, password, email): def create_account(username, password, email):
...@@ -211,6 +215,7 @@ def activate_account(activation_key): ...@@ -211,6 +215,7 @@ def activate_account(activation_key):
# This implicitly saves the registration # This implicitly saves the registration
registration.activate() registration.activate()
@intercept_errors(AccountInternalError, ignore_errors=[AccountRequestError]) @intercept_errors(AccountInternalError, ignore_errors=[AccountRequestError])
def request_password_change(email, orig_host, is_secure): def request_password_change(email, orig_host, is_secure):
"""Email a single-use link for performing a password reset. """Email a single-use link for performing a password reset.
......
openedx/core/djangoapps/user_api/api/profile.py
...@@ -50,10 +50,6 @@ class ProfileInternalError(Exception): ...@@ -50,10 +50,6 @@ class ProfileInternalError(Exception):
pass pass
FULL_NAME_MAX_LENGTH = 255
FULL_NAME_MIN_LENGTH = 2
@intercept_errors(ProfileInternalError, ignore_errors=[ProfileRequestError]) @intercept_errors(ProfileInternalError, ignore_errors=[ProfileRequestError])
def profile_info(username): def profile_info(username):
"""Retrieve a user's profile information. """Retrieve a user's profile information.
...@@ -91,36 +87,6 @@ def profile_info(username): ...@@ -91,36 +87,6 @@ def profile_info(username):
@intercept_errors(ProfileInternalError, ignore_errors=[ProfileRequestError]) @intercept_errors(ProfileInternalError, ignore_errors=[ProfileRequestError])
def update_profile(username, full_name=None):
"""Update a user's profile.
Arguments:
username (unicode): The username associated with the account.
Keyword Arguments:
full_name (unicode): If provided, set the user's full name to this value.
Returns:
None
Raises:
ProfileRequestError: If there is no profile matching the provided username.
"""
try:
profile = UserProfile.objects.get(user__username=username)
except UserProfile.DoesNotExist:
raise ProfileUserNotFound
if full_name is not None:
name_length = len(full_name)
if name_length > FULL_NAME_MAX_LENGTH or name_length < FULL_NAME_MIN_LENGTH:
raise ProfileInvalidField("full_name", full_name)
else:
profile.update_name(full_name)
@intercept_errors(ProfileInternalError, ignore_errors=[ProfileRequestError])
def preference_info(username): def preference_info(username):
"""Retrieve information about a user's preferences. """Retrieve information about a user's preferences.
......
openedx/core/djangoapps/user_api/tests/test_profile_api.py
...@@ -41,56 +41,10 @@ class ProfileApiTest(ModuleStoreTestCase): ...@@ -41,56 +41,10 @@ class ProfileApiTest(ModuleStoreTestCase):
'city': None, 'city': None,
}) })
def test_update_full_name(self):
account_api.create_account(self.USERNAME, self.PASSWORD, self.EMAIL)
profile_api.update_profile(self.USERNAME, full_name=u'ȻħȺɍłɇs')
profile = profile_api.profile_info(self.USERNAME)
self.assertEqual(profile['full_name'], u'ȻħȺɍłɇs')
@raises(profile_api.ProfileInvalidField)
@ddt.data('', 'a', 'a' * profile_api.FULL_NAME_MAX_LENGTH + 'a')
def test_update_full_name_invalid(self, invalid_name):
account_api.create_account(self.USERNAME, self.PASSWORD, self.EMAIL)
profile_api.update_profile(self.USERNAME, full_name=invalid_name)
@raises(profile_api.ProfileUserNotFound)
def test_update_profile_no_user(self):
profile_api.update_profile(self.USERNAME, full_name='test')
def test_retrieve_profile_no_user(self): def test_retrieve_profile_no_user(self):
profile = profile_api.profile_info('does not exist') profile = profile_api.profile_info('does not exist')
self.assertIs(profile, None) self.assertIs(profile, None)
def test_record_name_change_history(self):
account_api.create_account(self.USERNAME, self.PASSWORD, self.EMAIL)
# Change the name once
# Since the original name was an empty string, expect that the list
# of old names is empty
profile_api.update_profile(self.USERNAME, full_name='new name')
meta = UserProfile.objects.get(user__username=self.USERNAME).get_meta()
self.assertEqual(meta, {})
# Change the name again and expect the new name is stored in the history
profile_api.update_profile(self.USERNAME, full_name='another new name')
meta = UserProfile.objects.get(user__username=self.USERNAME).get_meta()
self.assertEqual(len(meta['old_names']), 1)
name, rationale, timestamp = meta['old_names'][0]
self.assertEqual(name, 'new name')
self.assertEqual(rationale, u'')
self._assert_is_datetime(timestamp)
# Change the name a third time and expect both names are stored in the history
profile_api.update_profile(self.USERNAME, full_name='yet another new name')
meta = UserProfile.objects.get(user__username=self.USERNAME).get_meta()
self.assertEqual(len(meta['old_names']), 2)
name, rationale, timestamp = meta['old_names'][1]
self.assertEqual(name, 'another new name')
self.assertEqual(rationale, u'')
self._assert_is_datetime(timestamp)
def test_update_and_retrieve_preference_info(self): def test_update_and_retrieve_preference_info(self):
account_api.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) account_api.create_account(self.USERNAME, self.PASSWORD, self.EMAIL)
......
openedx/core/djangoapps/user_api/tests/test_views.py
...@@ -27,6 +27,7 @@ from ..api import account as account_api, profile as profile_api ...@@ -27,6 +27,7 @@ from ..api import account as account_api, profile as profile_api
from ..models import UserOrgTag from ..models import UserOrgTag
from ..tests.factories import UserPreferenceFactory from ..tests.factories import UserPreferenceFactory
from ..tests.test_constants import SORTED_COUNTRIES from ..tests.test_constants import SORTED_COUNTRIES
from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH
TEST_API_KEY = "test_api_key" TEST_API_KEY = "test_api_key"
...@@ -840,7 +841,7 @@ class RegistrationViewTest(ApiTestCase): ...@@ -840,7 +841,7 @@ class RegistrationViewTest(ApiTestCase):
u"label": u"Full name", u"label": u"Full name",
u"instructions": u"The name that will appear on your certificates", u"instructions": u"The name that will appear on your certificates",
u"restrictions": { u"restrictions": {
"max_length": profile_api.FULL_NAME_MAX_LENGTH, "max_length": NAME_MIN_LENGTH,
}, },
} }
) )
...@@ -920,7 +921,7 @@ class RegistrationViewTest(ApiTestCase): ...@@ -920,7 +921,7 @@ class RegistrationViewTest(ApiTestCase):
u"label": u"Full name", u"label": u"Full name",
u"instructions": u"The name that will appear on your certificates", u"instructions": u"The name that will appear on your certificates",
u"restrictions": { u"restrictions": {
"max_length": profile_api.FULL_NAME_MAX_LENGTH "max_length": NAME_MIN_LENGTH
} }
} }
) )
......
openedx/core/djangoapps/user_api/views.py
...@@ -33,6 +33,8 @@ from .helpers import FormDescription, shim_student_view, require_post_params ...@@ -33,6 +33,8 @@ from .helpers import FormDescription, shim_student_view, require_post_params
from .models import UserPreference, UserProfile from .models import UserPreference, UserProfile
from .serializers import UserSerializer, UserPreferenceSerializer from .serializers import UserSerializer, UserPreferenceSerializer
from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH
class LoginSessionView(APIView): class LoginSessionView(APIView):
"""HTTP end-points for logging in users. """ """HTTP end-points for logging in users. """
...@@ -350,7 +352,7 @@ class RegistrationView(APIView): ...@@ -350,7 +352,7 @@ class RegistrationView(APIView):
label=name_label, label=name_label,
instructions=name_instructions, instructions=name_instructions,
restrictions={ restrictions={
"max_length": profile_api.FULL_NAME_MAX_LENGTH, "max_length": NAME_MIN_LENGTH,
}, },
required=required required=required
) )
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment