From 356b2335e9c9101cd04865ecf6d92ea2e9b0912c Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Thu, 23 Oct 2014 18:35:42 -0400 Subject: [PATCH] Add base support for cohorted group configurations TNL-649 --- CHANGELOG.rst | 2 + cms/djangoapps/contentstore/views/course.py | 28 +- .../views/tests/test_group_configurations.py | 46 +- .../contentstore/views/tests/test_item.py | 3 +- .../models/settings/course_metadata.py | 1 - cms/envs/common.py | 2 +- cms/static/js/models/group.js | 2 +- cms/static/js/models/group_configuration.js | 4 +- .../spec/models/group_configuration_spec.js | 6 +- cms/templates/group_configurations.html | 2 +- cms/templates/settings.html | 7 +- cms/templates/settings_advanced.html | 7 +- cms/templates/settings_graders.html | 7 +- cms/templates/widgets/header.html | 11 +- cms/urls.py | 2 +- common/djangoapps/lang_pref/middleware.py | 2 +- .../lang_pref/tests/test_middleware.py | 2 +- .../djangoapps/lang_pref/tests/test_views.py | 2 +- common/djangoapps/lang_pref/views.py | 2 +- .../student/tests/test_create_account.py | 2 +- common/djangoapps/student/views.py | 2 +- .../xmodule/modulestore/inheritance.py | 4 +- .../test_cross_modulestore_import_export.py | 3 +- .../xmodule/xmodule/partitions/partitions.py | 86 +++- .../xmodule/partitions/partitions_service.py | 85 +--- .../xmodule/partitions/test_partitions.py | 286 ------------- .../xmodule/partitions/tests}/__init__.py | 0 .../partitions/tests/test_partitions.py | 302 +++++++++++++ .../lib/xmodule/xmodule/split_test_module.py | 2 +- common/lib/xmodule/xmodule/tests/__init__.py | 4 +- .../xmodule/tests/test_split_test_module.py | 53 +-- common/test/acceptance/.coveragerc | 2 +- .../tests/studio/test_studio_split_test.py | 121 +++++- lms/.coveragerc | 2 +- .../courseware/tests/test_split_module.py | 2 +- .../tests/test_submitting_problems.py | 2 +- lms/djangoapps/instructor/views/api.py | 2 +- .../instructor_task/tests/test_integration.py | 2 +- .../features/unsubscribe.py | 2 +- lms/djangoapps/notification_prefs/tests.py | 2 +- lms/djangoapps/notification_prefs/views.py | 2 +- lms/djangoapps/notifier_api/tests.py | 4 +- lms/djangoapps/notifier_api/views.py | 2 +- lms/djangoapps/oauth2_handler/handlers.py | 2 +- lms/djangoapps/oauth2_handler/tests.py | 2 +- .../student_account/test/test_views.py | 5 +- lms/djangoapps/student_account/views.py | 4 +- .../student_profile/test/test_views.py | 4 +- lms/djangoapps/student_profile/views.py | 2 +- lms/envs/common.py | 4 +- lms/lib/xblock/runtime.py | 9 +- lms/urls.py | 2 +- .../user_api/api => openedx}/__init__.py | 0 .../migrations => openedx/core}/__init__.py | 0 .../core/djangoapps}/__init__.py | 0 openedx/core/djangoapps/user_api/__init__.py | 0 .../core/djangoapps/user_api/api/__init__.py | 0 .../core}/djangoapps/user_api/api/account.py | 4 +- .../djangoapps/user_api/api/course_tag.py | 0 .../core}/djangoapps/user_api/api/profile.py | 5 +- .../core}/djangoapps/user_api/helpers.py | 11 +- .../user_api/management/__init__.py | 0 .../user_api/management/commands/__init__.py | 0 .../management/commands/email_opt_in_list.py | 267 ++++++++++++ .../user_api/management/tests/__init__.py | 0 .../tests/test_email_opt_in_list.py | 399 ++++++++++++++++++ .../core}/djangoapps/user_api/middleware.py | 3 +- .../user_api/migrations/0001_initial.py | 0 ...nique_usercoursetags_user_course_id_key.py | 0 .../migrations/0003_rename_usercoursetags.py | 0 .../user_api/migrations/__init__.py | 0 .../core}/djangoapps/user_api/models.py | 0 .../djangoapps/user_api/partition_schemes.py | 61 +++ .../core}/djangoapps/user_api/serializers.py | 3 +- .../djangoapps/user_api/tests/__init__.py | 0 .../djangoapps/user_api/tests/factories.py | 3 +- .../user_api/tests/test_account_api.py | 4 +- .../user_api/tests/test_constants.py | 0 .../user_api/tests/test_course_tag_api.py | 4 +- .../djangoapps/user_api/tests/test_helpers.py | 8 +- .../user_api/tests/test_middleware.py | 7 +- .../djangoapps/user_api/tests/test_models.py | 5 +- .../user_api/tests/test_partition_schemes.py | 107 +++++ .../user_api/tests/test_profile_api.py | 6 +- .../djangoapps/user_api/tests/test_views.py | 10 +- .../core}/djangoapps/user_api/urls.py | 18 +- .../core}/djangoapps/user_api/views.py | 10 +- pavelib/tests.py | 2 +- pavelib/utils/test/suites/nose_suite.py | 2 +- setup.py | 16 +- 90 files changed, 1534 insertions(+), 567 deletions(-) delete mode 100644 common/lib/xmodule/xmodule/partitions/test_partitions.py rename common/{djangoapps/user_api => lib/xmodule/xmodule/partitions/tests}/__init__.py (100%) create mode 100644 common/lib/xmodule/xmodule/partitions/tests/test_partitions.py rename {common/djangoapps/user_api/api => openedx}/__init__.py (100%) rename {common/djangoapps/user_api/migrations => openedx/core}/__init__.py (100%) rename {common/djangoapps/user_api/tests => openedx/core/djangoapps}/__init__.py (100%) create mode 100644 openedx/core/djangoapps/user_api/__init__.py create mode 100644 openedx/core/djangoapps/user_api/api/__init__.py rename {common => openedx/core}/djangoapps/user_api/api/account.py (99%) rename {common => openedx/core}/djangoapps/user_api/api/course_tag.py (100%) rename {common => openedx/core}/djangoapps/user_api/api/profile.py (97%) rename {common => openedx/core}/djangoapps/user_api/helpers.py (97%) create mode 100644 openedx/core/djangoapps/user_api/management/__init__.py create mode 100644 openedx/core/djangoapps/user_api/management/commands/__init__.py create mode 100644 openedx/core/djangoapps/user_api/management/commands/email_opt_in_list.py create mode 100644 openedx/core/djangoapps/user_api/management/tests/__init__.py create mode 100644 openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py rename {common => openedx/core}/djangoapps/user_api/middleware.py (97%) rename {common => openedx/core}/djangoapps/user_api/migrations/0001_initial.py (100%) rename {common => openedx/core}/djangoapps/user_api/migrations/0002_auto__add_usercoursetags__add_unique_usercoursetags_user_course_id_key.py (100%) rename {common => openedx/core}/djangoapps/user_api/migrations/0003_rename_usercoursetags.py (100%) create mode 100644 openedx/core/djangoapps/user_api/migrations/__init__.py rename {common => openedx/core}/djangoapps/user_api/models.py (100%) create mode 100644 openedx/core/djangoapps/user_api/partition_schemes.py rename {common => openedx/core}/djangoapps/user_api/serializers.py (95%) create mode 100644 openedx/core/djangoapps/user_api/tests/__init__.py rename {common => openedx/core}/djangoapps/user_api/tests/factories.py (92%) rename {common => openedx/core}/djangoapps/user_api/tests/test_account_api.py (99%) rename {common => openedx/core}/djangoapps/user_api/tests/test_constants.py (100%) rename {common => openedx/core}/djangoapps/user_api/tests/test_course_tag_api.py (91%) rename {common => openedx/core}/djangoapps/user_api/tests/test_helpers.py (96%) rename {common => openedx/core}/djangoapps/user_api/tests/test_middleware.py (95%) rename {common => openedx/core}/djangoapps/user_api/tests/test_models.py (95%) create mode 100644 openedx/core/djangoapps/user_api/tests/test_partition_schemes.py rename {common => openedx/core}/djangoapps/user_api/tests/test_profile_api.py (98%) rename {common => openedx/core}/djangoapps/user_api/tests/test_views.py (99%) rename {common => openedx/core}/djangoapps/user_api/urls.py (76%) rename {common => openedx/core}/djangoapps/user_api/views.py (99%) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index fb5aa94e5c..a044ecc910 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Platform: Add base support for cohorted group configurations. TNL-649 + Common: Add configurable reset button to units Studio: Add support xblock validation messages on Studio unit/container page. TNL-683 diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 318e3f9f3f..ca9887b4d5 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -22,7 +22,7 @@ from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore from xmodule.contentstore.content import StaticContent from xmodule.tabs import PDFTextbookTabs -from xmodule.partitions.partitions import UserPartition, Group +from xmodule.partitions.partitions import UserPartition from xmodule.modulestore import EdxJSONEncoder from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError from opaque_keys import InvalidKeyError @@ -1173,7 +1173,7 @@ class GroupConfiguration(object): configuration = json.loads(json_string) except ValueError: raise GroupConfigurationsValidationError(_("invalid JSON")) - + configuration["version"] = UserPartition.VERSION return configuration def validate(self): @@ -1224,14 +1224,7 @@ class GroupConfiguration(object): """ Get user partition for saving in course. """ - groups = [Group(g["id"], g["name"]) for g in self.configuration["groups"]] - - return UserPartition( - self.configuration["id"], - self.configuration["name"], - self.configuration["description"], - groups - ) + return UserPartition.from_json(self.configuration) @staticmethod def get_usage_info(course, store): @@ -1345,15 +1338,12 @@ def group_configurations_list_handler(request, course_key_string): if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): group_configuration_url = reverse_course_url('group_configurations_list_handler', course_key) course_outline_url = reverse_course_url('course_handler', course_key) - split_test_enabled = SPLIT_TEST_COMPONENT_TYPE in ADVANCED_COMPONENT_TYPES and SPLIT_TEST_COMPONENT_TYPE in course.advanced_modules - configurations = GroupConfiguration.add_usage_info(course, store) - return render_to_response('group_configurations.html', { 'context_course': course, 'group_configuration_url': group_configuration_url, 'course_outline_url': course_outline_url, - 'configurations': configurations if split_test_enabled else None, + 'configurations': configurations if should_show_group_configurations_page(course) else None, }) elif "application/json" in request.META.get('HTTP_ACCEPT'): if request.method == 'POST': @@ -1432,6 +1422,16 @@ def group_configurations_detail_handler(request, course_key_string, group_config return JsonResponse(status=204) +def should_show_group_configurations_page(course): + """ + Returns true if Studio should show the "Group Configurations" page for the specified course. + """ + return ( + SPLIT_TEST_COMPONENT_TYPE in ADVANCED_COMPONENT_TYPES and + SPLIT_TEST_COMPONENT_TYPE in course.advanced_modules + ) + + def _get_course_creator_status(user): """ Helper method for returning the course creator status for a particular user, diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index 7d03c23c0f..5c0e5130d1 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -15,10 +15,17 @@ from xmodule.modulestore import ModuleStoreEnum GROUP_CONFIGURATION_JSON = { u'name': u'Test name', + u'scheme': u'random', u'description': u'Test description', + u'version': UserPartition.VERSION, u'groups': [ - {u'name': u'Group A'}, - {u'name': u'Group B'}, + { + u'name': u'Group A', + u'version': 1, + }, { + u'name': u'Group B', + u'version': 1, + }, ], } @@ -229,7 +236,8 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations expected = { u'description': u'Test description', u'name': u'Test name', - u'version': 1, + u'scheme': u'random', + u'version': UserPartition.VERSION, u'groups': [ {u'name': u'Group A', u'version': 1}, {u'name': u'Group B', u'version': 1}, @@ -279,15 +287,16 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio kwargs={'group_configuration_id': cid}, ) - def test_can_create_new_group_configuration_if_it_is_not_exist(self): + def test_can_create_new_group_configuration_if_it_does_not_exist(self): """ PUT new group configuration when no configurations exist in the course. """ expected = { u'id': 999, u'name': u'Test name', + u'scheme': u'random', u'description': u'Test description', - u'version': 1, + u'version': UserPartition.VERSION, u'groups': [ {u'id': 0, u'name': u'Group A', u'version': 1}, {u'id': 1, u'name': u'Group B', u'version': 1}, @@ -306,12 +315,12 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio self.assertEqual(content, expected) self.reload_course() # Verify that user_partitions in the course contains the new group configuration. - user_partititons = self.course.user_partitions - self.assertEqual(len(user_partititons), 1) - self.assertEqual(user_partititons[0].name, u'Test name') - self.assertEqual(len(user_partititons[0].groups), 2) - self.assertEqual(user_partititons[0].groups[0].name, u'Group A') - self.assertEqual(user_partititons[0].groups[1].name, u'Group B') + user_partitions = self.course.user_partitions + self.assertEqual(len(user_partitions), 1) + self.assertEqual(user_partitions[0].name, u'Test name') + self.assertEqual(len(user_partitions[0].groups), 2) + self.assertEqual(user_partitions[0].groups[0].name, u'Group A') + self.assertEqual(user_partitions[0].groups[1].name, u'Group B') def test_can_edit_group_configuration(self): """ @@ -323,8 +332,9 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio expected = { u'id': self.ID, u'name': u'New Test name', + u'scheme': u'random', u'description': u'New Test description', - u'version': 1, + u'version': UserPartition.VERSION, u'groups': [ {u'id': 0, u'name': u'New Group Name', u'version': 1}, {u'id': 2, u'name': u'Group C', u'version': 1}, @@ -430,8 +440,9 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): expected = [{ 'id': 0, 'name': 'Name 0', + 'scheme': 'random', 'description': 'Description 0', - 'version': 1, + 'version': UserPartition.VERSION, 'groups': [ {'id': 0, 'name': 'Group A', 'version': 1}, {'id': 1, 'name': 'Group B', 'version': 1}, @@ -454,8 +465,9 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): expected = [{ 'id': 0, 'name': 'Name 0', + 'scheme': 'random', 'description': 'Description 0', - 'version': 1, + 'version': UserPartition.VERSION, 'groups': [ {'id': 0, 'name': 'Group A', 'version': 1}, {'id': 1, 'name': 'Group B', 'version': 1}, @@ -469,8 +481,9 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): }, { 'id': 1, 'name': 'Name 1', + 'scheme': 'random', 'description': 'Description 1', - 'version': 1, + 'version': UserPartition.VERSION, 'groups': [ {'id': 0, 'name': 'Group A', 'version': 1}, {'id': 1, 'name': 'Group B', 'version': 1}, @@ -495,8 +508,9 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): expected = [{ 'id': 0, 'name': 'Name 0', + 'scheme': 'random', 'description': 'Description 0', - 'version': 1, + 'version': UserPartition.VERSION, 'groups': [ {'id': 0, 'name': 'Group A', 'version': 1}, {'id': 1, 'name': 'Group B', 'version': 1}, diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 496eee0c52..b496a7ffc4 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -223,8 +223,9 @@ class GetItemTest(ItemTest): GROUP_CONFIGURATION_JSON = { u'id': 0, u'name': u'first_partition', + u'scheme': u'random', u'description': u'First Partition', - u'version': 1, + u'version': UserPartition.VERSION, u'groups': [ {u'id': 0, u'name': u'New_NAME_A', u'version': 1}, {u'id': 1, u'name': u'New_NAME_B', u'version': 1}, diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index d1b0ebacf1..236451eb4f 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -28,7 +28,6 @@ class CourseMetadata(object): 'graded', 'hide_from_toc', 'pdf_textbooks', - 'user_partitions', 'name', # from xblock 'tags', # from xblock 'visible_to_staff_only', diff --git a/cms/envs/common.py b/cms/envs/common.py index 4055c5c944..4feb741582 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -607,7 +607,7 @@ INSTALLED_APPS = ( 'reverification', # User preferences - 'user_api', + 'openedx.core.djangoapps.user_api', 'django_openid_auth', 'embargo', diff --git a/cms/static/js/models/group.js b/cms/static/js/models/group.js index 96548f7e8e..9456c2c56a 100644 --- a/cms/static/js/models/group.js +++ b/cms/static/js/models/group.js @@ -7,7 +7,7 @@ define([ defaults: function() { return { name: '', - version: null, + version: 1, order: null }; }, diff --git a/cms/static/js/models/group_configuration.js b/cms/static/js/models/group_configuration.js index 95c97ee3d5..0ef7a27c6e 100644 --- a/cms/static/js/models/group_configuration.js +++ b/cms/static/js/models/group_configuration.js @@ -9,8 +9,9 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) { defaults: function() { return { name: '', + scheme: 'random', description: '', - version: null, + version: 2, groups: new GroupCollection([ { name: gettext('Group A'), @@ -71,6 +72,7 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) { return { id: this.get('id'), name: this.get('name'), + scheme: this.get('scheme'), description: this.get('description'), version: this.get('version'), groups: this.get('groups').toJSON() diff --git a/cms/static/js/spec/models/group_configuration_spec.js b/cms/static/js/spec/models/group_configuration_spec.js index 4a1b183f19..b58532d2a5 100644 --- a/cms/static/js/spec/models/group_configuration_spec.js +++ b/cms/static/js/spec/models/group_configuration_spec.js @@ -99,7 +99,8 @@ define([ 'id': 10, 'name': 'My Group Configuration', 'description': 'Some description', - 'version': 1, + 'version': 2, + 'scheme': 'random', 'groups': [ { 'version': 1, @@ -114,9 +115,10 @@ define([ 'id': 10, 'name': 'My Group Configuration', 'description': 'Some description', + 'scheme': 'random', 'showGroups': false, 'editing': false, - 'version': 1, + 'version': 2, 'groups': [ { 'version': 1, diff --git a/cms/templates/group_configurations.html b/cms/templates/group_configurations.html index d4d8650cef..954b1509ff 100644 --- a/cms/templates/group_configurations.html +++ b/cms/templates/group_configurations.html @@ -55,7 +55,7 @@ % else:
-

${_("Loading...")}

+

${_("Loading")}

% endif diff --git a/cms/templates/settings.html b/cms/templates/settings.html index 4746f6bb91..ea12f2453a 100644 --- a/cms/templates/settings.html +++ b/cms/templates/settings.html @@ -7,6 +7,7 @@ <%! from django.utils.translation import ugettext as _ from contentstore import utils + from contentstore.views.course import should_show_group_configurations_page import urllib %> @@ -312,10 +313,10 @@ CMS.URL.UPLOAD_ASSET = '${upload_asset_url}'; % endif diff --git a/cms/templates/settings_advanced.html b/cms/templates/settings_advanced.html index f4467945ff..af021a31d3 100644 --- a/cms/templates/settings_advanced.html +++ b/cms/templates/settings_advanced.html @@ -4,6 +4,7 @@ <%! from django.utils.translation import ugettext as _ from contentstore import utils + from contentstore.views.course import should_show_group_configurations_page from django.utils.html import escapejs %> <%block name="title">${_("Advanced Settings")} @@ -91,9 +92,9 @@
  • ${_("Details & Schedule")}
  • ${_("Grading")}
  • ${_("Course Team")}
    • - % if "split_test" in context_course.advanced_modules: -
  • ${_("Group Configurations")}
    • - % endif + % if should_show_group_configurations_page(context_course): +
  • ${_("Group Configurations")}
    • + % endif % endif diff --git a/cms/templates/settings_graders.html b/cms/templates/settings_graders.html index 641b06fc77..69c15d4a24 100644 --- a/cms/templates/settings_graders.html +++ b/cms/templates/settings_graders.html @@ -6,6 +6,7 @@ <%namespace name='static' file='static_content.html'/> <%! from contentstore import utils + from contentstore.views.course import should_show_group_configurations_page from django.utils.translation import ugettext as _ %> @@ -134,10 +135,10 @@ % endif diff --git a/cms/templates/widgets/header.html b/cms/templates/widgets/header.html index 266c656b17..6ff87f6d4b 100644 --- a/cms/templates/widgets/header.html +++ b/cms/templates/widgets/header.html @@ -3,6 +3,7 @@ from django.core.urlresolvers import reverse from django.utils.translation import ugettext as _ from contentstore.context_processors import doc_url + from contentstore.views.course import should_show_group_configurations_page %> <%page args="online_help_token"/> @@ -81,14 +82,14 @@
  • ${_("Course Team")}
    • + % if should_show_group_configurations_page(context_course): +
  • + ${_("Group Configurations")} +
    • + % endif
  • ${_("Advanced Settings")}
    • - % if "split_test" in context_course.advanced_modules: -
  • - ${_("Group Configurations")} -
    • - % endif diff --git a/cms/urls.py b/cms/urls.py index afdec0eec8..7e06f50339 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -39,7 +39,7 @@ urlpatterns = patterns('', # nopep8 url(r'^xmodule/', include('pipeline_js.urls')), url(r'^heartbeat$', include('heartbeat.urls')), - url(r'^user_api/', include('user_api.urls')), + url(r'^user_api/', include('openedx.core.djangoapps.user_api.urls')), url(r'^lang_pref/', include('lang_pref.urls')), ) diff --git a/common/djangoapps/lang_pref/middleware.py b/common/djangoapps/lang_pref/middleware.py index b14ea33690..da3a64ddef 100644 --- a/common/djangoapps/lang_pref/middleware.py +++ b/common/djangoapps/lang_pref/middleware.py @@ -2,7 +2,7 @@ Middleware for Language Preferences """ -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from lang_pref import LANGUAGE_KEY diff --git a/common/djangoapps/lang_pref/tests/test_middleware.py b/common/djangoapps/lang_pref/tests/test_middleware.py index 68d39265b6..042e86a712 100644 --- a/common/djangoapps/lang_pref/tests/test_middleware.py +++ b/common/djangoapps/lang_pref/tests/test_middleware.py @@ -3,7 +3,7 @@ from django.test.client import RequestFactory from django.contrib.sessions.middleware import SessionMiddleware from lang_pref.middleware import LanguagePreferenceMiddleware -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from lang_pref import LANGUAGE_KEY from student.tests.factories import UserFactory diff --git a/common/djangoapps/lang_pref/tests/test_views.py b/common/djangoapps/lang_pref/tests/test_views.py index 7d6bffd2a9..daa8e89127 100644 --- a/common/djangoapps/lang_pref/tests/test_views.py +++ b/common/djangoapps/lang_pref/tests/test_views.py @@ -4,7 +4,7 @@ Tests for the language setting view from django.core.urlresolvers import reverse from django.test import TestCase from student.tests.factories import UserFactory -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from lang_pref import LANGUAGE_KEY diff --git a/common/djangoapps/lang_pref/views.py b/common/djangoapps/lang_pref/views.py index 78f99b73e0..df75166392 100644 --- a/common/djangoapps/lang_pref/views.py +++ b/common/djangoapps/lang_pref/views.py @@ -4,7 +4,7 @@ Views for accessing language preferences from django.contrib.auth.decorators import login_required from django.http import HttpResponse, HttpResponseBadRequest -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from lang_pref import LANGUAGE_KEY diff --git a/common/djangoapps/student/tests/test_create_account.py b/common/djangoapps/student/tests/test_create_account.py index de5770f298..e72dd971eb 100644 --- a/common/djangoapps/student/tests/test_create_account.py +++ b/common/djangoapps/student/tests/test_create_account.py @@ -12,7 +12,7 @@ from django.test import TestCase, TransactionTestCase import mock -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from lang_pref import LANGUAGE_KEY from edxmako.tests import mako_middleware_process_request diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 43986e320f..d61b0e719e 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -83,7 +83,7 @@ from external_auth.login_and_register import ( from bulk_email.models import Optout, CourseAuthorization import shoppingcart from shoppingcart.models import DonationConfiguration -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from lang_pref import LANGUAGE_KEY import track.views diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index 97391130ef..8b8623e2dd 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -142,8 +142,8 @@ class InheritanceMixin(XBlockMixin): # This is should be scoped to content, but since it's defined in the policy # file, it is currently scoped to settings. user_partitions = UserPartitionList( - display_name=_("Experiment Group Configurations"), - help=_("Enter the configurations that govern how students are grouped for content experiments."), + display_name=_("Group Configurations"), + help=_("Enter the configurations that govern how students are grouped together."), default=[], scope=Scope.settings ) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index c8f5b5f0ea..ca3a10a3b1 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -31,6 +31,7 @@ from xmodule.modulestore.xml_exporter import export_to_xml from xmodule.modulestore.split_mongo.split_draft import DraftVersioningModuleStore from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST from xmodule.modulestore.inheritance import InheritanceMixin +from xmodule.partitions.tests.test_partitions import PartitionTestCase from xmodule.x_module import XModuleMixin from xmodule.modulestore.xml import XMLModuleStore @@ -291,7 +292,7 @@ COURSE_DATA_NAMES = ( @ddt.ddt @attr('mongo') -class CrossStoreXMLRoundtrip(CourseComparisonTest): +class CrossStoreXMLRoundtrip(CourseComparisonTest, PartitionTestCase): """ This class exists to test XML import and export between different modulestore classes. diff --git a/common/lib/xmodule/xmodule/partitions/partitions.py b/common/lib/xmodule/xmodule/partitions/partitions.py index c2752b951f..27b6f7a75f 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions.py +++ b/common/lib/xmodule/xmodule/partitions/partitions.py @@ -1,10 +1,20 @@ """Defines ``Group`` and ``UserPartition`` models for partitioning""" + from collections import namedtuple +from stevedore.extension import ExtensionManager + # We use ``id`` in this file as the IDs of our Groups and UserPartitions, # which Pylint disapproves of. # pylint: disable=invalid-name, redefined-builtin +class UserPartitionError(Exception): + """ + An error was found regarding user partitions. + """ + pass + + class Group(namedtuple("Group", "id name")): """ An id and name for a group of students. The id should be unique @@ -45,7 +55,7 @@ class Group(namedtuple("Group", "id name")): if isinstance(value, Group): return value - for key in ('id', 'name', 'version'): + for key in ("id", "name", "version"): if key not in value: raise TypeError("Group dict {0} missing value key '{1}'".format( value, key)) @@ -57,21 +67,50 @@ class Group(namedtuple("Group", "id name")): return Group(value["id"], value["name"]) -class UserPartition(namedtuple("UserPartition", "id name description groups")): +# The Stevedore extension point namespace for user partition scheme plugins. +USER_PARTITION_SCHEME_NAMESPACE = 'openedx.user_partition_scheme' + + +class UserPartition(namedtuple("UserPartition", "id name description groups scheme")): """ A named way to partition users into groups, primarily intended for running experiments. It is expected that each user will be in at most one group in a partition. - A Partition has an id, name, description, and a list of groups. + A Partition has an id, name, scheme, description, and a list of groups. The id is intended to be unique within the context where these are used. (e.g. for - partitions of users within a course, the ids should be unique per-course) + partitions of users within a course, the ids should be unique per-course). + The scheme is used to assign users into groups. """ - VERSION = 1 + VERSION = 2 - def __new__(cls, id, name, description, groups): + # The collection of user partition scheme extensions. + scheme_extensions = None + + # The default scheme to be used when upgrading version 1 partitions. + VERSION_1_SCHEME = "random" + + def __new__(cls, id, name, description, groups, scheme=None, scheme_id=VERSION_1_SCHEME): # pylint: disable=super-on-old-class - return super(UserPartition, cls).__new__(cls, int(id), name, description, groups) + if not scheme: + scheme = UserPartition.get_scheme(scheme_id) + return super(UserPartition, cls).__new__(cls, int(id), name, description, groups, scheme) + + @staticmethod + def get_scheme(name): + """ + Returns the user partition scheme with the given name. + """ + # Note: we're creating the extension manager lazily to ensure that the Python path + # has been correctly set up. Trying to create this statically will fail, unfortunately. + if not UserPartition.scheme_extensions: + UserPartition.scheme_extensions = ExtensionManager(namespace=USER_PARTITION_SCHEME_NAMESPACE) + try: + scheme = UserPartition.scheme_extensions[name].plugin + except KeyError: + raise UserPartitionError("Unrecognized scheme {0}".format(name)) + scheme.name = name + return scheme def to_json(self): """ @@ -84,6 +123,7 @@ class UserPartition(namedtuple("UserPartition", "id name description groups")): return { "id": self.id, "name": self.name, + "scheme": self.scheme.name, "description": self.description, "groups": [g.to_json() for g in self.groups], "version": UserPartition.VERSION @@ -102,20 +142,38 @@ class UserPartition(namedtuple("UserPartition", "id name description groups")): if isinstance(value, UserPartition): return value - for key in ('id', 'name', 'description', 'version', 'groups'): + for key in ("id", "name", "description", "version", "groups"): if key not in value: - raise TypeError("UserPartition dict {0} missing value key '{1}'" - .format(value, key)) + raise TypeError("UserPartition dict {0} missing value key '{1}'".format(value, key)) - if value["version"] != UserPartition.VERSION: - raise TypeError("UserPartition dict {0} has unexpected version" - .format(value)) + if value["version"] == 1: + # If no scheme was provided, set it to the default ('random') + scheme_id = UserPartition.VERSION_1_SCHEME + elif value["version"] == UserPartition.VERSION: + if not "scheme" in value: + raise TypeError("UserPartition dict {0} missing value key 'scheme'".format(value)) + scheme_id = value["scheme"] + else: + raise TypeError("UserPartition dict {0} has unexpected version".format(value)) groups = [Group.from_json(g) for g in value["groups"]] + scheme = UserPartition.get_scheme(scheme_id) + if not scheme: + raise TypeError("UserPartition dict {0} has unrecognized scheme {1}".format(value, scheme_id)) return UserPartition( value["id"], value["name"], value["description"], - groups + groups, + scheme, ) + + def get_group(self, group_id): + """ + Returns the group with the specified id. + """ + for group in self.groups: # pylint: disable=no-member + if group.id == group_id: + return group + return None diff --git a/common/lib/xmodule/xmodule/partitions/partitions_service.py b/common/lib/xmodule/xmodule/partitions/partitions_service.py index 82b283167e..bb23ca8225 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions_service.py +++ b/common/lib/xmodule/xmodule/partitions/partitions_service.py @@ -3,7 +3,6 @@ This is a service-like API that assigns tracks which groups users are in for var user partitions. It uses the user_service key/value store provided by the LMS runtime to persist the assignments. """ -import random from abc import ABCMeta, abstractproperty @@ -22,13 +21,11 @@ class PartitionService(object): """ raise NotImplementedError('Subclasses must implement course_partition') - def __init__(self, user_tags_service, course_id, track_function): - self.random = random.Random() - self._user_tags_service = user_tags_service - self._course_id = course_id + def __init__(self, runtime, track_function): + self.runtime = runtime self._track_function = track_function - def get_user_group_for_partition(self, user_partition_id): + def get_user_group_id_for_partition(self, user_partition_id): """ If the user is already assigned to a group in user_partition_id, return the group_id. @@ -53,17 +50,15 @@ class PartitionService(object): if user_partition is None: raise ValueError( "Configuration problem! No user_partition with id {0} " - "in course {1}".format(user_partition_id, self._course_id) + "in course {1}".format(user_partition_id, self.runtime.course_id) ) - group_id = self._get_group(user_partition) - - return group_id + group = self._get_group(user_partition) + return group.id if group else None def _get_user_partition(self, user_partition_id): """ - Look for a user partition with a matching id in - in the course's partitions. + Look for a user partition with a matching id in the course's partitions. Returns: A UserPartition, or None if not found. @@ -74,65 +69,13 @@ class PartitionService(object): return None - def _key_for_partition(self, user_partition): - """ - Returns the key to use to look up and save the user's group for a particular - condition. Always use this function rather than constructing the key directly. - """ - return 'xblock.partition_service.partition_{0}'.format(user_partition.id) - def _get_group(self, user_partition): """ - Return the group of the current user in user_partition. If they don't already have - one assigned, pick one and save it. Uses the runtime's user_service service to look up - and persist the info. + Returns the group from the specified user partition to which the user is assigned. + If the user has not yet been assigned, a group will be chosen for them based upon + the partition's scheme. """ - key = self._key_for_partition(user_partition) - scope = self._user_tags_service.COURSE_SCOPE - - group_id = self._user_tags_service.get_tag(scope, key) - if group_id is not None: - group_id = int(group_id) - - partition_group_ids = [group.id for group in user_partition.groups] - - # If a valid group id has been saved already, return it - if group_id is not None and group_id in partition_group_ids: - return group_id - - # TODO: what's the atomicity of the get above and the save here? If it's not in a - # single transaction, we could get a situation where the user sees one state in one - # thread, but then that decision gets overwritten--low probability, but still bad. - - # (If it is truly atomic, we should be fine--if one process is in the - # process of finding no group and making one, the other should block till it - # appears. HOWEVER, if we allow reads by the second one while the first - # process runs the transaction, we have a problem again: could read empty, - # have the first transaction finish, and pick a different group in a - # different process.) - - # If a group id hasn't yet been saved, or the saved group id is invalid, - # we need to pick one, save it, then return it - - # TODO: had a discussion in arch council about making randomization more - # deterministic (e.g. some hash). Could do that, but need to be careful not - # to introduce correlation between users or bias in generation. - - # See note above for explanation of local_random() - group = self.random.choice(user_partition.groups) - self._user_tags_service.set_tag(scope, key, group.id) - - # emit event for analytics - # FYI - context is always user ID that is logged in, NOT the user id that is - # being operated on. If instructor can move user explicitly, then we should - # put in event_info the user id that is being operated on. - event_info = { - 'group_id': group.id, - 'group_name': group.name, - 'partition_id': user_partition.id, - 'partition_name': user_partition.name - } - # TODO: Use the XBlock publish api instead - self._track_function('xmodule.partitions.assigned_user_to_partition', event_info) - - return group.id + user = self.runtime.get_real_user(self.runtime.anonymous_student_id) + return user_partition.scheme.get_group_for_user( + self.runtime.course_id, user, user_partition, track_function=self._track_function + ) diff --git a/common/lib/xmodule/xmodule/partitions/test_partitions.py b/common/lib/xmodule/xmodule/partitions/test_partitions.py deleted file mode 100644 index 0ff667d34e..0000000000 --- a/common/lib/xmodule/xmodule/partitions/test_partitions.py +++ /dev/null @@ -1,286 +0,0 @@ -""" -Test the partitions and partitions service - -""" - -from collections import defaultdict -from unittest import TestCase -from mock import Mock - -from xmodule.partitions.partitions import Group, UserPartition -from xmodule.partitions.partitions_service import PartitionService - - -class TestGroup(TestCase): - """Test constructing groups""" - def test_construct(self): - test_id = 10 - name = "Grendel" - group = Group(test_id, name) - self.assertEqual(group.id, test_id) - self.assertEqual(group.name, name) - - def test_string_id(self): - test_id = "10" - name = "Grendel" - group = Group(test_id, name) - self.assertEqual(group.id, 10) - - def test_to_json(self): - test_id = 10 - name = "Grendel" - group = Group(test_id, name) - jsonified = group.to_json() - act_jsonified = { - "id": test_id, - "name": name, - "version": group.VERSION - } - self.assertEqual(jsonified, act_jsonified) - - def test_from_json(self): - test_id = 5 - name = "Grendel" - jsonified = { - "id": test_id, - "name": name, - "version": Group.VERSION - } - group = Group.from_json(jsonified) - self.assertEqual(group.id, test_id) - self.assertEqual(group.name, name) - - def test_from_json_broken(self): - test_id = 5 - name = "Grendel" - # Bad version - jsonified = { - "id": test_id, - "name": name, - "version": 9001 - } - with self.assertRaisesRegexp(TypeError, "has unexpected version"): - group = Group.from_json(jsonified) - - # Missing key "id" - jsonified = { - "name": name, - "version": Group.VERSION - } - with self.assertRaisesRegexp(TypeError, "missing value key 'id'"): - group = Group.from_json(jsonified) - - # Has extra key - should not be a problem - jsonified = { - "id": test_id, - "name": name, - "version": Group.VERSION, - "programmer": "Cale" - } - group = Group.from_json(jsonified) - self.assertNotIn("programmer", group.to_json()) - - -class TestUserPartition(TestCase): - """Test constructing UserPartitions""" - def test_construct(self): - groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] - user_partition = UserPartition(0, 'Test Partition', 'for testing purposes', groups) - self.assertEqual(user_partition.id, 0) - self.assertEqual(user_partition.name, "Test Partition") - self.assertEqual(user_partition.description, "for testing purposes") - self.assertEqual(user_partition.groups, groups) - - def test_string_id(self): - groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] - user_partition = UserPartition("70", 'Test Partition', 'for testing purposes', groups) - self.assertEqual(user_partition.id, 70) - - def test_to_json(self): - groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] - upid = 0 - upname = "Test Partition" - updesc = "for testing purposes" - user_partition = UserPartition(upid, upname, updesc, groups) - - jsonified = user_partition.to_json() - act_jsonified = { - "id": upid, - "name": upname, - "description": updesc, - "groups": [group.to_json() for group in groups], - "version": user_partition.VERSION - } - self.assertEqual(jsonified, act_jsonified) - - def test_from_json(self): - groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] - upid = 1 - upname = "Test Partition" - updesc = "For Testing Purposes" - - jsonified = { - "id": upid, - "name": upname, - "description": updesc, - "groups": [group.to_json() for group in groups], - "version": UserPartition.VERSION - } - user_partition = UserPartition.from_json(jsonified) - self.assertEqual(user_partition.id, upid) - self.assertEqual(user_partition.name, upname) - self.assertEqual(user_partition.description, updesc) - for act_group in user_partition.groups: - self.assertIn(act_group.id, [0, 1]) - exp_group = groups[act_group.id] - self.assertEqual(exp_group.id, act_group.id) - self.assertEqual(exp_group.name, act_group.name) - - def test_from_json_broken(self): - groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] - upid = 1 - upname = "Test Partition" - updesc = "For Testing Purposes" - - # Missing field - jsonified = { - "name": upname, - "description": updesc, - "groups": [group.to_json() for group in groups], - "version": UserPartition.VERSION - } - with self.assertRaisesRegexp(TypeError, "missing value key 'id'"): - user_partition = UserPartition.from_json(jsonified) - - # Wrong version (it's over 9000!) - jsonified = { - 'id': upid, - "name": upname, - "description": updesc, - "groups": [group.to_json() for group in groups], - "version": 9001 - } - with self.assertRaisesRegexp(TypeError, "has unexpected version"): - user_partition = UserPartition.from_json(jsonified) - - # Has extra key - should not be a problem - jsonified = { - 'id': upid, - "name": upname, - "description": updesc, - "groups": [group.to_json() for group in groups], - "version": UserPartition.VERSION, - "programmer": "Cale" - } - user_partition = UserPartition.from_json(jsonified) - self.assertNotIn("programmer", user_partition.to_json()) - - -class StaticPartitionService(PartitionService): - """ - Mock PartitionService for testing. - """ - def __init__(self, partitions, **kwargs): - super(StaticPartitionService, self).__init__(**kwargs) - self._partitions = partitions - - @property - def course_partitions(self): - return self._partitions - - -class MemoryUserTagsService(object): - """ - An implementation of a user_tags XBlock service that - uses an in-memory dictionary for storage - """ - COURSE_SCOPE = 'course' - - def __init__(self): - self._tags = defaultdict(dict) - - def get_tag(self, scope, key): - """Sets the value of ``key`` to ``value``""" - print 'GETTING', scope, key, self._tags - return self._tags[scope].get(key) - - def set_tag(self, scope, key, value): - """Gets the value of ``key``""" - self._tags[scope][key] = value - print 'SET', scope, key, value, self._tags - - -class TestPartitionsService(TestCase): - """ - Test getting a user's group out of a partition - """ - - def setUp(self): - groups = [Group(0, 'Group 1'), Group(1, 'Group 2')] - self.partition_id = 0 - - self.user_tags_service = MemoryUserTagsService() - - user_partition = UserPartition(self.partition_id, 'Test Partition', 'for testing purposes', groups) - self.partitions_service = StaticPartitionService( - [user_partition], - user_tags_service=self.user_tags_service, - course_id=Mock(), - track_function=Mock() - ) - - def test_get_user_group_for_partition(self): - # get a group assigned to the user - group1 = self.partitions_service.get_user_group_for_partition(self.partition_id) - - # make sure we get the same group back out if we try a second time - group2 = self.partitions_service.get_user_group_for_partition(self.partition_id) - - self.assertEqual(group1, group2) - - # test that we error if given an invalid partition id - with self.assertRaises(ValueError): - self.partitions_service.get_user_group_for_partition(3) - - def test_user_in_deleted_group(self): - # get a group assigned to the user - should be group 0 or 1 - old_group = self.partitions_service.get_user_group_for_partition(self.partition_id) - self.assertIn(old_group, [0, 1]) - - # Change the group definitions! No more group 0 or 1 - groups = [Group(3, 'Group 3'), Group(4, 'Group 4')] - user_partition = UserPartition(self.partition_id, 'Test Partition', 'for testing purposes', groups) - self.partitions_service = StaticPartitionService( - [user_partition], - user_tags_service=self.user_tags_service, - course_id=Mock(), - track_function=Mock() - ) - - # Now, get a new group using the same call - should be 3 or 4 - new_group = self.partitions_service.get_user_group_for_partition(self.partition_id) - self.assertIn(new_group, [3, 4]) - - # We should get the same group over multiple calls - new_group_2 = self.partitions_service.get_user_group_for_partition(self.partition_id) - self.assertEqual(new_group, new_group_2) - - def test_change_group_name(self): - # Changing the name of the group shouldn't affect anything - # get a group assigned to the user - should be group 0 or 1 - old_group = self.partitions_service.get_user_group_for_partition(self.partition_id) - self.assertIn(old_group, [0, 1]) - - # Change the group names - groups = [Group(0, 'Group 0'), Group(1, 'Group 1')] - user_partition = UserPartition(self.partition_id, 'Test Partition', 'for testing purposes', groups) - self.partitions_service = StaticPartitionService( - [user_partition], - user_tags_service=self.user_tags_service, - course_id=Mock(), - track_function=Mock() - ) - - # Now, get a new group using the same call - new_group = self.partitions_service.get_user_group_for_partition(self.partition_id) - self.assertEqual(old_group, new_group) diff --git a/common/djangoapps/user_api/__init__.py b/common/lib/xmodule/xmodule/partitions/tests/__init__.py similarity index 100% rename from common/djangoapps/user_api/__init__.py rename to common/lib/xmodule/xmodule/partitions/tests/__init__.py diff --git a/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py b/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py new file mode 100644 index 0000000000..6b13b7cb4e --- /dev/null +++ b/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py @@ -0,0 +1,302 @@ +""" +Test the partitions and partitions service + +""" + +from unittest import TestCase +from mock import Mock + +from stevedore.extension import Extension, ExtensionManager +from xmodule.partitions.partitions import Group, UserPartition, UserPartitionError, USER_PARTITION_SCHEME_NAMESPACE +from xmodule.partitions.partitions_service import PartitionService +from xmodule.tests import get_test_system + + +class TestGroup(TestCase): + """Test constructing groups""" + def test_construct(self): + test_id = 10 + name = "Grendel" + group = Group(test_id, name) + self.assertEqual(group.id, test_id) # pylint: disable=no-member + self.assertEqual(group.name, name) + + def test_string_id(self): + test_id = "10" + name = "Grendel" + group = Group(test_id, name) + self.assertEqual(group.id, 10) # pylint: disable=no-member + + def test_to_json(self): + test_id = 10 + name = "Grendel" + group = Group(test_id, name) + jsonified = group.to_json() + act_jsonified = { + "id": test_id, + "name": name, + "version": group.VERSION + } + self.assertEqual(jsonified, act_jsonified) + + def test_from_json(self): + test_id = 5 + name = "Grendel" + jsonified = { + "id": test_id, + "name": name, + "version": Group.VERSION + } + group = Group.from_json(jsonified) + self.assertEqual(group.id, test_id) # pylint: disable=no-member + self.assertEqual(group.name, name) + + def test_from_json_broken(self): + test_id = 5 + name = "Grendel" + # Bad version + jsonified = { + "id": test_id, + "name": name, + "version": 9001 + } + with self.assertRaisesRegexp(TypeError, "has unexpected version"): + Group.from_json(jsonified) + + # Missing key "id" + jsonified = { + "name": name, + "version": Group.VERSION + } + with self.assertRaisesRegexp(TypeError, "missing value key 'id'"): + Group.from_json(jsonified) + + # Has extra key - should not be a problem + jsonified = { + "id": test_id, + "name": name, + "version": Group.VERSION, + "programmer": "Cale" + } + group = Group.from_json(jsonified) + self.assertNotIn("programmer", group.to_json()) + + +class MockUserPartitionScheme(object): + """ + Mock user partition scheme + """ + def __init__(self, name="mock", current_group=None, **kwargs): + super(MockUserPartitionScheme, self).__init__(**kwargs) + self.name = name + self.current_group = current_group + + def get_group_for_user(self, course_id, user, user_partition, track_function=None): # pylint: disable=unused-argument + """ + Returns the current group if set, else the first group from the specified user partition. + """ + if self.current_group: + return self.current_group + groups = user_partition.groups + if not groups or len(groups) == 0: + return None + return groups[0] + + +class PartitionTestCase(TestCase): + """Base class for test cases that require partitions""" + TEST_ID = 0 + TEST_NAME = "Mock Partition" + TEST_DESCRIPTION = "for testing purposes" + TEST_GROUPS = [Group(0, 'Group 1'), Group(1, 'Group 2')] + TEST_SCHEME_NAME = "mock" + + def setUp(self): + # Set up two user partition schemes: mock and random + extensions = [ + Extension( + self.TEST_SCHEME_NAME, USER_PARTITION_SCHEME_NAMESPACE, + MockUserPartitionScheme(self.TEST_SCHEME_NAME), None + ), + Extension( + "random", USER_PARTITION_SCHEME_NAMESPACE, MockUserPartitionScheme("random"), None + ), + ] + UserPartition.scheme_extensions = ExtensionManager.make_test_instance( + extensions, namespace=USER_PARTITION_SCHEME_NAMESPACE + ) + + # Create a test partition + self.user_partition = UserPartition( + self.TEST_ID, + self.TEST_NAME, + self.TEST_DESCRIPTION, + self.TEST_GROUPS, + extensions[0].plugin + ) + + +class TestUserPartition(PartitionTestCase): + """Test constructing UserPartitions""" + + def test_construct(self): + user_partition = UserPartition( + self.TEST_ID, self.TEST_NAME, self.TEST_DESCRIPTION, self.TEST_GROUPS, MockUserPartitionScheme() + ) + self.assertEqual(user_partition.id, self.TEST_ID) # pylint: disable=no-member + self.assertEqual(user_partition.name, self.TEST_NAME) + self.assertEqual(user_partition.description, self.TEST_DESCRIPTION) # pylint: disable=no-member + self.assertEqual(user_partition.groups, self.TEST_GROUPS) # pylint: disable=no-member + self.assertEquals(user_partition.scheme.name, self.TEST_SCHEME_NAME) # pylint: disable=no-member + + def test_string_id(self): + user_partition = UserPartition( + "70", self.TEST_NAME, self.TEST_DESCRIPTION, self.TEST_GROUPS + ) + self.assertEqual(user_partition.id, 70) # pylint: disable=no-member + + def test_to_json(self): + jsonified = self.user_partition.to_json() + act_jsonified = { + "id": self.TEST_ID, + "name": self.TEST_NAME, + "description": self.TEST_DESCRIPTION, + "groups": [group.to_json() for group in self.TEST_GROUPS], + "version": self.user_partition.VERSION, + "scheme": self.TEST_SCHEME_NAME + } + self.assertEqual(jsonified, act_jsonified) + + def test_from_json(self): + jsonified = { + "id": self.TEST_ID, + "name": self.TEST_NAME, + "description": self.TEST_DESCRIPTION, + "groups": [group.to_json() for group in self.TEST_GROUPS], + "version": UserPartition.VERSION, + "scheme": "mock", + } + user_partition = UserPartition.from_json(jsonified) + self.assertEqual(user_partition.id, self.TEST_ID) # pylint: disable=no-member + self.assertEqual(user_partition.name, self.TEST_NAME) # pylint: disable=no-member + self.assertEqual(user_partition.description, self.TEST_DESCRIPTION) # pylint: disable=no-member + for act_group in user_partition.groups: # pylint: disable=no-member + self.assertIn(act_group.id, [0, 1]) + exp_group = self.TEST_GROUPS[act_group.id] + self.assertEqual(exp_group.id, act_group.id) + self.assertEqual(exp_group.name, act_group.name) + + def test_version_upgrade(self): + # Version 1 partitions did not have a scheme specified + jsonified = { + "id": self.TEST_ID, + "name": self.TEST_NAME, + "description": self.TEST_DESCRIPTION, + "groups": [group.to_json() for group in self.TEST_GROUPS], + "version": 1, + } + user_partition = UserPartition.from_json(jsonified) + self.assertEqual(user_partition.scheme.name, "random") # pylint: disable=no-member + + def test_from_json_broken(self): + # Missing field + jsonified = { + "name": self.TEST_NAME, + "description": self.TEST_DESCRIPTION, + "groups": [group.to_json() for group in self.TEST_GROUPS], + "version": UserPartition.VERSION, + "scheme": self.TEST_SCHEME_NAME, + } + with self.assertRaisesRegexp(TypeError, "missing value key 'id'"): + UserPartition.from_json(jsonified) + + # Missing scheme + jsonified = { + 'id': self.TEST_ID, + "name": self.TEST_NAME, + "description": self.TEST_DESCRIPTION, + "groups": [group.to_json() for group in self.TEST_GROUPS], + "version": UserPartition.VERSION, + } + with self.assertRaisesRegexp(TypeError, "missing value key 'scheme'"): + UserPartition.from_json(jsonified) + + # Invalid scheme + jsonified = { + 'id': self.TEST_ID, + "name": self.TEST_NAME, + "description": self.TEST_DESCRIPTION, + "groups": [group.to_json() for group in self.TEST_GROUPS], + "version": UserPartition.VERSION, + "scheme": "no_such_scheme", + } + with self.assertRaisesRegexp(UserPartitionError, "Unrecognized scheme"): + UserPartition.from_json(jsonified) + + # Wrong version (it's over 9000!) + # Wrong version (it's over 9000!) + jsonified = { + 'id': self.TEST_ID, + "name": self.TEST_NAME, + "description": self.TEST_DESCRIPTION, + "groups": [group.to_json() for group in self.TEST_GROUPS], + "version": 9001, + "scheme": self.TEST_SCHEME_NAME, + } + with self.assertRaisesRegexp(TypeError, "has unexpected version"): + UserPartition.from_json(jsonified) + + # Has extra key - should not be a problem + jsonified = { + 'id': self.TEST_ID, + "name": self.TEST_NAME, + "description": self.TEST_DESCRIPTION, + "groups": [group.to_json() for group in self.TEST_GROUPS], + "version": UserPartition.VERSION, + "scheme": "mock", + "programmer": "Cale", + } + user_partition = UserPartition.from_json(jsonified) + self.assertNotIn("programmer", user_partition.to_json()) + + +class StaticPartitionService(PartitionService): + """ + Mock PartitionService for testing. + """ + def __init__(self, partitions, **kwargs): + super(StaticPartitionService, self).__init__(**kwargs) + self._partitions = partitions + + @property + def course_partitions(self): + return self._partitions + + +class TestPartitionService(PartitionTestCase): + """ + Test getting a user's group out of a partition + """ + + def setUp(self): + super(TestPartitionService, self).setUp() + self.partition_service = StaticPartitionService( + [self.user_partition], + runtime=get_test_system(), + track_function=Mock() + ) + + def test_get_user_group_id_for_partition(self): + # assign the first group to be returned + user_partition_id = self.user_partition.id # pylint: disable=no-member + groups = self.user_partition.groups # pylint: disable=no-member + self.user_partition.scheme.current_group = groups[0] # pylint: disable=no-member + + # get a group assigned to the user + group1_id = self.partition_service.get_user_group_id_for_partition(user_partition_id) + self.assertEqual(group1_id, groups[0].id) # pylint: disable=no-member + + # switch to the second group and verify that it is returned for the user + self.user_partition.scheme.current_group = groups[1] # pylint: disable=no-member + group2_id = self.partition_service.get_user_group_id_for_partition(user_partition_id) + self.assertEqual(group2_id, groups[1].id) # pylint: disable=no-member diff --git a/common/lib/xmodule/xmodule/split_test_module.py b/common/lib/xmodule/xmodule/split_test_module.py index 3dd59bfd68..a56b1e26bf 100644 --- a/common/lib/xmodule/xmodule/split_test_module.py +++ b/common/lib/xmodule/xmodule/split_test_module.py @@ -188,7 +188,7 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): partitions_service = self.runtime.service(self, 'partitions') if not partitions_service: return None - return partitions_service.get_user_group_for_partition(self.user_partition_id) + return partitions_service.get_user_group_id_for_partition(self.user_partition_id) @property def is_configured(self): diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 4c8ff94f76..44d9fc0123 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -79,13 +79,15 @@ def get_test_system(course_id=SlashSeparatedCourseKey('org', 'course', 'run')): where `my_render_func` is a function of the form my_render_func(template, context). """ + user = Mock(is_staff=False) return TestModuleSystem( static_url='/static', track_function=Mock(), get_module=Mock(), render_template=mock_render_template, replace_urls=str, - user=Mock(is_staff=False), + user=user, + get_real_user=lambda(__): user, filestore=Mock(), debug=True, hostname="edx.org", diff --git a/common/lib/xmodule/xmodule/tests/test_split_test_module.py b/common/lib/xmodule/xmodule/tests/test_split_test_module.py index b82310c845..36755a0308 100644 --- a/common/lib/xmodule/xmodule/tests/test_split_test_module.py +++ b/common/lib/xmodule/xmodule/tests/test_split_test_module.py @@ -6,6 +6,7 @@ import lxml from mock import Mock, patch from fs.memoryfs import MemoryFS +from xmodule.partitions.tests.test_partitions import StaticPartitionService, PartitionTestCase, MockUserPartitionScheme from xmodule.tests.xml import factories as xml from xmodule.tests.xml import XModuleXmlImportTest from xmodule.tests import get_test_system @@ -13,7 +14,6 @@ from xmodule.x_module import AUTHOR_VIEW, STUDENT_VIEW from xmodule.validation import StudioValidationMessage from xmodule.split_test_module import SplitTestDescriptor, SplitTestFields from xmodule.partitions.partitions import Group, UserPartition -from xmodule.partitions.test_partitions import StaticPartitionService, MemoryUserTagsService class SplitTestModuleFactory(xml.XmlImportFactory): @@ -23,11 +23,12 @@ class SplitTestModuleFactory(xml.XmlImportFactory): tag = 'split_test' -class SplitTestModuleTest(XModuleXmlImportTest): +class SplitTestModuleTest(XModuleXmlImportTest, PartitionTestCase): """ Base class for all split_module tests. """ def setUp(self): + super(SplitTestModuleTest, self).setUp() self.course_id = 'test_org/test_course_number/test_run' # construct module course = xml.CourseFactory.build() @@ -57,16 +58,16 @@ class SplitTestModuleTest(XModuleXmlImportTest): self.module_system.descriptor_system = self.course.runtime self.course.runtime.export_fs = MemoryFS() - self.tags_service = MemoryUserTagsService() - self.module_system._services['user_tags'] = self.tags_service # pylint: disable=protected-access - self.partitions_service = StaticPartitionService( [ - UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta')]), - UserPartition(1, 'second_partition', 'Second Partition', [Group("0", 'abel'), Group("1", 'baker'), Group("2", 'charlie')]) + self.user_partition, + UserPartition( + 1, 'second_partition', 'Second Partition', + [Group("0", 'abel'), Group("1", 'baker'), Group("2", 'charlie')], + MockUserPartitionScheme() + ) ], - user_tags_service=self.tags_service, - course_id=self.course.id, + runtime=self.module_system, track_function=Mock(name='track_function'), ) self.module_system._services['partitions'] = self.partitions_service # pylint: disable=protected-access @@ -81,50 +82,28 @@ class SplitTestModuleLMSTest(SplitTestModuleTest): Test the split test module """ - @ddt.data(('0', 'split_test_cond0'), ('1', 'split_test_cond1')) + @ddt.data((0, 'split_test_cond0'), (1, 'split_test_cond1')) @ddt.unpack def test_child(self, user_tag, child_url_name): - self.tags_service.set_tag( - self.tags_service.COURSE_SCOPE, - 'xblock.partition_service.partition_0', - user_tag - ) - + self.user_partition.scheme.current_group = self.user_partition.groups[user_tag] # pylint: disable=no-member self.assertEquals(self.split_test_module.child_descriptor.url_name, child_url_name) - @ddt.data(('0',), ('1',)) - @ddt.unpack - def test_child_old_tag_value(self, _user_tag): - # If user_tag has a stale value, we should still get back a valid child url - self.tags_service.set_tag( - self.tags_service.COURSE_SCOPE, - 'xblock.partition_service.partition_0', - '2' - ) - - self.assertIn(self.split_test_module.child_descriptor.url_name, ['split_test_cond0', 'split_test_cond1']) - - @ddt.data(('0', 'HTML FOR GROUP 0'), ('1', 'HTML FOR GROUP 1')) + @ddt.data((0, 'HTML FOR GROUP 0'), (1, 'HTML FOR GROUP 1')) @ddt.unpack def test_get_html(self, user_tag, child_content): - self.tags_service.set_tag( - self.tags_service.COURSE_SCOPE, - 'xblock.partition_service.partition_0', - user_tag - ) - + self.user_partition.scheme.current_group = self.user_partition.groups[user_tag] # pylint: disable=no-member self.assertIn( child_content, self.module_system.render(self.split_test_module, STUDENT_VIEW).content ) - @ddt.data(('0',), ('1',)) + @ddt.data((0,), (1,)) @ddt.unpack def test_child_missing_tag_value(self, _user_tag): # If user_tag has a missing value, we should still get back a valid child url self.assertIn(self.split_test_module.child_descriptor.url_name, ['split_test_cond0', 'split_test_cond1']) - @ddt.data(('100',), ('200',), ('300',), ('400',), ('500',), ('600',), ('700',), ('800',), ('900',), ('1000',)) + @ddt.data((100,), (200,), (300,), (400,), (500,), (600,), (700,), (800,), (900,), (1000,)) @ddt.unpack def test_child_persist_new_tag_value_when_tag_missing(self, _user_tag): # If a user_tag has a missing value, a group should be saved/persisted for that user. diff --git a/common/test/acceptance/.coveragerc b/common/test/acceptance/.coveragerc index 87fe2964fe..32a237f3bb 100644 --- a/common/test/acceptance/.coveragerc +++ b/common/test/acceptance/.coveragerc @@ -1,7 +1,7 @@ [run] data_file = reports/bok_choy/.coverage source = lms, cms, common/djangoapps, common/lib -omit = lms/envs/*, cms/envs/*, common/djangoapps/terrain/*, common/djangoapps/*/migrations/*, */test*, */management/*, */urls*, */wsgi* +omit = lms/envs/*, cms/envs/*, common/djangoapps/terrain/*, common/djangoapps/*/migrations/*, openedx/core/djangoapps/*/migrations/*, */test*, */management/*, */urls*, */wsgi* parallel = True [report] diff --git a/common/test/acceptance/tests/studio/test_studio_split_test.py b/common/test/acceptance/tests/studio/test_studio_split_test.py index 1fabdff540..e3a0861b78 100644 --- a/common/test/acceptance/tests/studio/test_studio_split_test.py +++ b/common/test/acceptance/tests/studio/test_studio_split_test.py @@ -9,6 +9,7 @@ from nose.plugins.attrib import attr from selenium.webdriver.support.ui import Select from xmodule.partitions.partitions import Group, UserPartition +from xmodule.partitions.tests.test_partitions import MockUserPartitionScheme from bok_choy.promise import Promise, EmptyPromise from ...fixtures.course import XBlockFixtureDesc @@ -30,6 +31,16 @@ class SplitTestMixin(object): """ Mixin that contains useful methods for split_test module testing. """ + @staticmethod + def create_user_partition_json(partition_id, name, description, groups): + """ + Helper method to create user partition JSON. + """ + return UserPartition( + partition_id, name, description, groups, MockUserPartitionScheme("random") + ).to_json() + + def verify_groups(self, container, active_groups, inactive_groups, verify_missing_groups_not_present=True): """ Check that the groups appear and are correctly categorized as to active and inactive. @@ -80,8 +91,18 @@ class SplitTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, 'Configuration alpha,beta', 'first', [Group("0", 'alpha'), Group("1", 'beta')]).to_json(), - UserPartition(1, 'Configuration 0,1,2', 'second', [Group("0", 'Group 0'), Group("1", 'Group 1'), Group("2", 'Group 2')]).to_json() + self.create_user_partition_json( + 0, + 'Configuration alpha,beta', + 'first', + [Group("0", 'alpha'), Group("1", 'beta')] + ), + self.create_user_partition_json( + 1, + 'Configuration 0,1,2', + 'second', + [Group("0", 'Group 0'), Group("1", 'Group 1'), Group("2", 'Group 2')] + ), ], }, }) @@ -124,8 +145,12 @@ class SplitTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, 'Configuration alpha,beta', 'first', - [Group("0", 'alpha'), Group("2", 'gamma')]).to_json() + self.create_user_partition_json( + 0, + 'Configuration alpha,beta', + 'first', + [Group("0", 'alpha'), Group("2", 'gamma')] + ) ], }, }) @@ -189,7 +214,7 @@ class SplitTest(ContainerBase, SplitTestMixin): @attr('shard_1') class SettingsMenuTest(StudioCourseTest): """ - Tests that Setting menu is rendered correctly in Studio + Tests that Settings menu is rendered correctly in Studio """ def setUp(self): @@ -324,7 +349,7 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, "Name", "Description.", groups).to_json(), + self.create_user_partition_json(0, "Name", "Description.", groups), ], }, }) @@ -396,8 +421,18 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, 'Name of the Group Configuration', 'Description of the group configuration.', [Group("0", 'Group 0'), Group("1", 'Group 1')]).to_json(), - UserPartition(1, 'Name of second Group Configuration', 'Second group configuration.', [Group("0", 'Alpha'), Group("1", 'Beta'), Group("2", 'Gamma')]).to_json(), + self.create_user_partition_json( + 0, + 'Name of the Group Configuration', + 'Description of the group configuration.', + [Group("0", 'Group 0'), Group("1", 'Group 1')] + ), + self.create_user_partition_json( + 1, + 'Name of second Group Configuration', + 'Second group configuration.', + [Group("0", 'Alpha'), Group("1", 'Beta'), Group("2", 'Gamma')] + ), ], }, }) @@ -531,7 +566,12 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, 'Name of the Group Configuration', 'Description of the group configuration.', [Group("0", 'Group A'), Group("1", 'Group B'), Group("2", 'Group C')]).to_json(), + self.create_user_partition_json( + 0, + 'Name of the Group Configuration', + 'Description of the group configuration.', + [Group("0", 'Group A'), Group("1", 'Group B'), Group("2", 'Group C')] + ), ], }, }) @@ -610,8 +650,18 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, 'Name of the Group Configuration', 'Description of the group configuration.', [Group("0", 'Group 0'), Group("1", 'Group 1')]).to_json(), - UserPartition(1, 'Name of second Group Configuration', 'Second group configuration.', [Group("0", 'Alpha'), Group("1", 'Beta'), Group("2", 'Gamma')]).to_json(), + self.create_user_partition_json( + 0, + 'Name of the Group Configuration', + 'Description of the group configuration.', + [Group("0", 'Group 0'), Group("1", 'Group 1')] + ), + self.create_user_partition_json( + 1, + 'Name of second Group Configuration', + 'Second group configuration.', + [Group("0", 'Alpha'), Group("1", 'Beta'), Group("2", 'Gamma')] + ), ], }, }) @@ -696,7 +746,12 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, "Name", "Description.", [Group("0", "Group A"), Group("1", "Group B")]).to_json(), + self.create_user_partition_json( + 0, + "Name", + "Description.", + [Group("0", "Group A"), Group("1", "Group B")] + ), ], }, }) @@ -728,7 +783,12 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, "Name", "Description.", [Group("0", "Group A"), Group("1", "Group B")]).to_json(), + self.create_user_partition_json( + 0, + "Name", + "Description.", + [Group("0", "Group A"), Group("1", "Group B")] + ), ], }, }) @@ -771,8 +831,18 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, 'Configuration 1', 'Description of the group configuration.', [Group("0", 'Group 0'), Group("1", 'Group 1')]).to_json(), - UserPartition(1, 'Configuration 2', 'Second group configuration.', [Group("0", 'Alpha'), Group("1", 'Beta'), Group("2", 'Gamma')]).to_json() + self.create_user_partition_json( + 0, + 'Configuration 1', + 'Description of the group configuration.', + [Group("0", 'Group 0'), Group("1", 'Group 1')] + ), + self.create_user_partition_json( + 1, + 'Configuration 2', + 'Second group configuration.', + [Group("0", 'Alpha'), Group("1", 'Beta'), Group("2", 'Gamma')] + ) ], }, }) @@ -804,7 +874,12 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, "Name", "Description.", [Group("0", "Group A"), Group("1", "Group B")]).to_json() + self.create_user_partition_json( + 0, + "Name", + "Description.", + [Group("0", "Group A"), Group("1", "Group B")] + ) ], }, }) @@ -840,8 +915,18 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin): self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ - UserPartition(0, "Name", "Description.", [Group("0", "Group A"), Group("1", "Group B")]).to_json(), - UserPartition(1, 'Name of second Group Configuration', 'Second group configuration.', [Group("0", 'Alpha'), Group("1", 'Beta'), Group("2", 'Gamma')]).to_json(), + self.create_user_partition_json( + 0, + "Name", + "Description.", + [Group("0", "Group A"), Group("1", "Group B")] + ), + self.create_user_partition_json( + 1, + 'Name of second Group Configuration', + 'Second group configuration.', + [Group("0", 'Alpha'), Group("1", 'Beta'), Group("2", 'Gamma')] + ), ], }, }) diff --git a/lms/.coveragerc b/lms/.coveragerc index 72b7b037ef..3d5d9fd0af 100644 --- a/lms/.coveragerc +++ b/lms/.coveragerc @@ -2,7 +2,7 @@ [run] data_file = reports/lms/.coverage source = lms,common/djangoapps -omit = lms/envs/*, common/djangoapps/terrain/*, common/djangoapps/*/migrations/* +omit = lms/envs/*, common/djangoapps/terrain/*, common/djangoapps/*/migrations/*, openedx/core/djangoapps/*/migrations/* [report] ignore_errors = True diff --git a/lms/djangoapps/courseware/tests/test_split_module.py b/lms/djangoapps/courseware/tests/test_split_module.py index 2fe280c12c..ee72589459 100644 --- a/lms/djangoapps/courseware/tests/test_split_module.py +++ b/lms/djangoapps/courseware/tests/test_split_module.py @@ -12,7 +12,7 @@ from student.tests.factories import UserFactory, CourseEnrollmentFactory from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.partitions.partitions import Group, UserPartition -from user_api.tests.factories import UserCourseTagFactory +from openedx.core.djangoapps.user_api.tests.factories import UserCourseTagFactory @override_settings(MODULESTORE=TEST_DATA_MOCK_MODULESTORE) diff --git a/lms/djangoapps/courseware/tests/test_submitting_problems.py b/lms/djangoapps/courseware/tests/test_submitting_problems.py index a51aaa6ad5..904204764b 100644 --- a/lms/djangoapps/courseware/tests/test_submitting_problems.py +++ b/lms/djangoapps/courseware/tests/test_submitting_problems.py @@ -27,7 +27,7 @@ from student.models import anonymous_id_for_user from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.partitions.partitions import Group, UserPartition -from user_api.tests.factories import UserCourseTagFactory +from openedx.core.djangoapps.user_api.tests.factories import UserCourseTagFactory @override_settings(MODULESTORE=TEST_DATA_MOCK_MODULESTORE) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 60caded8b5..a5efa2b738 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -62,7 +62,7 @@ import instructor_analytics.basic import instructor_analytics.distributions import instructor_analytics.csvs import csv -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from instructor.views import INVOICE_KEY from submissions import api as sub_api # installed from the edx-submissions repository diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index 5d386ddb92..eee8f124d3 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -17,7 +17,7 @@ from django.core.urlresolvers import reverse from capa.tests.response_xml_factory import (CodeResponseXMLFactory, CustomResponseXMLFactory) -from user_api.tests.factories import UserCourseTagFactory +from openedx.core.djangoapps.user_api.tests.factories import UserCourseTagFactory from xmodule.modulestore.tests.factories import ItemFactory from xmodule.modulestore import ModuleStoreEnum from xmodule.partitions.partitions import Group, UserPartition diff --git a/lms/djangoapps/notification_prefs/features/unsubscribe.py b/lms/djangoapps/notification_prefs/features/unsubscribe.py index 1eaab95177..987211b0cd 100644 --- a/lms/djangoapps/notification_prefs/features/unsubscribe.py +++ b/lms/djangoapps/notification_prefs/features/unsubscribe.py @@ -1,7 +1,7 @@ from django.contrib.auth.models import User from lettuce import step, world from notification_prefs import NOTIFICATION_PREF_KEY -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference USERNAME = "robot" diff --git a/lms/djangoapps/notification_prefs/tests.py b/lms/djangoapps/notification_prefs/tests.py index 3206dfaabc..91e84b063c 100644 --- a/lms/djangoapps/notification_prefs/tests.py +++ b/lms/djangoapps/notification_prefs/tests.py @@ -12,7 +12,7 @@ from notification_prefs import NOTIFICATION_PREF_KEY from notification_prefs.views import ajax_enable, ajax_disable, ajax_status, set_subscription, UsernameCipher from student.tests.factories import UserFactory from edxmako.tests import mako_middleware_process_request -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from util.testing import UrlResetMixin diff --git a/lms/djangoapps/notification_prefs/views.py b/lms/djangoapps/notification_prefs/views.py index eef24c5ee4..3cada57beb 100644 --- a/lms/djangoapps/notification_prefs/views.py +++ b/lms/djangoapps/notification_prefs/views.py @@ -12,7 +12,7 @@ from django.views.decorators.http import require_GET, require_POST from edxmako.shortcuts import render_to_response from notification_prefs import NOTIFICATION_PREF_KEY -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference class UsernameDecryptionException(Exception): diff --git a/lms/djangoapps/notifier_api/tests.py b/lms/djangoapps/notifier_api/tests.py index 5005781342..545adaea10 100644 --- a/lms/djangoapps/notifier_api/tests.py +++ b/lms/djangoapps/notifier_api/tests.py @@ -13,8 +13,8 @@ from notifier_api.views import NotifierUsersViewSet from opaque_keys.edx.locator import CourseLocator from student.models import CourseEnrollment from student.tests.factories import UserFactory, CourseEnrollmentFactory -from user_api.models import UserPreference -from user_api.tests.factories import UserPreferenceFactory +from openedx.core.djangoapps.user_api.models import UserPreference +from openedx.core.djangoapps.user_api.tests.factories import UserPreferenceFactory from util.testing import UrlResetMixin from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory diff --git a/lms/djangoapps/notifier_api/views.py b/lms/djangoapps/notifier_api/views.py index 84a38639a9..9be79b0717 100644 --- a/lms/djangoapps/notifier_api/views.py +++ b/lms/djangoapps/notifier_api/views.py @@ -3,7 +3,7 @@ from rest_framework.viewsets import ReadOnlyModelViewSet from notification_prefs import NOTIFICATION_PREF_KEY from notifier_api.serializers import NotifierUserSerializer -from user_api.views import ApiKeyHeaderPermission +from openedx.core.djangoapps.user_api.views import ApiKeyHeaderPermission class NotifierUsersViewSet(ReadOnlyModelViewSet): diff --git a/lms/djangoapps/oauth2_handler/handlers.py b/lms/djangoapps/oauth2_handler/handlers.py index b44ab7403f..fe1957bbee 100644 --- a/lms/djangoapps/oauth2_handler/handlers.py +++ b/lms/djangoapps/oauth2_handler/handlers.py @@ -6,7 +6,7 @@ from django.core.cache import cache from courseware.access import has_access from student.models import anonymous_id_for_user from student.models import UserProfile -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from lang_pref import LANGUAGE_KEY from xmodule.modulestore.django import modulestore from xmodule.course_module import CourseDescriptor diff --git a/lms/djangoapps/oauth2_handler/tests.py b/lms/djangoapps/oauth2_handler/tests.py index 36d7fbfd49..b1676e948f 100644 --- a/lms/djangoapps/oauth2_handler/tests.py +++ b/lms/djangoapps/oauth2_handler/tests.py @@ -9,7 +9,7 @@ from student.models import anonymous_id_for_user from student.models import UserProfile from student.roles import CourseStaffRole, CourseInstructorRole from student.tests.factories import UserFactory, UserProfileFactory -from user_api.models import UserPreference +from openedx.core.djangoapps.user_api.models import UserPreference from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # Will also run default tests for IDTokens and UserInfo diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index 6fe8456bc4..2f223a934a 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -16,9 +16,8 @@ from django.test.utils import override_settings from util.testing import UrlResetMixin from third_party_auth.tests.testutil import simulate_running_pipeline -from user_api.api import account as account_api -from user_api.api import profile as profile_api -from util.bad_request_rate_limiter import BadRequestRateLimiter +from openedx.core.djangoapps.user_api.api import account as account_api +from openedx.core.djangoapps.user_api.api import profile as profile_api from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, mixed_store_config ) diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index 647427a27a..9251eee900 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -24,8 +24,8 @@ from student.views import ( register_user as old_register_view ) -from user_api.api import account as account_api -from user_api.api import profile as profile_api +from openedx.core.djangoapps.user_api.api import account as account_api +from openedx.core.djangoapps.user_api.api import profile as profile_api from util.bad_request_rate_limiter import BadRequestRateLimiter from student_account.helpers import auth_pipeline_urls diff --git a/lms/djangoapps/student_profile/test/test_views.py b/lms/djangoapps/student_profile/test/test_views.py index c24bd0ea0a..6785796ded 100644 --- a/lms/djangoapps/student_profile/test/test_views.py +++ b/lms/djangoapps/student_profile/test/test_views.py @@ -11,8 +11,8 @@ from django.conf import settings from django.core.urlresolvers import reverse from util.testing import UrlResetMixin -from user_api.api import account as account_api -from user_api.api import profile as profile_api +from openedx.core.djangoapps.user_api.api import account as account_api +from openedx.core.djangoapps.user_api.api import profile as profile_api from lang_pref import LANGUAGE_KEY, api as language_api diff --git a/lms/djangoapps/student_profile/views.py b/lms/djangoapps/student_profile/views.py index a7a6a3c0d0..51fac70c3e 100644 --- a/lms/djangoapps/student_profile/views.py +++ b/lms/djangoapps/student_profile/views.py @@ -11,7 +11,7 @@ from django.views.decorators.http import require_http_methods from django_future.csrf import ensure_csrf_cookie from django.contrib.auth.decorators import login_required from edxmako.shortcuts import render_to_response -from user_api.api import profile as profile_api +from openedx.core.djangoapps.user_api.api import profile as profile_api from lang_pref import LANGUAGE_KEY, api as language_api import third_party_auth diff --git a/lms/envs/common.py b/lms/envs/common.py index 6b260e44c0..e84d2d059b 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -931,7 +931,7 @@ MIDDLEWARE_CLASSES = ( # Adds user tags to tracking events # Must go before TrackMiddleware, to get the context set up - 'user_api.middleware.UserTagsEventContextMiddleware', + 'openedx.core.djangoapps.user_api.middleware.UserTagsEventContextMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', 'track.middleware.TrackMiddleware', @@ -1482,7 +1482,7 @@ INSTALLED_APPS = ( # User API 'rest_framework', - 'user_api', + 'openedx.core.djangoapps.user_api', # Shopping cart 'shoppingcart', diff --git a/lms/lib/xblock/runtime.py b/lms/lib/xblock/runtime.py index 2c1729ec49..f96a157acf 100644 --- a/lms/lib/xblock/runtime.py +++ b/lms/lib/xblock/runtime.py @@ -7,7 +7,7 @@ import xblock.reference.plugins from django.core.urlresolvers import reverse from django.conf import settings -from user_api.api import course_tag as user_course_tag_api +from openedx.core.djangoapps.user_api.api import course_tag as user_course_tag_api from xmodule.modulestore.django import modulestore from xmodule.x_module import ModuleSystem from xmodule.partitions.partitions_service import PartitionService @@ -128,13 +128,13 @@ class LmsPartitionService(PartitionService): course. (If and when XBlock directly provides access from one block (e.g. a split_test_module) - to another (e.g. a course_module), this won't be neccessary, but for now it seems like + to another (e.g. a course_module), this won't be necessary, but for now it seems like the least messy way to hook things through) """ @property def course_partitions(self): - course = modulestore().get_course(self._course_id) + course = modulestore().get_course(self.runtime.course_id) return course.user_partitions @@ -194,8 +194,7 @@ class LmsModuleSystem(LmsHandlerUrls, ModuleSystem): # pylint: disable=abstract services = kwargs.setdefault('services', {}) services['user_tags'] = UserTagsService(self) services['partitions'] = LmsPartitionService( - user_tags_service=services['user_tags'], - course_id=kwargs.get('course_id', None), + runtime=self, track_function=kwargs.get('track_function', None), ) services['fs'] = xblock.reference.plugins.FSService() diff --git a/lms/urls.py b/lms/urls.py index 4730514479..edb02028e1 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -57,7 +57,7 @@ urlpatterns = ('', # nopep8 url(r'^heartbeat$', include('heartbeat.urls')), - url(r'^user_api/', include('user_api.urls')), + url(r'^user_api/', include('openedx.core.djangoapps.user_api.urls')), url(r'^notifier_api/', include('notifier_api.urls')), diff --git a/common/djangoapps/user_api/api/__init__.py b/openedx/__init__.py similarity index 100% rename from common/djangoapps/user_api/api/__init__.py rename to openedx/__init__.py diff --git a/common/djangoapps/user_api/migrations/__init__.py b/openedx/core/__init__.py similarity index 100% rename from common/djangoapps/user_api/migrations/__init__.py rename to openedx/core/__init__.py diff --git a/common/djangoapps/user_api/tests/__init__.py b/openedx/core/djangoapps/__init__.py similarity index 100% rename from common/djangoapps/user_api/tests/__init__.py rename to openedx/core/djangoapps/__init__.py diff --git a/openedx/core/djangoapps/user_api/__init__.py b/openedx/core/djangoapps/user_api/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/user_api/api/__init__.py b/openedx/core/djangoapps/user_api/api/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/user_api/api/account.py b/openedx/core/djangoapps/user_api/api/account.py similarity index 99% rename from common/djangoapps/user_api/api/account.py rename to openedx/core/djangoapps/user_api/api/account.py index 118838c7b4..e79969f1b7 100644 --- a/common/djangoapps/user_api/api/account.py +++ b/openedx/core/djangoapps/user_api/api/account.py @@ -11,8 +11,8 @@ from django.db import transaction, IntegrityError from django.core.validators import validate_email, validate_slug, ValidationError from django.contrib.auth.forms import PasswordResetForm -from user_api.models import User, UserProfile, Registration, PendingEmailChange -from user_api.helpers import intercept_errors +from ..models import User, UserProfile, Registration, PendingEmailChange +from ..helpers import intercept_errors USERNAME_MIN_LENGTH = 2 diff --git a/common/djangoapps/user_api/api/course_tag.py b/openedx/core/djangoapps/user_api/api/course_tag.py similarity index 100% rename from common/djangoapps/user_api/api/course_tag.py rename to openedx/core/djangoapps/user_api/api/course_tag.py diff --git a/common/djangoapps/user_api/api/profile.py b/openedx/core/djangoapps/user_api/api/profile.py similarity index 97% rename from common/djangoapps/user_api/api/profile.py rename to openedx/core/djangoapps/user_api/api/profile.py index 8bc2f01f9c..f0033cc919 100644 --- a/common/djangoapps/user_api/api/profile.py +++ b/openedx/core/djangoapps/user_api/api/profile.py @@ -13,9 +13,9 @@ from django.db import IntegrityError from pytz import UTC import analytics -from user_api.models import User, UserProfile, UserPreference, UserOrgTag -from user_api.helpers import intercept_errors from eventtracking import tracker +from ..models import User, UserProfile, UserPreference, UserOrgTag +from ..helpers import intercept_errors log = logging.getLogger(__name__) @@ -34,6 +34,7 @@ class ProfileInvalidField(ProfileRequestError): """ The proposed value for a field is not in a valid format. """ def __init__(self, field, value): + super(ProfileInvalidField, self).__init__() self.field = field self.value = value diff --git a/common/djangoapps/user_api/helpers.py b/openedx/core/djangoapps/user_api/helpers.py similarity index 97% rename from common/djangoapps/user_api/helpers.py rename to openedx/core/djangoapps/user_api/helpers.py index 04bd7065e0..bb85be6fa9 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/openedx/core/djangoapps/user_api/helpers.py @@ -12,7 +12,7 @@ from django.http import HttpResponseBadRequest LOGGER = logging.getLogger(__name__) -def intercept_errors(api_error, ignore_errors=[]): +def intercept_errors(api_error, ignore_errors=None): """ Function decorator that intercepts exceptions and translates them into API-specific errors (usually an "internal" error). @@ -33,13 +33,20 @@ def intercept_errors(api_error, ignore_errors=[]): """ def _decorator(func): + """ + Function decorator that intercepts exceptions and translates them into API-specific errors. + """ @wraps(func) def _wrapped(*args, **kwargs): + """ + Wrapper that evaluates a function, intercepting exceptions and translating them into + API-specific errors. + """ try: return func(*args, **kwargs) except Exception as ex: # Raise the original exception if it's in our list of "ignored" errors - for ignored in ignore_errors: + for ignored in ignore_errors or []: if isinstance(ex, ignored): raise diff --git a/openedx/core/djangoapps/user_api/management/__init__.py b/openedx/core/djangoapps/user_api/management/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/user_api/management/commands/__init__.py b/openedx/core/djangoapps/user_api/management/commands/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/user_api/management/commands/email_opt_in_list.py b/openedx/core/djangoapps/user_api/management/commands/email_opt_in_list.py new file mode 100644 index 0000000000..b71f72ef91 --- /dev/null +++ b/openedx/core/djangoapps/user_api/management/commands/email_opt_in_list.py @@ -0,0 +1,267 @@ +"""Generate a list indicating whether users have opted in or out of receiving email from an org. + +Email opt-in is stored as an org-level preference. +When reports are generated, we need to handle: + +1) Org aliases: some organizations might have multiple course key "org" values. + We choose the most recently set preference among all org aliases. + Since this information isn't stored anywhere in edx-platform, + the caller needs to pass in the list of orgs and aliases. + +2) No preference set: Some users may not have an opt-in preference set + if they enrolled before the preference was introduced. + These users are opted in by default. + +3) Restricting to a subset of courses in an org: Some orgs have courses + that we don't want to include in the results (e.g. EdX-created test courses). + Allow the caller to explicitly specify the list of courses in the org. + +The command will always use the read replica database if one is configured. + +""" +import os.path +import csv +import time +import contextlib +import logging + +from django.core.management.base import BaseCommand, CommandError +from django.conf import settings +from django.db import connections + +from opaque_keys.edx.keys import CourseKey +from xmodule.modulestore.django import modulestore + + +LOGGER = logging.getLogger(__name__) + + +class Command(BaseCommand): + """Generate a list of email opt-in values for user enrollments. """ + + args = " --courses=COURSE_ID_LIST" + help = "Generate a list of email opt-in values for user enrollments." + + # Fields output in the CSV + OUTPUT_FIELD_NAMES = [ + "email", + "full_name", + "course_id", + "is_opted_in_for_email", + "preference_set_date" + ] + + # Number of records to read at a time when making + # multiple queries over a potentially large dataset. + QUERY_INTERVAL = 1000 + + def handle(self, *args, **options): + """Execute the command. + + Arguments: + file_path (str): Path to the output file. + *org_list (unicode): List of organization aliases. + + Keyword Arguments: + courses (unicode): Comma-separated list of course keys. If provided, + include only these courses in the results. + + Raises: + CommandError + + """ + file_path, org_list = self._parse_args(args) + + # Retrieve all the courses for the org. + # If we were given a specific list of courses to include, + # filter out anything not in that list. + courses = self._get_courses_for_org(org_list) + only_courses = options.get("courses") + if only_courses is not None: + only_courses = [ + CourseKey.from_string(course_key.strip()) + for course_key in only_courses.split(",") + ] + courses = list(set(courses) & set(only_courses)) + + # Add in organizations from the course keys, to ensure + # we're including orgs with different capitalizations + org_list = list(set(org_list) | set(course.org for course in courses)) + + # If no courses are found, abort + if not courses: + raise CommandError( + u"No courses found for orgs: {orgs}".format( + orgs=", ".join(org_list) + ) + ) + + # Let the user know what's about to happen + LOGGER.info( + u"Retrieving data for courses: {courses}".format( + courses=", ".join([unicode(course) for course in courses]) + ) + ) + + # Open the output file and generate the report. + with open(file_path, "w") as file_handle: + with self._log_execution_time(): + self._write_email_opt_in_prefs(file_handle, org_list, courses) + + # Remind the user where the output file is + LOGGER.info(u"Output file: {file_path}".format(file_path=file_path)) + + def _parse_args(self, args): + """Check and parse arguments. + + Validates that the right number of args were provided + and that the output file doesn't already exist. + + Arguments: + args (list): List of arguments given at the command line. + + Returns: + Tuple of (file_path, org_list) + + Raises: + CommandError + + """ + if len(args) < 2: + raise CommandError(u"Usage: {args}".format(args=self.args)) + + file_path = args[0] + org_list = args[1:] + if os.path.exists(file_path): + raise CommandError("File already exists at '{path}'".format(path=file_path)) + + return file_path, org_list + + def _get_courses_for_org(self, org_aliases): + """Retrieve all course keys for a particular org. + + Arguments: + org_aliases (list): List of aliases for the org. + + Returns: + List of `CourseKey`s + + """ + all_courses = modulestore().get_courses() + orgs_lowercase = [org.lower() for org in org_aliases] + return [ + course.id + for course in all_courses + if course.id.org.lower() in orgs_lowercase + ] + + @contextlib.contextmanager + def _log_execution_time(self): + """Context manager for measuring execution time. """ + start_time = time.time() + yield + execution_time = time.time() - start_time + LOGGER.info(u"Execution time: {time} seconds".format(time=execution_time)) + + def _write_email_opt_in_prefs(self, file_handle, org_aliases, courses): + """Write email opt-in preferences to the output file. + + This will generate a CSV with one row for each enrollment. + This means that the user's "opt in" preference will be specified + multiple times if the user has enrolled in multiple courses + within the org. However, the values should always be the same: + if the user is listed as "opted out" for course A, she will + also be listed as "opted out" for courses B, C, and D. + + Arguments: + file_handle (file): Handle to the output file. + org_aliases (list): List of aliases for the org. + courses (list): List of course keys in the org. + + Returns: + None + + """ + writer = csv.DictWriter(file_handle, fieldnames=self.OUTPUT_FIELD_NAMES) + cursor = self._db_cursor() + query = ( + u""" + SELECT + user.`email` AS `email`, + profile.`name` AS `full_name`, + enrollment.`course_id` AS `course_id`, + ( + SELECT value + FROM user_api_userorgtag + WHERE org IN ( {org_list} ) + AND `key`=\"email-optin\" + AND `user_id`=user.`id` + ORDER BY modified DESC + LIMIT 1 + ) AS `is_opted_in_for_email`, + ( + SELECT modified + FROM user_api_userorgtag + WHERE org IN ( {org_list} ) + AND `key`=\"email-optin\" + AND `user_id`=user.`id` + ORDER BY modified DESC + LIMIT 1 + ) AS `preference_set_date` + FROM + student_courseenrollment AS enrollment + LEFT JOIN auth_user AS user ON user.id=enrollment.user_id + LEFT JOIN auth_userprofile AS profile ON profile.user_id=user.id + WHERE enrollment.course_id IN ( {course_id_list} ) + """ + ).format( + course_id_list=self._sql_list(courses), + org_list=self._sql_list(org_aliases) + ) + + cursor.execute(query) + row_count = 0 + for row in self._iterate_results(cursor): + email, full_name, course_id, is_opted_in, pref_set_date = row + writer.writerow({ + "email": email.encode('utf-8'), + "full_name": full_name.encode('utf-8'), + "course_id": course_id.encode('utf-8'), + "is_opted_in_for_email": is_opted_in if is_opted_in else "True", + "preference_set_date": pref_set_date, + }) + row_count += 1 + + # Log the number of rows we processed + LOGGER.info(u"Retrieved {num_rows} records.".format(num_rows=row_count)) + + def _iterate_results(self, cursor): + """Iterate through the results of a database query, fetching in chunks. + + Arguments: + cursor: The database cursor + + Yields: + tuple of row values from the query + + """ + while True: + rows = cursor.fetchmany(self.QUERY_INTERVAL) + if not rows: + break + for row in rows: + yield row + + def _sql_list(self, values): + """Serialize a list of values for including in a SQL "IN" statement. """ + return u",".join([u'"{}"'.format(val) for val in values]) + + def _db_cursor(self): + """Return a database cursor to the read replica if one is available. """ + # Use the read replica if one has been configured + db_alias = ( + 'read_replica' + if 'read_replica' in settings.DATABASES + else 'default' + ) + return connections[db_alias].cursor() diff --git a/openedx/core/djangoapps/user_api/management/tests/__init__.py b/openedx/core/djangoapps/user_api/management/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py b/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py new file mode 100644 index 0000000000..58d60f0a0d --- /dev/null +++ b/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py @@ -0,0 +1,399 @@ +# -*- coding: utf-8 -*- +"""Tests for the email opt-in list management command. """ +import os.path +import tempfile +import shutil +import csv +from collections import defaultdict +from unittest import skipUnless + + +import ddt +from django.conf import settings +from django.test.utils import override_settings +from django.core.management.base import CommandError + + +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, mixed_store_config +from xmodule.modulestore.tests.factories import CourseFactory +from student.tests.factories import UserFactory, CourseEnrollmentFactory +from student.models import CourseEnrollment + +import openedx.core.djangoapps.user_api.api.profile as profile_api +from openedx.core.djangoapps.user_api.models import UserOrgTag +from openedx.core.djangoapps.user_api.management.commands import email_opt_in_list + + +MODULESTORE_CONFIG = mixed_store_config(settings.COMMON_TEST_DATA_ROOT, {}, include_xml=False) + + +@ddt.ddt +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +@override_settings(MODULESTORE=MODULESTORE_CONFIG) +class EmailOptInListTest(ModuleStoreTestCase): + """Tests for the email opt-in list management command. """ + + USER_USERNAME = "test_user" + USER_FIRST_NAME = u"Ṫëṡẗ" + USER_LAST_NAME = u"Űśéŕ" + + TEST_ORG = u"téśt_őŕǵ" + + OUTPUT_FILE_NAME = "test_org_email_opt_in.csv" + OUTPUT_FIELD_NAMES = [ + "email", + "full_name", + "course_id", + "is_opted_in_for_email", + "preference_set_date" + ] + + def setUp(self): + self.user = UserFactory.create( + username=self.USER_USERNAME, + first_name=self.USER_FIRST_NAME, + last_name=self.USER_LAST_NAME + ) + self.courses = [] + self.enrollments = defaultdict(list) + + def test_not_enrolled(self): + self._create_courses_and_enrollments((self.TEST_ORG, False)) + output = self._run_command(self.TEST_ORG) + + # The user isn't enrolled in the course, so the output should be empty + self._assert_output(output) + + def test_enrolled_no_pref(self): + self._create_courses_and_enrollments((self.TEST_ORG, True)) + output = self._run_command(self.TEST_ORG) + + # By default, if no preference is set by the user is enrolled, opt in + self._assert_output(output, (self.user, self.courses[0].id, True)) + + def test_enrolled_pref_opted_in(self): + self._create_courses_and_enrollments((self.TEST_ORG, True)) + self._set_opt_in_pref(self.user, self.TEST_ORG, True) + output = self._run_command(self.TEST_ORG) + self._assert_output(output, (self.user, self.courses[0].id, True)) + + def test_enrolled_pref_opted_out(self): + self._create_courses_and_enrollments((self.TEST_ORG, True)) + self._set_opt_in_pref(self.user, self.TEST_ORG, False) + output = self._run_command(self.TEST_ORG) + self._assert_output(output, (self.user, self.courses[0].id, False)) + + def test_opt_in_then_opt_out(self): + self._create_courses_and_enrollments((self.TEST_ORG, True)) + self._set_opt_in_pref(self.user, self.TEST_ORG, True) + self._set_opt_in_pref(self.user, self.TEST_ORG, False) + output = self._run_command(self.TEST_ORG) + self._assert_output(output, (self.user, self.courses[0].id, False)) + + def test_exclude_non_org_courses(self): + # Enroll in a course that's not in the org + self._create_courses_and_enrollments( + (self.TEST_ORG, True), + ("other_org", True) + ) + + # Opt out of the other course + self._set_opt_in_pref(self.user, "other_org", False) + + # The first course is included in the results, + # but the second course is excluded, + # so the user should be opted in by default. + output = self._run_command(self.TEST_ORG) + self._assert_output( + output, + (self.user, self.courses[0].id, True), + expect_pref_datetime=False + ) + + def test_enrolled_conflicting_prefs(self): + # Enroll in two courses, both in the org + self._create_courses_and_enrollments( + (self.TEST_ORG, True), + ("org_alias", True) + ) + + # Opt into the first course, then opt out of the second course + self._set_opt_in_pref(self.user, self.TEST_ORG, True) + self._set_opt_in_pref(self.user, "org_alias", False) + + # The second preference change should take precedence + # Note that *both* courses are included in the list, + # but they should have the same value. + output = self._run_command(self.TEST_ORG, other_names=["org_alias"]) + self._assert_output( + output, + (self.user, self.courses[0].id, False), + (self.user, self.courses[1].id, False) + ) + + # Opt into the first course + # Even though the other course still has a preference set to false, + # the newest preference takes precedence + self._set_opt_in_pref(self.user, self.TEST_ORG, True) + output = self._run_command(self.TEST_ORG, other_names=["org_alias"]) + self._assert_output( + output, + (self.user, self.courses[0].id, True), + (self.user, self.courses[1].id, True) + ) + + @ddt.data(True, False) + def test_unenrolled_from_all_courses(self, opt_in_pref): + # Enroll in the course and set a preference + self._create_courses_and_enrollments((self.TEST_ORG, True)) + self._set_opt_in_pref(self.user, self.TEST_ORG, opt_in_pref) + + # Unenroll from the course + CourseEnrollment.unenroll(self.user, self.courses[0].id, skip_refund=True) + + # Enrollments should still appear in the outpu + output = self._run_command(self.TEST_ORG) + self._assert_output(output, (self.user, self.courses[0].id, opt_in_pref)) + + def test_unenrolled_from_some_courses(self): + # Enroll in several courses in the org + self._create_courses_and_enrollments( + (self.TEST_ORG, True), + (self.TEST_ORG, True), + (self.TEST_ORG, True), + ("org_alias", True) + ) + + # Set a preference for the aliased course + self._set_opt_in_pref(self.user, "org_alias", False) + + # Unenroll from the aliased course + CourseEnrollment.unenroll(self.user, self.courses[3].id, skip_refund=True) + + # Expect that the preference still applies, + # and all the enrollments should appear in the list + output = self._run_command(self.TEST_ORG, other_names=["org_alias"]) + self._assert_output( + output, + (self.user, self.courses[0].id, False), + (self.user, self.courses[1].id, False), + (self.user, self.courses[2].id, False), + (self.user, self.courses[3].id, False) + ) + + def test_no_courses_for_org_name(self): + self._create_courses_and_enrollments((self.TEST_ORG, True)) + self._set_opt_in_pref(self.user, self.TEST_ORG, True) + + # No course available for this particular org + with self.assertRaisesRegexp(CommandError, "^No courses found for orgs:"): + self._run_command("other_org") + + def test_specify_subset_of_courses(self): + # Create several courses in the same org + self._create_courses_and_enrollments( + (self.TEST_ORG, True), + (self.TEST_ORG, True), + (self.TEST_ORG, True), + ) + + # Execute the command, but exclude the second course from the list + only_courses = [self.courses[0].id, self.courses[1].id] + self._run_command(self.TEST_ORG, only_courses=only_courses) + + # Choose numbers before and after the query interval boundary + @ddt.data(2, 3, 4, 5, 6, 7, 8, 9) + def test_many_users(self, num_users): + # Create many users and enroll them in the test course + course = CourseFactory.create(org=self.TEST_ORG) + usernames = [] + for _ in range(num_users): + user = UserFactory.create() + usernames.append(user.username) + CourseEnrollmentFactory.create(course_id=course.id, user=user) + + # Generate the report + output = self._run_command(self.TEST_ORG, query_interval=4) + + # Expect that every enrollment shows up in the report + output_emails = [row["email"] for row in output] + for email in output_emails: + self.assertIn(email, output_emails) + + def test_org_capitalization(self): + # Lowercase some of the org names in the course IDs + self._create_courses_and_enrollments( + ("MyOrg", True), + ("myorg", True) + ) + + # Set preferences for both courses + self._set_opt_in_pref(self.user, "MyOrg", True) + self._set_opt_in_pref(self.user, "myorg", False) + + # Execute the command, expecting both enrollments to show up + # We're passing in the uppercase org, but we set the lowercase + # version more recently, so we expect the lowercase org + # preference to apply. + output = self._run_command("MyOrg") + self._assert_output( + output, + (self.user, self.courses[0].id, False), + (self.user, self.courses[1].id, False) + ) + + @ddt.data(0, 1) + def test_not_enough_args(self, num_args): + args = ["dummy"] * num_args + expected_msg_regex = "^Usage: --courses=COURSE_ID_LIST$" + with self.assertRaisesRegexp(CommandError, expected_msg_regex): + email_opt_in_list.Command().handle(*args) + + def test_file_already_exists(self): + temp_file = tempfile.NamedTemporaryFile(delete=True) + + def _cleanup(): # pylint: disable=missing-docstring + temp_file.close() + + with self.assertRaisesRegexp(CommandError, "^File already exists"): + email_opt_in_list.Command().handle(temp_file.name, self.TEST_ORG) + + def _create_courses_and_enrollments(self, *args): + """Create courses and enrollments. + + Created courses and enrollments are stored in instance variables + so tests can refer to them later. + + Arguments: + *args: Tuples of (course_org, should_enroll), where + course_org is the name of the org in the course key + and should_enroll is a boolean indicating whether to enroll + the user in the course. + + Returns: + None + + """ + for course_number, (course_org, should_enroll) in enumerate(args): + course = CourseFactory.create(org=course_org, number=str(course_number)) + if should_enroll: + enrollment = CourseEnrollmentFactory.create( + is_active=True, + course_id=course.id, + user=self.user + ) + self.enrollments[course.id].append(enrollment) + self.courses.append(course) + + def _set_opt_in_pref(self, user, org, is_opted_in): + """Set the email opt-in preference. + + Arguments: + user (User): The user model. + org (unicode): The org in the course key. + is_opted_in (bool): Whether the user is opted in or out of emails. + + Returns: + None + + """ + profile_api.update_email_opt_in(user.username, org, is_opted_in) + + def _latest_pref_set_date(self, user): + """Retrieve the latest opt-in preference for the user, + across all orgs and preference keys. + + Arguments: + user (User): The user whos preference was set. + + Returns: + ISO-formatted date string or empty string + + """ + pref = UserOrgTag.objects.filter(user=user).order_by("-modified") + return pref[0].modified.isoformat(' ') if len(pref) > 0 else "" + + def _run_command(self, org, other_names=None, only_courses=None, query_interval=None): + """Execute the management command to generate the email opt-in list. + + Arguments: + org (unicode): The org to generate the report for. + + Keyword Arguments: + other_names (list): List of other aliases for the org. + only_courses (list): If provided, include only these course IDs in the report. + query_interval (int): If provided, override the default query interval. + + Returns: + list: The rows of the generated CSV report. Each item is a dictionary. + + """ + # Create a temporary directory for the output + # Delete it when we're finished + temp_dir_path = tempfile.mkdtemp() + + def _cleanup(): # pylint: disable=missing-docstring + shutil.rmtree(temp_dir_path) + + self.addCleanup(_cleanup) + + # Sanitize the arguments + if other_names is None: + other_names = [] + + output_path = os.path.join(temp_dir_path, self.OUTPUT_FILE_NAME) + org_list = [org] + other_names + if only_courses is not None: + only_courses = ",".join(unicode(course_id) for course_id in only_courses) + + command = email_opt_in_list.Command() + + # Override the query interval to speed up the tests + if query_interval is not None: + command.QUERY_INTERVAL = query_interval + + # Execute the command + command.handle(output_path, *org_list, courses=only_courses) + + # Retrieve the output from the file + try: + with open(output_path) as output_file: + reader = csv.DictReader(output_file, fieldnames=self.OUTPUT_FIELD_NAMES) + rows = [row for row in reader] + except IOError: + self.fail("Could not find or open output file at '{path}'".format(path=output_path)) + + # Return the output as a list of dictionaries + return rows + + def _assert_output(self, output, *args, **kwargs): + """Check the output of the report. + + Arguments: + output (list): List of rows in the output CSV file. + *args: Tuples of (user, course_id, opt_in_pref) + + Keyword Arguments: + expect_pref_datetime (bool): If false, expect an empty + string for the preference. + + Returns: + None + + Raises: + AssertionError + + """ + self.assertEqual(len(output), len(args)) + for user, course_id, opt_in_pref in args: + self.assertIn({ + "email": user.email.encode('utf-8'), + "full_name": user.profile.name.encode('utf-8'), + "course_id": unicode(course_id).encode('utf-8'), + "is_opted_in_for_email": unicode(opt_in_pref), + "preference_set_date": ( + self._latest_pref_set_date(self.user) + if kwargs.get("expect_pref_datetime", True) + else "" + ) + }, output) diff --git a/common/djangoapps/user_api/middleware.py b/openedx/core/djangoapps/user_api/middleware.py similarity index 97% rename from common/djangoapps/user_api/middleware.py rename to openedx/core/djangoapps/user_api/middleware.py index 1f4d4aa1cc..144ab8220e 100644 --- a/common/djangoapps/user_api/middleware.py +++ b/openedx/core/djangoapps/user_api/middleware.py @@ -8,7 +8,8 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from track.contexts import COURSE_REGEX -from user_api.models import UserCourseTag + +from .models import UserCourseTag class UserTagsEventContextMiddleware(object): diff --git a/common/djangoapps/user_api/migrations/0001_initial.py b/openedx/core/djangoapps/user_api/migrations/0001_initial.py similarity index 100% rename from common/djangoapps/user_api/migrations/0001_initial.py rename to openedx/core/djangoapps/user_api/migrations/0001_initial.py diff --git a/common/djangoapps/user_api/migrations/0002_auto__add_usercoursetags__add_unique_usercoursetags_user_course_id_key.py b/openedx/core/djangoapps/user_api/migrations/0002_auto__add_usercoursetags__add_unique_usercoursetags_user_course_id_key.py similarity index 100% rename from common/djangoapps/user_api/migrations/0002_auto__add_usercoursetags__add_unique_usercoursetags_user_course_id_key.py rename to openedx/core/djangoapps/user_api/migrations/0002_auto__add_usercoursetags__add_unique_usercoursetags_user_course_id_key.py diff --git a/common/djangoapps/user_api/migrations/0003_rename_usercoursetags.py b/openedx/core/djangoapps/user_api/migrations/0003_rename_usercoursetags.py similarity index 100% rename from common/djangoapps/user_api/migrations/0003_rename_usercoursetags.py rename to openedx/core/djangoapps/user_api/migrations/0003_rename_usercoursetags.py diff --git a/openedx/core/djangoapps/user_api/migrations/__init__.py b/openedx/core/djangoapps/user_api/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/user_api/models.py b/openedx/core/djangoapps/user_api/models.py similarity index 100% rename from common/djangoapps/user_api/models.py rename to openedx/core/djangoapps/user_api/models.py diff --git a/openedx/core/djangoapps/user_api/partition_schemes.py b/openedx/core/djangoapps/user_api/partition_schemes.py new file mode 100644 index 0000000000..b3b40212db --- /dev/null +++ b/openedx/core/djangoapps/user_api/partition_schemes.py @@ -0,0 +1,61 @@ +""" +Provides partition support to the user service. +""" + +import random +import api.course_tag as course_tag_api + +from xmodule.partitions.partitions import UserPartitionError + + +class RandomUserPartitionScheme(object): + """ + This scheme randomly assigns users into the partition's groups. + """ + RANDOM = random.Random() + + @classmethod + def get_group_for_user(cls, course_id, user, user_partition, track_function=None): + """ + Returns the group from the specified user position to which the user is assigned. + If the user has not yet been assigned, a group will be randomly chosen for them. + """ + partition_key = cls._key_for_partition(user_partition) + group_id = course_tag_api.get_course_tag(user, course_id, partition_key) + group = user_partition.get_group(int(group_id)) if not group_id is None else None + if group is None: + if not user_partition.groups: + raise UserPartitionError('Cannot assign user to an empty user partition') + + # pylint: disable=fixme + # TODO: had a discussion in arch council about making randomization more + # deterministic (e.g. some hash). Could do that, but need to be careful not + # to introduce correlation between users or bias in generation. + group = cls.RANDOM.choice(user_partition.groups) + + # persist the value as a course tag + course_tag_api.set_course_tag(user, course_id, partition_key, group.id) + + if track_function: + # emit event for analytics + # FYI - context is always user ID that is logged in, NOT the user id that is + # being operated on. If instructor can move user explicitly, then we should + # put in event_info the user id that is being operated on. + event_info = { + 'group_id': group.id, + 'group_name': group.name, + 'partition_id': user_partition.id, + 'partition_name': user_partition.name + } + # pylint: disable=fixme + # TODO: Use the XBlock publish api instead + track_function('xmodule.partitions.assigned_user_to_partition', event_info) + + return group + + @classmethod + def _key_for_partition(cls, user_partition): + """ + Returns the key to use to look up and save the user's group for a given user partition. + """ + return 'xblock.partition_service.partition_{0}'.format(user_partition.id) diff --git a/common/djangoapps/user_api/serializers.py b/openedx/core/djangoapps/user_api/serializers.py similarity index 95% rename from common/djangoapps/user_api/serializers.py rename to openedx/core/djangoapps/user_api/serializers.py index 09aa25e939..7c7227fff6 100644 --- a/common/djangoapps/user_api/serializers.py +++ b/openedx/core/djangoapps/user_api/serializers.py @@ -1,7 +1,8 @@ from django.contrib.auth.models import User from rest_framework import serializers from student.models import UserProfile -from user_api.models import UserPreference + +from .models import UserPreference class UserSerializer(serializers.HyperlinkedModelSerializer): diff --git a/openedx/core/djangoapps/user_api/tests/__init__.py b/openedx/core/djangoapps/user_api/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/user_api/tests/factories.py b/openedx/core/djangoapps/user_api/tests/factories.py similarity index 92% rename from common/djangoapps/user_api/tests/factories.py rename to openedx/core/djangoapps/user_api/tests/factories.py index 60ee7c2ddc..8b1198bef5 100644 --- a/common/djangoapps/user_api/tests/factories.py +++ b/openedx/core/djangoapps/user_api/tests/factories.py @@ -2,9 +2,10 @@ from factory.django import DjangoModelFactory from factory import SubFactory from student.tests.factories import UserFactory -from user_api.models import UserPreference, UserCourseTag, UserOrgTag from opaque_keys.edx.locations import SlashSeparatedCourseKey +from ..models import UserPreference, UserCourseTag, UserOrgTag + # Factories are self documenting # pylint: disable=missing-docstring diff --git a/common/djangoapps/user_api/tests/test_account_api.py b/openedx/core/djangoapps/user_api/tests/test_account_api.py similarity index 99% rename from common/djangoapps/user_api/tests/test_account_api.py rename to openedx/core/djangoapps/user_api/tests/test_account_api.py index 3ab1aac029..27b708abce 100644 --- a/common/djangoapps/user_api/tests/test_account_api.py +++ b/openedx/core/djangoapps/user_api/tests/test_account_api.py @@ -12,8 +12,8 @@ from django.core import mail from django.test import TestCase from django.conf import settings -from user_api.api import account as account_api -from user_api.models import UserProfile +from ..api import account as account_api +from ..models import UserProfile @ddt.ddt diff --git a/common/djangoapps/user_api/tests/test_constants.py b/openedx/core/djangoapps/user_api/tests/test_constants.py similarity index 100% rename from common/djangoapps/user_api/tests/test_constants.py rename to openedx/core/djangoapps/user_api/tests/test_constants.py diff --git a/common/djangoapps/user_api/tests/test_course_tag_api.py b/openedx/core/djangoapps/user_api/tests/test_course_tag_api.py similarity index 91% rename from common/djangoapps/user_api/tests/test_course_tag_api.py rename to openedx/core/djangoapps/user_api/tests/test_course_tag_api.py index 51b48a56a4..a329df2a3a 100644 --- a/common/djangoapps/user_api/tests/test_course_tag_api.py +++ b/openedx/core/djangoapps/user_api/tests/test_course_tag_api.py @@ -4,11 +4,11 @@ Test the user course tag API. from django.test import TestCase from student.tests.factories import UserFactory -from user_api.api import course_tag as course_tag_api +from openedx.core.djangoapps.user_api.api import course_tag as course_tag_api from opaque_keys.edx.locations import SlashSeparatedCourseKey -class TestUserService(TestCase): +class TestCourseTagAPI(TestCase): """ Test the user service """ diff --git a/common/djangoapps/user_api/tests/test_helpers.py b/openedx/core/djangoapps/user_api/tests/test_helpers.py similarity index 96% rename from common/djangoapps/user_api/tests/test_helpers.py rename to openedx/core/djangoapps/user_api/tests/test_helpers.py index 76fcce7ddf..713ff41206 100644 --- a/common/djangoapps/user_api/tests/test_helpers.py +++ b/openedx/core/djangoapps/user_api/tests/test_helpers.py @@ -4,10 +4,10 @@ Tests for helper functions. import json import mock import ddt +from django.http import HttpRequest, HttpResponse from django.test import TestCase from nose.tools import raises -from django.http import HttpRequest, HttpResponse -from user_api.helpers import ( +from ..helpers import ( intercept_errors, shim_student_view, FormDescription, InvalidFieldError ) @@ -49,12 +49,12 @@ class InterceptErrorsTest(TestCase): def test_ignores_expected_errors(self): intercepted_function(raise_error=ValueError) - @mock.patch('user_api.helpers.LOGGER') + @mock.patch('openedx.core.djangoapps.user_api.helpers.LOGGER') def test_logs_errors(self, mock_logger): expected_log_msg = ( u"An unexpected error occurred when calling 'intercepted_function' " u"with arguments '()' and " - u"keyword arguments '{'raise_error': }': " + u"keyword arguments '{'raise_error': }': " u"FakeInputException()" ) diff --git a/common/djangoapps/user_api/tests/test_middleware.py b/openedx/core/djangoapps/user_api/tests/test_middleware.py similarity index 95% rename from common/djangoapps/user_api/tests/test_middleware.py rename to openedx/core/djangoapps/user_api/tests/test_middleware.py index d51ed2057e..297ad0a140 100644 --- a/common/djangoapps/user_api/tests/test_middleware.py +++ b/openedx/core/djangoapps/user_api/tests/test_middleware.py @@ -6,8 +6,9 @@ from django.http import HttpResponse from django.test.client import RequestFactory from student.tests.factories import UserFactory, AnonymousUserFactory -from user_api.tests.factories import UserCourseTagFactory -from user_api.middleware import UserTagsEventContextMiddleware + +from ..tests.factories import UserCourseTagFactory +from ..middleware import UserTagsEventContextMiddleware class TagsMiddlewareTest(TestCase): @@ -29,7 +30,7 @@ class TagsMiddlewareTest(TestCase): self.response = Mock(spec=HttpResponse) - patcher = patch('user_api.middleware.tracker') + patcher = patch('openedx.core.djangoapps.user_api.middleware.tracker') self.tracker = patcher.start() self.addCleanup(patcher.stop) diff --git a/common/djangoapps/user_api/tests/test_models.py b/openedx/core/djangoapps/user_api/tests/test_models.py similarity index 95% rename from common/djangoapps/user_api/tests/test_models.py rename to openedx/core/djangoapps/user_api/tests/test_models.py index fabcbec8d2..09606403ab 100644 --- a/common/djangoapps/user_api/tests/test_models.py +++ b/openedx/core/djangoapps/user_api/tests/test_models.py @@ -2,8 +2,9 @@ from django.db import IntegrityError from django.test import TestCase from xmodule.modulestore.tests.factories import CourseFactory from student.tests.factories import UserFactory -from user_api.tests.factories import UserPreferenceFactory, UserCourseTagFactory, UserOrgTagFactory -from user_api.models import UserPreference + +from ..tests.factories import UserPreferenceFactory, UserCourseTagFactory, UserOrgTagFactory +from ..models import UserPreference class UserPreferenceModelTest(TestCase): diff --git a/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py b/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py new file mode 100644 index 0000000000..f1b9ec9dc0 --- /dev/null +++ b/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py @@ -0,0 +1,107 @@ +""" +Test the user api's partition extensions. +""" +from collections import defaultdict +from mock import patch + +from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme, UserPartitionError +from student.tests.factories import UserFactory +from xmodule.partitions.partitions import Group, UserPartition +from xmodule.partitions.tests.test_partitions import PartitionTestCase + + +class MemoryCourseTagAPI(object): + """ + An implementation of a user service that uses an in-memory dictionary for storage + """ + def __init__(self): + self._tags = defaultdict(dict) + + def get_course_tag(self, __, course_id, key): + """Sets the value of ``key`` to ``value``""" + return self._tags[course_id].get(key) + + def set_course_tag(self, __, course_id, key, value): + """Gets the value of ``key``""" + self._tags[course_id][key] = value + + +class TestRandomUserPartitionScheme(PartitionTestCase): + """ + Test getting a user's group out of a partition + """ + + MOCK_COURSE_ID = "mock-course-id" + + def setUp(self): + super(TestRandomUserPartitionScheme, self).setUp() + # Patch in a memory-based user service instead of using the persistent version + course_tag_api = MemoryCourseTagAPI() + self.user_service_patcher = patch( + 'openedx.core.djangoapps.user_api.partition_schemes.course_tag_api', course_tag_api + ) + self.user_service_patcher.start() + + # Create a test user + self.user = UserFactory.create() + + def tearDown(self): + self.user_service_patcher.stop() + + def test_get_group_for_user(self): + # get a group assigned to the user + group1_id = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, self.user_partition) + + # make sure we get the same group back out every time + for __ in range(0, 10): + group2_id = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, self.user_partition) + self.assertEqual(group1_id, group2_id) + + def test_empty_partition(self): + empty_partition = UserPartition( + self.TEST_ID, + 'Test Partition', + 'for testing purposes', + [], + scheme=RandomUserPartitionScheme + ) + # get a group assigned to the user + with self.assertRaisesRegexp(UserPartitionError, "Cannot assign user to an empty user partition"): + RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, empty_partition) + + def test_user_in_deleted_group(self): + # get a group assigned to the user - should be group 0 or 1 + old_group = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, self.user_partition) + self.assertIn(old_group.id, [0, 1]) + + # Change the group definitions! No more group 0 or 1 + groups = [Group(3, 'Group 3'), Group(4, 'Group 4')] + user_partition = UserPartition(self.TEST_ID, 'Test Partition', 'for testing purposes', groups) + + # Now, get a new group using the same call - should be 3 or 4 + new_group = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, user_partition) + self.assertIn(new_group.id, [3, 4]) + + # We should get the same group over multiple calls + new_group_2 = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, user_partition) + self.assertEqual(new_group, new_group_2) + + def test_change_group_name(self): + # Changing the name of the group shouldn't affect anything + # get a group assigned to the user - should be group 0 or 1 + old_group = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, self.user_partition) + self.assertIn(old_group.id, [0, 1]) + + # Change the group names + groups = [Group(0, 'Group 0'), Group(1, 'Group 1')] + user_partition = UserPartition( + self.TEST_ID, + 'Test Partition', + 'for testing purposes', + groups, + scheme=RandomUserPartitionScheme + ) + + # Now, get a new group using the same call + new_group = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, user_partition) + self.assertEqual(old_group.id, new_group.id) diff --git a/common/djangoapps/user_api/tests/test_profile_api.py b/openedx/core/djangoapps/user_api/tests/test_profile_api.py similarity index 98% rename from common/djangoapps/user_api/tests/test_profile_api.py rename to openedx/core/djangoapps/user_api/tests/test_profile_api.py index 9e0f7e554e..297b50c4f8 100644 --- a/common/djangoapps/user_api/tests/test_profile_api.py +++ b/openedx/core/djangoapps/user_api/tests/test_profile_api.py @@ -10,9 +10,9 @@ from dateutil.parser import parse as parse_datetime from xmodule.modulestore.tests.factories import CourseFactory import datetime -from user_api.api import account as account_api -from user_api.api import profile as profile_api -from user_api.models import UserProfile, UserOrgTag +from ..api import account as account_api +from ..api import profile as profile_api +from ..models import UserProfile, UserOrgTag @ddt.ddt diff --git a/common/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py similarity index 99% rename from common/djangoapps/user_api/tests/test_views.py rename to openedx/core/djangoapps/user_api/tests/test_views.py index 69380eb220..6a3f8259a4 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -16,16 +16,16 @@ from pytz import UTC import mock from xmodule.modulestore.tests.factories import CourseFactory -from user_api.api import account as account_api, profile as profile_api - from student.tests.factories import UserFactory -from user_api.models import UserOrgTag -from user_api.tests.factories import UserPreferenceFactory +from unittest import SkipTest from django_comment_common import models from opaque_keys.edx.locations import SlashSeparatedCourseKey from third_party_auth.tests.testutil import simulate_running_pipeline -from user_api.tests.test_constants import SORTED_COUNTRIES +from ..api import account as account_api, profile as profile_api +from ..models import UserOrgTag +from ..tests.factories import UserPreferenceFactory +from ..tests.test_constants import SORTED_COUNTRIES TEST_API_KEY = "test_api_key" diff --git a/common/djangoapps/user_api/urls.py b/openedx/core/djangoapps/user_api/urls.py similarity index 76% rename from common/djangoapps/user_api/urls.py rename to openedx/core/djangoapps/user_api/urls.py index b6b903f350..3b840cc6b5 100644 --- a/common/djangoapps/user_api/urls.py +++ b/openedx/core/djangoapps/user_api/urls.py @@ -1,17 +1,21 @@ -# pylint: disable=missing-docstring +""" +Defines the URL routes for this app. +""" + from django.conf import settings from django.conf.urls import include, patterns, url from rest_framework import routers -from user_api import views as user_api_views -from user_api.models import UserPreference + +from . import views as user_api_views +from .models import UserPreference -user_api_router = routers.DefaultRouter() -user_api_router.register(r'users', user_api_views.UserViewSet) -user_api_router.register(r'user_prefs', user_api_views.UserPreferenceViewSet) +USER_API_ROUTER = routers.DefaultRouter() +USER_API_ROUTER.register(r'users', user_api_views.UserViewSet) +USER_API_ROUTER.register(r'user_prefs', user_api_views.UserPreferenceViewSet) urlpatterns = patterns( '', - url(r'^v1/', include(user_api_router.urls)), + url(r'^v1/', include(USER_API_ROUTER.urls)), url( r'^v1/preferences/(?P{})/users/$'.format(UserPreference.KEY_REGEX), user_api_views.PreferenceUsersListView.as_view() diff --git a/common/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py similarity index 99% rename from common/djangoapps/user_api/views.py rename to openedx/core/djangoapps/user_api/views.py index 41e69aa3f5..3ee7415290 100644 --- a/common/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -1,5 +1,6 @@ """HTTP end-points for the User API. """ import copy +import third_party_auth from django.conf import settings from django.contrib.auth.models import User @@ -19,16 +20,15 @@ from rest_framework import viewsets from rest_framework.views import APIView from rest_framework.exceptions import ParseError from django_countries import countries -from user_api.serializers import UserSerializer, UserPreferenceSerializer -from user_api.models import UserPreference, UserProfile from django_comment_common.models import Role from opaque_keys.edx.locations import SlashSeparatedCourseKey from edxmako.shortcuts import marketing_link -import third_party_auth from util.authentication import SessionAuthenticationAllowInactiveUser -from user_api.api import account as account_api, profile as profile_api -from user_api.helpers import FormDescription, shim_student_view, require_post_params +from .api import account as account_api, profile as profile_api +from .helpers import FormDescription, shim_student_view, require_post_params +from .models import UserPreference +from .serializers import UserSerializer, UserPreferenceSerializer class ApiKeyHeaderPermission(permissions.BasePermission): diff --git a/pavelib/tests.py b/pavelib/tests.py index e8da51c6fc..9b1058b221 100644 --- a/pavelib/tests.py +++ b/pavelib/tests.py @@ -48,7 +48,7 @@ def test_system(options): if test_id: if not system: system = test_id.split('/')[0] - if system == 'common': + if system in ['common', 'openedx']: system = 'lms' opts['test_id'] = test_id diff --git a/pavelib/utils/test/suites/nose_suite.py b/pavelib/utils/test/suites/nose_suite.py index 4e0e0992d8..42787bc216 100644 --- a/pavelib/utils/test/suites/nose_suite.py +++ b/pavelib/utils/test/suites/nose_suite.py @@ -128,7 +128,7 @@ class SystemTestSuite(NoseTestSuite): # django-nose will import them early in the test process, # thereby making sure that we load any django models that are # only defined in test files. - default_test_id = "{system}/djangoapps/* common/djangoapps/*".format( + default_test_id = "{system}/djangoapps/* common/djangoapps/* openedx/core/djangoapps/*".format( system=self.root ) diff --git a/setup.py b/setup.py index cdd8f74211..90ee4ef273 100644 --- a/setup.py +++ b/setup.py @@ -1,14 +1,24 @@ -from setuptools import setup, find_packages +""" +Setup script for the Open edX package. +""" + +from setuptools import setup setup( name="Open edX", - version="0.1", + version="0.2", install_requires=['distribute'], requires=[], # NOTE: These are not the names we should be installing. This tree should - # be reorgnized to be a more conventional Python tree. + # be reorganized to be a more conventional Python tree. packages=[ + "openedx.core.djangoapps.user_api", "lms", "cms", ], + entry_points={ + 'openedx.user_partition_scheme': [ + 'random = openedx.core.djangoapps.user_api.partition_schemes:RandomUserPartitionScheme', + ], + } )