diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 5c8f2586a8..6a125170ed 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1671,6 +1671,13 @@ class EntranceExamConfiguration(models.Model): class LanguageProficiency(models.Model): """ Represents a user's language proficiency. + + Note that we have not found a way to emit analytics change events by using signals directly on this + model or on UserProfile. Therefore if you are changing LanguageProficiency values, it is important + to go through the accounts API (AccountsView) defined in + /edx-platform/openedx/core/djangoapps/user_api/accounts/views.py or the AccountLegacyProfileSerializer + in /edx-platform/openedx/core/djangoapps/user_api/accounts/serializers.py so that the events are + emitted. """ class Meta: unique_together = (('code', 'user_profile'),) diff --git a/common/test/acceptance/tests/lms/test_account_settings.py b/common/test/acceptance/tests/lms/test_account_settings.py index 22b153d185..72d37fb41c 100644 --- a/common/test/acceptance/tests/lms/test_account_settings.py +++ b/common/test/acceptance/tests/lms/test_account_settings.py @@ -46,14 +46,14 @@ class AccountSettingsTestMixin(EventsTestMixin, WebAppTest): self.USER_SETTINGS_CHANGED_EVENT_NAME, self.start_time, self.user_id, num_times, setting=setting ) - def verify_settings_changed_events(self, events): + def verify_settings_changed_events(self, events, table=None): """ Verify a particular set of account settings change events were fired. """ expected_referers = [self.ACCOUNT_SETTINGS_REFERER] * len(events) for event in events: event[u'user_id'] = long(self.user_id) - event[u'table'] = u"auth_userprofile" + event[u'table'] = u"auth_userprofile" if table is None else table self.verify_events_of_type(self.USER_SETTINGS_CHANGED_EVENT_NAME, events, expected_referers=expected_referers) @@ -424,6 +424,20 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest): [u'Pushto', u''], ) + self.verify_settings_changed_events( + [{ + u"setting": u"language_proficiencies", + u"old": [], + u"new": [{u"code": u"ps"}], + }, + { + u"setting": u"language_proficiencies", + u"old": [{u"code": u"ps"}], + u"new": [], + }], + table=u"student_languageproficiency" + ) + def test_connected_accounts(self): """ Test that fields for third party auth providers exist. diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 9cf4db14d2..86fbfef1d7 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -6,7 +6,7 @@ from django.core.exceptions import ObjectDoesNotExist from django.conf import settings from django.core.validators import validate_email, validate_slug, ValidationError -from student.models import User, UserProfile, Registration +from student.models import User, UserProfile, Registration, USER_SETTINGS_CHANGED_EVENT_NAME from student import views as student_views from ..errors import ( @@ -24,6 +24,7 @@ from . import ( USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH ) from .serializers import AccountLegacyProfileSerializer, AccountUserSerializer +from eventtracking import tracker @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) @@ -187,9 +188,28 @@ def update_account_settings(requesting_user, update, username=None): try: # If everything validated, go ahead and save the serializers. + + # We have not found a way using signals to get the language proficiency changes (grouped by user). + # As a workaround, store old and new values here and emit them after save is complete. + if "language_proficiencies" in update: + old_language_proficiencies = legacy_profile_serializer.data["language_proficiencies"] + for serializer in user_serializer, legacy_profile_serializer: serializer.save() + if "language_proficiencies" in update: + new_language_proficiencies = legacy_profile_serializer.data["language_proficiencies"] + tracker.emit( + USER_SETTINGS_CHANGED_EVENT_NAME, + { + "setting": "language_proficiencies", + "old": old_language_proficiencies, + "new": new_language_proficiencies, + "user_id": existing_user.id, + "table": existing_user_profile.language_proficiencies.model._meta.db_table, + } + ) + # 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. if old_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 6f820f88ca..4da5770961 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -15,7 +15,7 @@ from student.tests.factories import UserFactory from django.conf import settings from django.contrib.auth.models import User from django.core import mail -from student.models import PendingEmailChange +from student.models import PendingEmailChange, USER_SETTINGS_CHANGED_EVENT_NAME from ...errors import ( UserNotFound, UserNotAuthorized, AccountUpdateError, AccountValidationError, AccountUserAlreadyExists, AccountUsernameInvalid, AccountEmailInvalid, AccountPasswordInvalid, AccountRequestError @@ -24,6 +24,7 @@ from ..api import ( get_account_settings, update_account_settings, create_account, activate_account, request_password_change ) from .. import USERNAME_MAX_LENGTH, EMAIL_MAX_LENGTH, PASSWORD_MAX_LENGTH +from util.testing import EventTestMixin def mock_render_to_string(template_name, context): @@ -32,7 +33,7 @@ def mock_render_to_string(template_name, context): @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS') -class TestAccountApi(TestCase): +class TestAccountApi(EventTestMixin, TestCase): """ 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 @@ -41,7 +42,7 @@ class TestAccountApi(TestCase): password = "test" def setUp(self): - super(TestAccountApi, self).setUp() + super(TestAccountApi, self).setUp('openedx.core.djangoapps.user_api.accounts.api.tracker') self.user = UserFactory.create(password=self.password) self.different_user = UserFactory.create(password=self.password) self.staff_user = UserFactory(is_staff=True, password=self.password) @@ -191,6 +192,31 @@ class TestAccountApi(TestCase): pending_change = PendingEmailChange.objects.filter(user=self.user) self.assertEqual(0, len(pending_change)) + def test_language_proficiency_eventing(self): + """ + Test that eventing of language proficiencies, which happens update_account_settings method, behaves correctly. + """ + def verify_event_emitted(new_value, old_value): + update_account_settings(self.user, {"language_proficiencies": new_value}) + self.assert_event_emitted( + USER_SETTINGS_CHANGED_EVENT_NAME, setting="language_proficiencies", + old=old_value, new=new_value, user_id=self.user.id, table="student_languageproficiency" + ) + self.reset_tracker() + + # First, test that no event is emitted if language_proficiencies is not included. + update_account_settings(self.user, {"year_of_birth": 900}) + account_settings = get_account_settings(self.user) + self.assertEqual(900, account_settings["year_of_birth"]) + self.assert_no_events_were_emitted() + + # New change language_proficiencies and verify events are fired. + verify_event_emitted([{"code": "en"}], []) + verify_event_emitted([{"code": "en"}, {"code": "fr"}], [{"code": "en"}]) + # Note that events are fired even if there has been no actual change. + verify_event_emitted([{"code": "en"}, {"code": "fr"}], [{"code": "en"}, {"code": "fr"}]) + verify_event_emitted([], [{"code": "en"}, {"code": "fr"}]) + @patch('openedx.core.djangoapps.user_api.accounts.image_helpers._PROFILE_IMAGE_SIZES', [50, 10]) @patch.dict( diff --git a/openedx/core/djangoapps/user_api/helpers.py b/openedx/core/djangoapps/user_api/helpers.py index 8fc9b6c69b..296ddece5b 100644 --- a/openedx/core/djangoapps/user_api/helpers.py +++ b/openedx/core/djangoapps/user_api/helpers.py @@ -56,7 +56,7 @@ def intercept_errors(api_error, ignore_errors=None): func_name=func.func_name, args=args, kwargs=kwargs, - exception=repr(ex) + exception=ex.developer_message if hasattr(ex, 'developer_message') else repr(ex) ) LOGGER.warning(msg) raise @@ -70,7 +70,7 @@ def intercept_errors(api_error, ignore_errors=None): func_name=func.func_name, args=args, kwargs=kwargs, - exception=repr(ex) + exception=ex.developer_message if hasattr(ex, 'developer_message') else repr(ex) ) LOGGER.exception(msg) raise api_error(msg)