Commit 17392ef3 by Andy Armstrong

Code review feedback and test cleanup.

parent 7742735f
......@@ -58,7 +58,6 @@ UNENROLL_DONE = Signal(providing_args=["course_enrollment", "skip_refund"])
log = logging.getLogger(__name__)
AUDIT_LOG = logging.getLogger("audit")
SessionStore = import_module(settings.SESSION_ENGINE).SessionStore # pylint: disable=invalid-name
USER_SETTINGS_CHANGED_EVENT_NAME = 'edx.user.settings.changed'
class AnonymousUserId(models.Model):
......@@ -349,7 +348,6 @@ def user_profile_post_save_callback(sender, **kwargs):
emit_field_changed_events(
user_profile,
user_profile.user,
USER_SETTINGS_CHANGED_EVENT_NAME,
sender._meta.db_table,
excluded_fields=['meta']
)
......@@ -375,7 +373,6 @@ def user_post_save_callback(sender, **kwargs):
emit_field_changed_events(
user,
user,
USER_SETTINGS_CHANGED_EVENT_NAME,
sender._meta.db_table,
excluded_fields=['last_login'],
hidden_fields=['password']
......@@ -1675,9 +1672,8 @@ class LanguageProficiency(models.Model):
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.
/edx-platform/openedx/core/djangoapps/user_api/accounts/views.py or its associated api method
(update_account_settings) so that the events are emitted.
"""
class Meta:
unique_together = (('code', 'user_profile'),)
......
......@@ -6,9 +6,11 @@ from django.test import TestCase
from django_countries.fields import Country
from student.models import PasswordHistory, USER_SETTINGS_CHANGED_EVENT_NAME
from student.models import PasswordHistory
from student.tests.factories import UserFactory
from student.tests.tests import UserSettingsEventTestMixin
import mock
from django.db.utils import IntegrityError
class TestUserProfileEvents(UserSettingsEventTestMixin, TestCase):
......@@ -72,6 +74,18 @@ class TestUserProfileEvents(UserSettingsEventTestMixin, TestCase):
self.profile.save()
self.assert_no_events_were_emitted()
@mock.patch('student.models.UserProfile.save', side_effect=IntegrityError)
def test_no_event_if_save_failed(self, _save_mock):
"""
Verify no event is triggered if the save does not complete. Note that the pre_save
signal is not called in this case either, but the intent is to make it clear that this model
should never emit an event if save fails.
"""
self.profile.gender = "unknown"
with self.assertRaises(IntegrityError):
self.profile.save()
self.assert_no_events_were_emitted()
class TestUserEvents(UserSettingsEventTestMixin, TestCase):
"""
......@@ -120,3 +134,15 @@ class TestUserEvents(UserSettingsEventTestMixin, TestCase):
self.user.passwordhistory_set.add(PasswordHistory(password='new_password'))
self.user.save()
self.assert_no_events_were_emitted()
@mock.patch('django.contrib.auth.models.User.save', side_effect=IntegrityError)
def test_no_event_if_save_failed(self, _save_mock):
"""
Verify no event is triggered if the save does not complete. Note that the pre_save
signal is not called in this case either, but the intent is to make it clear that this model
should never emit an event if save fails.
"""
self.user.password = u'new password'
with self.assertRaises(IntegrityError):
self.user.save()
self.assert_no_events_were_emitted()
......@@ -21,13 +21,13 @@ from mock import Mock, patch
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from student.models import (
anonymous_id_for_user, user_by_anonymous_id, CourseEnrollment, unique_id_for_user,
LinkedInAddToProfileConfiguration, USER_SETTINGS_CHANGED_EVENT_NAME
anonymous_id_for_user, user_by_anonymous_id, CourseEnrollment, unique_id_for_user, LinkedInAddToProfileConfiguration
)
from student.views import (process_survey_link, _cert_info,
change_enrollment, complete_course_mode_info)
from student.tests.factories import UserFactory, CourseModeFactory
from util.testing import EventTestMixin
from util.model_utils import USER_SETTINGS_CHANGED_EVENT_NAME
from xmodule.modulestore.tests.factories import CourseFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
......@@ -499,6 +499,8 @@ class UserSettingsEventTestMixin(EventTestMixin):
Expected settings are passed in via `kwargs`.
"""
if 'truncated' not in kwargs:
kwargs['truncated'] = []
self.assert_event_emitted(
USER_SETTINGS_CHANGED_EVENT_NAME,
table=self.table, # pylint: disable=no-member
......
......@@ -9,6 +9,9 @@ from django.db.models.fields.related import RelatedField
from django_countries.fields import Country
# The setting name used for events when "settings" (account settings, preferences, profile information) change.
USER_SETTINGS_CHANGED_EVENT_NAME = u'edx.user.settings.changed'
def get_changed_fields_dict(instance, model_class):
"""
......@@ -26,7 +29,7 @@ def get_changed_fields_dict(instance, model_class):
Returns:
dict: a mapping of field names to current database values of those
fields, or an empty dicit if the model is new
fields, or an empty dict if the model is new
"""
try:
old_model = model_class.objects.get(pk=instance.pk)
......@@ -46,8 +49,8 @@ def get_changed_fields_dict(instance, model_class):
return changed_fields
def emit_field_changed_events(instance, user, event_name, db_table, excluded_fields=None, hidden_fields=None):
"""Emits an event for each field that has changed.
def emit_field_changed_events(instance, user, db_table, excluded_fields=None, hidden_fields=None):
"""Emits a settings changed 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`.
......@@ -55,7 +58,6 @@ def emit_field_changed_events(instance, user, event_name, db_table, excluded_fie
Args:
instance (Model instance): the model instance that is being saved
user (User): the user that this instance 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
excluded_fields (list): a list of field names for which events should
not be emitted
......@@ -88,18 +90,17 @@ def emit_field_changed_events(instance, user, event_name, db_table, excluded_fie
if field_name not in excluded_fields:
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)
emit_setting_changed_event(user, db_table, field_name, old_value, new_value)
# Remove the now inaccurate _changed_fields attribute.
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):
def emit_setting_changed_event(user, 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.
......@@ -120,7 +121,7 @@ def emit_setting_changed_event(user, event_name, db_table, setting_name, old_val
if new_was_truncated:
truncated_values.append("new")
tracker.emit(
event_name,
USER_SETTINGS_CHANGED_EVENT_NAME,
{
"setting": setting_name,
"old": serialized_old_value,
......
......@@ -259,6 +259,7 @@ class LearnerProfilePage(FieldsMixin, PageObject):
self.wait_for_element_visibility('.u-field-remove-button', "remove button is visible")
self.q(css='.u-field-remove-button').first.click()
self.wait_for_ajax()
self.mouse_hover(self.browser.find_element_by_css_selector('.image-wrapper'))
self.wait_for_element_visibility('.u-field-upload-button', "upload button is visible")
return True
......
......@@ -308,7 +308,7 @@ class EventsTestMixin(object):
def get_matching_events(self, username, event_type):
"""
Returns a cursor for the matching browser events.
Returns a cursor for the matching browser events related emitted for the specified username.
"""
return self.event_collection.find({
"username": username,
......@@ -319,7 +319,7 @@ class EventsTestMixin(object):
def verify_events_of_type(self, username, event_type, expected_events, expected_referers=None):
"""Verify that the expected events of a given type were logged.
Args:
username (str): The name of the authenticated user.
username (str): The name of the user for which events will be tested.
event_type (str): The type of event to be verified.
expected_events (list): A list of dicts representing the events that should
have been fired.
......
......@@ -253,13 +253,13 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest):
"""
Test behaviour of "Email" field.
"""
EMAIL = u"test@example.com"
username, user_id = self.log_in_as_unique_user(email=EMAIL)
email = u"test@example.com"
username, user_id = self.log_in_as_unique_user(email=email)
self.visit_account_settings_page()
self._test_text_field(
u'email',
u'Email Address',
EMAIL,
email,
u'@',
[u'me@here.com', u'you@there.com'],
success_message='Click the link in the message to update your email address.',
......@@ -273,13 +273,13 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest):
{
u"user_id": long(user_id),
u"setting": u"email",
u"old": EMAIL,
u"old": email,
u"new": u'me@here.com'
},
{
u"user_id": long(user_id),
u"setting": u"email",
u"old": EMAIL, # NOTE the first email change was never confirmed, so old has not changed.
u"old": email, # NOTE the first email change was never confirmed, so old has not changed.
u"new": u'you@there.com'
}
],
......@@ -423,22 +423,6 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest):
[u'Pakistan', u'Palau'],
)
def test_country_field_events(self):
"""
Test that saving the country field records the correct events.
"""
self.reset_event_tracking()
self.assertEqual(self.account_settings_page.value_for_dropdown_field(u'country', u'Pakistan'), u'Pakistan')
self.account_settings_page.wait_for_messsage(u'country', self.SUCCESS_MESSAGE)
self.verify_settings_changed_events(
self.username, self.user_id,
[{
u"setting": u"country",
u"old": None,
u"new": u'PK',
}],
)
def test_preferred_language_field(self):
"""
Test behaviour of "Preferred Language" field.
......
......@@ -87,7 +87,6 @@ class LearnerProfileTestMixin(EventsTestMixin):
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:
......@@ -99,7 +98,6 @@ class LearnerProfileTestMixin(EventsTestMixin):
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)
......@@ -113,7 +111,7 @@ class LearnerProfileTestMixin(EventsTestMixin):
requesting_username,
u"edx.user.settings.viewed",
[{
u"user_id": long(profile_user_id),
u"user_id": int(profile_user_id),
u"page": u"profile",
u"visibility": unicode(visibility),
}]
......@@ -135,7 +133,7 @@ class LearnerProfileTestMixin(EventsTestMixin):
"""
self.verify_events_of_type(
username,
u"edx.user.settings.changed",
self.USER_SETTINGS_CHANGED_EVENT_NAME,
[{
u"user_id": long(user_id),
u"table": u"user_api_userpreference",
......@@ -173,8 +171,7 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest):
"""
username, user_id = self.log_in_as_unique_user()
profile_page = self.visit_profile_page(username)
self.assertTrue(profile_page.privacy_field_visible)
self.assertEquals(profile_page.privacy, self.PRIVACY_PUBLIC)
self.verify_profile_page_is_public(profile_page)
def assert_default_image_has_public_access(self, profile_page):
"""
......@@ -199,7 +196,7 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest):
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
old_value=self.PRIVACY_PRIVATE, # Note: default value was public, so we first change to private
new_value=self.PRIVACY_PUBLIC,
)
......@@ -224,7 +221,7 @@ class OwnLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest):
profile_page.privacy = self.PRIVACY_PRIVATE
self.verify_user_preference_changed_event(
username, user_id, "account_privacy",
old_value=self.PRIVACY_PUBLIC,
old_value=None, # Note: no old value as the default preference is public
new_value=self.PRIVACY_PRIVATE,
)
......@@ -648,24 +645,22 @@ class DifferentUserLearnerProfilePageTest(LearnerProfileTestMixin, WebAppTest):
def test_different_user_under_age(self):
"""
Scenario: Verify that desired fields are shown when looking at a different user's private profile.
Scenario: Verify that an under age user's profile is private to others.
Given that I am a registered user.
And I visit an under age user's profile page.
Then I shouldn't see the profile visibility selector dropdown.
Then I see some of the profile fields are shown.
Then I see that only the private fields are shown.
"""
under_age_birth_year = datetime.now().year - 10
different_username, different_user_id = self._initialize_different_user(
privacy=self.PRIVACY_PUBLIC,
birth_year=under_age_birth_year
)
self.log_in_as_unique_user()
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_view_event(different_user_id, visibility=self.PRIVACY_PRIVATE)
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_public_profile(self):
"""
......
......@@ -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, USER_SETTINGS_CHANGED_EVENT_NAME
from student.models import User, UserProfile, Registration
from student import views as student_views
from util.model_utils import emit_setting_changed_event
......@@ -201,7 +201,6 @@ def update_account_settings(requesting_user, update, username=None):
new_language_proficiencies = legacy_profile_serializer.data["language_proficiencies"]
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,
......
......@@ -15,7 +15,8 @@ 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, USER_SETTINGS_CHANGED_EVENT_NAME
from student.models import PendingEmailChange
from student.tests.tests import UserSettingsEventTestMixin
from ...errors import (
UserNotFound, UserNotAuthorized, AccountUpdateError, AccountValidationError,
AccountUserAlreadyExists, AccountUsernameInvalid, AccountEmailInvalid, AccountPasswordInvalid, AccountRequestError
......@@ -24,7 +25,6 @@ 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):
......@@ -33,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(EventTestMixin, TestCase):
class TestAccountApi(UserSettingsEventTestMixin, 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
......@@ -42,10 +42,12 @@ class TestAccountApi(EventTestMixin, TestCase):
password = "test"
def setUp(self):
super(TestAccountApi, self).setUp('openedx.core.djangoapps.user_api.accounts.api.tracker')
super(TestAccountApi, self).setUp()
self.table = "student_languageproficiency"
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)
self.reset_tracker()
def test_get_username_provided(self):
"""Test the difference in behavior when a username is supplied to get_account_settings."""
......@@ -198,19 +200,10 @@ class TestAccountApi(EventTestMixin, TestCase):
"""
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.assert_user_setting_event_emitted(setting='language_proficiencies', old=old_value, new=new_value)
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.
# 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.
......
......@@ -5,7 +5,6 @@ from django.db.models.signals import pre_delete, post_delete, pre_save, post_sav
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
......@@ -59,6 +58,7 @@ def pre_save_callback(sender, **kwargs):
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):
"""
......@@ -66,8 +66,8 @@ def post_save_callback(sender, **kwargs):
"""
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.user, sender._meta.db_table, user_preference.key,
user_preference._old_value, user_preference.value
)
user_preference._old_value = None
......@@ -79,8 +79,7 @@ def post_delete_callback(sender, **kwargs):
"""
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
user_preference.user, sender._meta.db_table, user_preference.key, user_preference.value, None
)
......
......@@ -3,9 +3,8 @@ 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 student.tests.tests import UserSettingsEventTestMixin
from xmodule.modulestore.tests.factories import CourseFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
......@@ -13,8 +12,6 @@ from ..tests.factories import UserPreferenceFactory, UserCourseTagFactory, UserO
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):
......@@ -93,12 +90,13 @@ class UserPreferenceModelTest(ModuleStoreTestCase):
self.assertIsNone(pref)
class TestUserPreferenceEvents(EventTestMixin, TestCase):
class TestUserPreferenceEvents(UserSettingsEventTestMixin, TestCase):
"""
Mixin for verifying that user preference events are fired correctly.
"""
def setUp(self):
super(TestUserPreferenceEvents, self).setUp('util.model_utils.tracker')
super(TestUserPreferenceEvents, self).setUp()
self.table = "user_api_userpreference"
self.user = UserFactory.create()
self.TEST_KEY = "test key"
self.TEST_VALUE = "test value"
......@@ -110,9 +108,7 @@ class TestUserPreferenceEvents(EventTestMixin, TestCase):
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"
)
self.assert_user_setting_event_emitted(setting='new key', old=None, new="new value")
def test_update_user_preference(self):
"""
......@@ -120,34 +116,14 @@ class TestUserPreferenceEvents(EventTestMixin, TestCase):
"""
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"
)
self.assert_user_setting_event_emitted(setting=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,
)
self.assert_user_setting_event_emitted(setting=self.TEST_KEY, old=self.TEST_VALUE, new=None)
def test_truncated_user_preference_event(self):
"""
......@@ -157,18 +133,12 @@ class TestUserPreferenceEvents(EventTestMixin, TestCase):
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.assert_user_setting_event_emitted(
setting=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"],
self.assert_user_setting_event_emitted(
setting=self.TEST_KEY, old="z" * MAX_STRING_LENGTH, new="x" * MAX_STRING_LENGTH, truncated=["old", "new"]
)
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