diff --git a/common/djangoapps/util/model_utils.py b/common/djangoapps/util/model_utils.py index 4fed1ec97f..a3e3aa11a1 100644 --- a/common/djangoapps/util/model_utils.py +++ b/common/djangoapps/util/model_utils.py @@ -3,6 +3,7 @@ Utilities for django models. """ from eventtracking import tracker +from django.conf import settings from django.core.exceptions import ObjectDoesNotExist from django.db.models.fields.related import RelatedField @@ -46,9 +47,7 @@ def get_changed_fields_dict(instance, model_class): def emit_field_changed_events(instance, user, event_name, db_table, excluded_fields=None, hidden_fields=None): - """ - For the given model instance, emit a setting changed event the fields that - have changed since the last save. + """Emits an event for each field that has changed. Note that this function expects that a `_changed_fields` dict has been set as an attribute on `instance` (see `get_changed_fields_dict`. @@ -87,16 +86,61 @@ def emit_field_changed_events(instance, user, event_name, db_table, excluded_fie changed_fields = getattr(instance, '_changed_fields', {}) for field_name in changed_fields: if field_name not in excluded_fields: - tracker.emit( - event_name, - { - "setting": field_name, - 'old': clean_field(field_name, changed_fields[field_name]), - 'new': clean_field(field_name, getattr(instance, field_name)), - "user_id": user.id, - "table": db_table - } - ) + old_value = clean_field(field_name, changed_fields[field_name]) + new_value = clean_field(field_name, getattr(instance, field_name)) + emit_setting_changed_event(user, event_name, db_table, field_name, old_value, new_value) # Remove the now inaccurate _changed_fields attribute. - if getattr(instance, '_changed_fields', None): + if hasattr(instance, '_changed_fields'): del instance._changed_fields + + +def emit_setting_changed_event(user, event_name, db_table, setting_name, old_value, new_value): + """Emits an event for a change in a setting. + + Args: + user (User): the user that this setting is associated with. + event_name (str): the name of the event to be emitted. + db_table (str): the name of the table that we're modifying. + setting_name (str): the name of the setting being changed. + old_value (object): the value before the change. + new_value (object): the new value being saved. + + Returns: + None + """ + # Compute the maximum value length so that two copies can fit into the maximum event size + # in addition to all the other fields recorded. + max_value_length = settings.TRACK_MAX_EVENT / 4 + + serialized_old_value, old_was_truncated = _get_truncated_setting_value(old_value, max_length=max_value_length) + serialized_new_value, new_was_truncated = _get_truncated_setting_value(new_value, max_length=max_value_length) + truncated_values = [] + if old_was_truncated: + truncated_values.append("old") + if new_was_truncated: + truncated_values.append("new") + tracker.emit( + event_name, + { + "setting": setting_name, + "old": serialized_old_value, + "new": serialized_new_value, + "truncated": truncated_values, + "user_id": user.id, + "table": db_table, + } + ) + + +def _get_truncated_setting_value(value, max_length=None): + """ + Returns the truncated form of a setting value. + + Returns: + truncated_value (object): the possibly truncated version of the value. + was_truncated (bool): returns true if the serialized value was truncated. + """ + if isinstance(value, basestring) and max_length is not None and len(value) > max_length: + return value[0:max_length], True + else: + return value, False diff --git a/common/test/acceptance/pages/lms/learner_profile.py b/common/test/acceptance/pages/lms/learner_profile.py index fd6abe32d6..3e83438309 100644 --- a/common/test/acceptance/pages/lms/learner_profile.py +++ b/common/test/acceptance/pages/lms/learner_profile.py @@ -201,11 +201,12 @@ class LearnerProfilePage(FieldsMixin, PageObject): self.wait_for_field('image') return self.q(css='.u-field-upload-button').visible - def upload_file(self, filename): + def upload_file(self, filename, wait_for_upload_button=True): """ Helper method to upload an image file. """ - self.wait_for_element_visibility('.u-field-upload-button', "upload button is visible") + if wait_for_upload_button: + self.wait_for_element_visibility('.u-field-upload-button', "upload button is visible") file_path = InstructorDashboardPage.get_asset_path(filename) # make the elements visible. diff --git a/common/test/acceptance/tests/lms/test_account_settings.py b/common/test/acceptance/tests/lms/test_account_settings.py index 843610a5db..44a1a89ffa 100644 --- a/common/test/acceptance/tests/lms/test_account_settings.py +++ b/common/test/acceptance/tests/lms/test_account_settings.py @@ -47,8 +47,10 @@ class AccountSettingsTestMixin(EventsTestMixin, WebAppTest): """ expected_referers = [self.ACCOUNT_SETTINGS_REFERER] * len(events) for event in events: - event[u'user_id'] = long(user_id) - event[u'table'] = u"auth_userprofile" if table is None else table + event[u"user_id"] = long(user_id) + event[u"table"] = u"auth_userprofile" if table is None else table + event[u"truncated"] = [] + self.verify_events_of_type( username, self.USER_SETTINGS_CHANGED_EVENT_NAME, events, expected_referers=expected_referers @@ -233,16 +235,18 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest): self.verify_settings_changed_events( self.username, self.user_id, - [{ - u"setting": u"name", - u"old": self.username, - u"new": u"another name", - }, - { - u"setting": u"name", - u"old": u'another name', - u"new": self.username, - }] + [ + { + u"setting": u"name", + u"old": self.username, + u"new": u"another name", + }, + { + u"setting": u"name", + u"old": u'another name', + u"new": self.username, + } + ] ) def test_email_field(self): @@ -339,16 +343,18 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest): ) self.verify_settings_changed_events( self.username, self.user_id, - [{ - u"setting": u"level_of_education", - u"old": None, - u"new": u'b', - }, - { - u"setting": u"level_of_education", - u"old": u'b', - u"new": None, - }] + [ + { + u"setting": u"level_of_education", + u"old": None, + u"new": u'b', + }, + { + u"setting": u"level_of_education", + u"old": u'b', + u"new": None, + } + ] ) def test_gender_field(self): @@ -363,16 +369,18 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest): ) self.verify_settings_changed_events( self.username, self.user_id, - [{ - u"setting": u"gender", - u"old": None, - u"new": u'f', - }, - { - u"setting": u"gender", - u"old": u'f', - u"new": None, - }] + [ + { + u"setting": u"gender", + u"old": None, + u"new": u'f', + }, + { + u"setting": u"gender", + u"old": u'f', + u"new": None, + } + ] ) def test_year_of_birth_field(self): @@ -390,16 +398,18 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest): ) self.verify_settings_changed_events( self.username, self.user_id, - [{ - u"setting": u"year_of_birth", - u"old": None, - u"new": 1980, - }, - { - u"setting": u"year_of_birth", - u"old": 1980, - u"new": None, - }] + [ + { + u"setting": u"year_of_birth", + u"old": None, + u"new": 1980L, + }, + { + u"setting": u"year_of_birth", + u"old": 1980L, + u"new": None, + } + ] ) def test_country_field(self): @@ -441,16 +451,19 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest): ) 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": [], - }], + self.username, self.user_id, + [ + { + 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" ) diff --git a/common/test/acceptance/tests/lms/test_learner_profile.py b/common/test/acceptance/tests/lms/test_learner_profile.py index bc24081746..a2104c275d 100644 --- a/common/test/acceptance/tests/lms/test_learner_profile.py +++ b/common/test/acceptance/tests/lms/test_learner_profile.py @@ -19,8 +19,8 @@ class LearnerProfileTestMixin(EventsTestMixin): Mixin with helper methods for testing learner profile pages. """ - PRIVACY_PUBLIC = 'all_users' - PRIVACY_PRIVATE = 'private' + PRIVACY_PUBLIC = u'all_users' + PRIVACY_PRIVATE = u'private' PUBLIC_PROFILE_FIELDS = ['username', 'country', 'language_proficiencies', 'bio'] PRIVATE_PROFILE_FIELDS = ['username'] @@ -84,6 +84,27 @@ class LearnerProfileTestMixin(EventsTestMixin): str(birth_year) ) + def verify_profile_page_is_public(self, profile_page, is_editable=True): + """ + Verify that the profile page is currently public. + :return: + """ + self.assertEqual(profile_page.visible_fields, self.PUBLIC_PROFILE_FIELDS) + if is_editable: + self.assertTrue(profile_page.privacy_field_visible) + self.assertEqual(profile_page.editable_fields, self.PUBLIC_PROFILE_EDITABLE_FIELDS) + else: + self.assertEqual(profile_page.editable_fields, []) + + def verify_profile_page_is_private(self, profile_page, is_editable=True): + """ + Verify that the profile page is currently private. + :return: + """ + if is_editable: + self.assertTrue(profile_page.privacy_field_visible) + self.assertEqual(profile_page.visible_fields, self.PRIVATE_PROFILE_FIELDS) + def verify_profile_page_view_event(self, requesting_username, profile_user_id, visibility=None): """ Verifies that the correct view event was captured for the profile page. @@ -108,6 +129,24 @@ class LearnerProfileTestMixin(EventsTestMixin): self.USER_SETTINGS_CHANGED_EVENT_NAME, self.start_time, profile_user_id, num_times, setting=setting ) + def verify_user_preference_changed_event(self, username, user_id, setting, old_value=None, new_value=None): + """ + Verifies that the correct user preference changed event was recorded. + """ + self.verify_events_of_type( + username, + u"edx.user.settings.changed", + [{ + u"user_id": long(user_id), + u"table": u"user_api_userpreference", + u"setting": unicode(setting), + u"old": old_value, + u"new": new_value, + u"truncated": [], + }], + expected_referers=["/u/{username}".format(username=username)], + ) + class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): """ @@ -144,6 +183,56 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): self.assertTrue(profile_page.profile_has_default_image) self.assertTrue(profile_page.profile_has_image_with_public_access()) + def test_make_profile_public(self): + """ + Scenario: Verify that the user can change their privacy. + + Given that I am a registered user + And I visit my private profile page + And I set the profile visibility to public + Then a user preference changed event should be recorded + When I reload the page + Then the profile visibility should be shown as public + """ + username, user_id = self.log_in_as_unique_user() + profile_page = self.visit_profile_page(username, privacy=self.PRIVACY_PRIVATE) + profile_page.privacy = self.PRIVACY_PUBLIC + self.verify_user_preference_changed_event( + username, user_id, "account_privacy", + old_value=None, # Note: no old value as the default preference is private + new_value=self.PRIVACY_PUBLIC, + ) + + # Reload the page and verify that the profile is now public + self.browser.refresh() + profile_page.wait_for_page() + self.verify_profile_page_is_public(profile_page) + + def test_make_profile_private(self): + """ + Scenario: Verify that the user can change their privacy. + + Given that I am a registered user + And I visit my public profile page + And I set the profile visibility to private + Then a user preference changed event should be recorded + When I reload the page + Then the profile visibility should be shown as private + """ + username, user_id = self.log_in_as_unique_user() + profile_page = self.visit_profile_page(username, privacy=self.PRIVACY_PUBLIC) + profile_page.privacy = self.PRIVACY_PRIVATE + self.verify_user_preference_changed_event( + username, user_id, "account_privacy", + old_value=self.PRIVACY_PUBLIC, + new_value=self.PRIVACY_PRIVATE, + ) + + # Reload the page and verify that the profile is now private + self.browser.refresh() + profile_page.wait_for_page() + self.verify_profile_page_is_private(profile_page) + def test_dashboard_learner_profile_link(self): """ Scenario: Verify that my profile link is present on dashboard page and we can navigate to correct page. @@ -177,10 +266,7 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): """ username, user_id = self.log_in_as_unique_user() profile_page = self.visit_profile_page(username, privacy=self.PRIVACY_PRIVATE) - - self.assertTrue(profile_page.privacy_field_visible) - self.assertEqual(profile_page.visible_fields, self.PRIVATE_PROFILE_FIELDS) - + self.verify_profile_page_is_private(profile_page) self.verify_profile_page_view_event(username, user_id, visibility=self.PRIVACY_PRIVATE) def test_fields_on_my_public_profile(self): @@ -197,12 +283,7 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): """ username, user_id = self.log_in_as_unique_user() profile_page = self.visit_profile_page(username, privacy=self.PRIVACY_PUBLIC) - - self.assertTrue(profile_page.privacy_field_visible) - self.assertEqual(profile_page.visible_fields, self.PUBLIC_PROFILE_FIELDS) - - self.assertEqual(profile_page.editable_fields, self.PUBLIC_PROFILE_EDITABLE_FIELDS) - + self.verify_profile_page_is_public(profile_page) self.verify_profile_page_view_event(username, user_id, visibility=self.PRIVACY_PUBLIC) def _test_dropdown_field(self, profile_page, field_id, new_value, displayed_value, mode): @@ -510,7 +591,7 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): profile_page.visit() self.assertTrue(profile_page.profile_has_default_image) - self.assert_event_emitted_num_times(user_id, 'profile_image_uploaded_at', 1) + self.assert_event_emitted_num_times(user_id, 'profile_image_uploaded_at', 2) def test_user_cannot_remove_default_image(self): """ @@ -539,8 +620,10 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): """ username, user_id = self.log_in_as_unique_user() profile_page = self.visit_profile_page(username, privacy=self.PRIVACY_PUBLIC) + self.assert_default_image_has_public_access(profile_page) profile_page.upload_file(filename='image.jpg') - profile_page.upload_file(filename='image.jpg') + self.assertTrue(profile_page.image_upload_success) + profile_page.upload_file(filename='image.jpg', wait_for_upload_button=False) self.assert_event_emitted_num_times(user_id, 'profile_image_uploaded_at', 2) @@ -560,9 +643,7 @@ class DifferentUserLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): different_username, different_user_id = self._initialize_different_user(privacy=self.PRIVACY_PRIVATE) username, __ = self.log_in_as_unique_user() profile_page = self.visit_profile_page(different_username) - - self.assertFalse(profile_page.privacy_field_visible) - self.assertEqual(profile_page.visible_fields, self.PRIVATE_PROFILE_FIELDS) + self.verify_profile_page_is_private(profile_page, is_editable=False) self.verify_profile_page_view_event(username, different_user_id, visibility=self.PRIVACY_PRIVATE) def test_different_user_under_age(self): @@ -601,9 +682,7 @@ class DifferentUserLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest): username, __ = self.log_in_as_unique_user() profile_page = self.visit_profile_page(different_username) profile_page.wait_for_public_fields() - self.assertFalse(profile_page.privacy_field_visible) - self.assertEqual(profile_page.visible_fields, self.PUBLIC_PROFILE_FIELDS) - self.assertEqual(profile_page.editable_fields, []) + self.verify_profile_page_is_public(profile_page, is_editable=False) self.verify_profile_page_view_event(username, different_user_id, visibility=self.PRIVACY_PUBLIC) def _initialize_different_user(self, privacy=None, birth_year=None): diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 86fbfef1d7..8be5ab5346 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -8,6 +8,7 @@ from django.core.validators import validate_email, validate_slug, ValidationErro from student.models import User, UserProfile, Registration, USER_SETTINGS_CHANGED_EVENT_NAME from student import views as student_views +from util.model_utils import emit_setting_changed_event from ..errors import ( AccountUpdateError, AccountValidationError, AccountUsernameInvalid, AccountPasswordInvalid, @@ -24,7 +25,6 @@ from . import ( USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH ) from .serializers import AccountLegacyProfileSerializer, AccountUserSerializer -from eventtracking import tracker @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) @@ -199,15 +199,13 @@ def update_account_settings(requesting_user, update, username=None): 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, - } + emit_setting_changed_event( + user=existing_user, + event_name=USER_SETTINGS_CHANGED_EVENT_NAME, + db_table=existing_user_profile.language_proficiencies.model._meta.db_table, + setting_name="language_proficiencies", + old_value=old_language_proficiencies, + new_value=new_language_proficiencies, ) # If the name was changed, store information about the change operation. This is outside of the diff --git a/openedx/core/djangoapps/user_api/models.py b/openedx/core/djangoapps/user_api/models.py index e3642fa2e6..569df46b05 100644 --- a/openedx/core/djangoapps/user_api/models.py +++ b/openedx/core/djangoapps/user_api/models.py @@ -1,8 +1,12 @@ from django.contrib.auth.models import User from django.core.validators import RegexValidator from django.db import models +from django.db.models.signals import pre_delete, post_delete, pre_save, post_save +from django.dispatch import receiver from model_utils.models import TimeStampedModel +from student.models import USER_SETTINGS_CHANGED_EVENT_NAME +from util.model_utils import get_changed_fields_dict, emit_setting_changed_event from xmodule_django.models import CourseKeyField # Currently, the "student" app is responsible for @@ -47,6 +51,39 @@ class UserPreference(models.Model): return None +@receiver(pre_save, sender=UserPreference) +def pre_save_callback(sender, **kwargs): + """ + Event changes to user preferences. + """ + user_preference = kwargs["instance"] + user_preference._old_value = get_changed_fields_dict(user_preference, sender).get("value", None) + +@receiver(post_save, sender=UserPreference) +def post_save_callback(sender, **kwargs): + """ + Event changes to user preferences. + """ + user_preference = kwargs["instance"] + emit_setting_changed_event( + user_preference.user, USER_SETTINGS_CHANGED_EVENT_NAME, sender._meta.db_table, + user_preference.key, user_preference._old_value, user_preference.value + ) + user_preference._old_value = None + + +@receiver(post_delete, sender=UserPreference) +def post_delete_callback(sender, **kwargs): + """ + Event changes to user preferences. + """ + user_preference = kwargs["instance"] + emit_setting_changed_event( + user_preference.user, USER_SETTINGS_CHANGED_EVENT_NAME, sender._meta.db_table, + user_preference.key, user_preference.value, None + ) + + class UserCourseTag(models.Model): """ Per-course user tags, to be used by various things that want to store tags about diff --git a/openedx/core/djangoapps/user_api/tests/test_models.py b/openedx/core/djangoapps/user_api/tests/test_models.py index aae66e10d6..e076409617 100644 --- a/openedx/core/djangoapps/user_api/tests/test_models.py +++ b/openedx/core/djangoapps/user_api/tests/test_models.py @@ -1,12 +1,20 @@ +import json + from django.db import IntegrityError +from django.test import TestCase + +from student.models import USER_SETTINGS_CHANGED_EVENT_NAME +from student.tests.factories import UserFactory +from util.testing import EventTestMixin from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from student.tests.factories import UserFactory from ..tests.factories import UserPreferenceFactory, UserCourseTagFactory, UserOrgTagFactory from ..models import UserPreference from ..preferences.api import set_user_preference +USER_PREFERENCE_TABLE_NAME = "user_api_userpreference" + class UserPreferenceModelTest(ModuleStoreTestCase): def test_duplicate_user_key(self): @@ -83,3 +91,84 @@ class UserPreferenceModelTest(ModuleStoreTestCase): # get preference for key that doesn't exist for user pref = UserPreference.get_value(user, 'testkey_none') self.assertIsNone(pref) + + +class TestUserPreferenceEvents(EventTestMixin, TestCase): + """ + Mixin for verifying that user preference events are fired correctly. + """ + def setUp(self): + super(TestUserPreferenceEvents, self).setUp('util.model_utils.tracker') + self.user = UserFactory.create() + self.TEST_KEY = "test key" + self.TEST_VALUE = "test value" + self.user_preference = UserPreference.objects.create(user=self.user, key=self.TEST_KEY, value=self.TEST_VALUE) + self.reset_tracker() + + def test_create_user_preference(self): + """ + Verify that we emit an event when a user preference is created. + """ + UserPreference.objects.create(user=self.user, key="new key", value="new value") + self.assert_user_preference_event_emitted( + key="new key", old=None, new="new value" + ) + + def test_update_user_preference(self): + """ + Verify that we emit an event when a user preference is updated. + """ + self.user_preference.value = "new value" + self.user_preference.save() + self.assert_user_preference_event_emitted( + key=self.TEST_KEY, old=self.TEST_VALUE, new="new value" + ) + + def test_delete_user_preference(self): + """ + Verify that we emit an event when a user preference is deleted. + """ + self.user_preference.delete() + self.assert_user_preference_event_emitted( + key=self.TEST_KEY, old=self.TEST_VALUE, new=None + ) + + def assert_user_preference_event_emitted(self, key, old, new, truncated=[]): + """ + Helper method to assert that we emit the expected user preference events. + + Expected settings are passed in via `kwargs`. + """ + self.assert_event_emitted( + USER_SETTINGS_CHANGED_EVENT_NAME, + table=USER_PREFERENCE_TABLE_NAME, + user_id=self.user.id, + setting=key, + old=old, + new=new, + truncated=truncated, + ) + + def test_truncated_user_preference_event(self): + """ + Verify that we truncate the preference value if it is too long. + """ + MAX_STRING_LENGTH = 12500 + OVERSIZE_STRING_LENGTH = MAX_STRING_LENGTH + 10 + self.user_preference.value = "z" * OVERSIZE_STRING_LENGTH + self.user_preference.save() + self.assert_user_preference_event_emitted( + key=self.TEST_KEY, + old=self.TEST_VALUE, + new="z" * MAX_STRING_LENGTH, + truncated=["new"], + ) + + self.user_preference.value = "x" * OVERSIZE_STRING_LENGTH + self.user_preference.save() + self.assert_user_preference_event_emitted( + key=self.TEST_KEY, + old="z" * MAX_STRING_LENGTH, + new="x" * MAX_STRING_LENGTH, + truncated=["old", "new"], + )