From 8516c0dbffff6a5e656d051da644ea46edb408cb Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Sat, 12 Jul 2014 13:43:12 -0400 Subject: [PATCH 1/8] add ability for microsites to define additional profile fields to be captured in an overriden signup form - register.html- and then store additional profile in the profile's 'meta' field as JSON blob fix broken unit test add some unit tests --- .../student/tests/test_microsite.py | 55 ++++++++++++++++++- common/djangoapps/student/views.py | 37 +++++++++++-- 2 files changed, 86 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/student/tests/test_microsite.py b/common/djangoapps/student/tests/test_microsite.py index b447ad3a13..d76aa9a72a 100644 --- a/common/djangoapps/student/tests/test_microsite.py +++ b/common/djangoapps/student/tests/test_microsite.py @@ -4,8 +4,24 @@ Test for User Creation from Micro-Sites from django.test import TestCase from student.models import UserSignupSource import mock +import json from django.core.urlresolvers import reverse +from django.contrib.auth.models import User +FAKE_MICROSITE = { + "SITE_NAME": "openedx.localhost", + "REGISTRATION_EXTRA_FIELDS": { + "address1": "required", + "city": "required", + "state": "required", + "country": "required", + "company": "required", + "title": "required" + }, + "extended_profile_fields": [ + "address1", "state", "company", "title" + ] +} def fake_site_name(name, default=None): # pylint: disable=W0613 """ @@ -14,8 +30,13 @@ def fake_site_name(name, default=None): # pylint: disable=W0613 if name == 'SITE_NAME': return 'openedx.localhost' else: - return None + return default +def fake_microsite_get_value(name, default=None): # pylint: disable=W0613 + """ + create a fake microsite site name + """ + return FAKE_MICROSITE.get(name, default) class TestMicrosite(TestCase): """Test for Account Creation from a white labeled Micro-Sites""" @@ -30,6 +51,14 @@ class TestMicrosite(TestCase): "honor_code": "true", "terms_of_service": "true", } + self.extended_params = dict(self.params.items() + { + "address1": "foo", + "city": "foo", + "state": "foo", + "country": "foo", + "company": "foo", + "title": "foo" + }.items()) @mock.patch("microsite_configuration.microsite.get_value", fake_site_name) def test_user_signup_source(self): @@ -49,3 +78,27 @@ class TestMicrosite(TestCase): response = self.client.post(self.url, self.params) self.assertEqual(response.status_code, 200) self.assertEqual(len(UserSignupSource.objects.filter(site='openedx.localhost')), 0) + + @mock.patch("microsite_configuration.microsite.get_value", fake_microsite_get_value) + def test_user_signup_missing_enhanced_profile(self): + """ + test to create a user form the microsite but don't provide any of the microsite specific + profile information + """ + response = self.client.post(self.url, self.params) + self.assertEqual(response.status_code, 400) + + @mock.patch("microsite_configuration.microsite.get_value", fake_microsite_get_value) + def test_user_signup_including_enhanced_profile(self): + """ + test to create a user form the microsite but don't provide any of the microsite specific + profile information + """ + response = self.client.post(self.url, self.extended_params) + self.assertEqual(response.status_code, 200) + user = User.objects.get(username=self.username) + meta = json.loads(user.profile.meta) + self.assertEqual(meta['address1'], 'foo') + self.assertEqual(meta['state'], 'foo') + self.assertEqual(meta['company'], 'foo') + self.assertEqual(meta['title'], 'foo') diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 4ad74eb228..b26213624f 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -6,6 +6,7 @@ import logging import re import uuid import time +import json from collections import defaultdict from pytz import UTC @@ -1039,7 +1040,7 @@ def user_signup_handler(sender, **kwargs): # pylint: disable=W0613 log.info(u'user {} originated from a white labeled "Microsite"'.format(kwargs['instance'].id)) -def _do_create_account(post_vars): +def _do_create_account(post_vars, extended_profile=None): """ Given cleaned post variables, create the User and UserProfile objects, as well as the registration for this user. @@ -1089,6 +1090,10 @@ def _do_create_account(post_vars): profile.country = post_vars.get('country') profile.goals = post_vars.get('goals') + # add any extended profile information in the denormalized 'meta' field in the profile + if extended_profile: + profile.meta = json.dumps(extended_profile) + try: profile.year_of_birth = int(post_vars['year_of_birth']) except (ValueError, KeyError): @@ -1115,7 +1120,12 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many js = {'success': False} # pylint: disable-msg=invalid-name post_vars = post_override if post_override else request.POST - extra_fields = getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {}) + + # allow for microsites to define their own set of required/optional/hidden fields + extra_fields = microsite.get_value( + 'REGISTRATION_EXTRA_FIELDS', + getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {}) + ) if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline.running(request): post_vars = dict(post_vars.items()) @@ -1188,7 +1198,7 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many else: min_length = 2 - if len(post_vars[field_name]) < min_length: + if field_name not in post_vars or len(post_vars[field_name]) < min_length: error_str = { 'username': _('Username must be minimum of two characters long'), 'email': _('A properly formatted e-mail is required'), @@ -1204,7 +1214,12 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many 'city': _('A city is required'), 'country': _('A country is required') } - js['value'] = error_str[field_name] + + if field_name in error_str: + js['value'] = error_str[field_name] + else: + js['value'] = _('You are missing one or more required fields') + js['field'] = field_name return JsonResponse(js, status=400) @@ -1248,10 +1263,22 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many js['field'] = 'password' return JsonResponse(js, status=400) + # allow microsites to define 'extended profile fields' which are + # captured on user signup (for example via an overriden registration.html) + # and then stored in the UserProfile + extended_profile_fields = microsite.get_value('extended_profile_fields', []) + extended_profile = None + + for field in extended_profile_fields: + if field in post_vars: + if not extended_profile: + extended_profile = {} + extended_profile[field] = post_vars[field] + # Ok, looks like everything is legit. Create the account. try: with transaction.commit_on_success(): - ret = _do_create_account(post_vars) + ret = _do_create_account(post_vars, extended_profile) except AccountValidationError as e: return JsonResponse({'success': False, 'value': e.message, 'field': e.field}, status=400) From 930ba01bc2d05a89bc71e512184e294f9d206367 Mon Sep 17 00:00:00 2001 From: Stephen Sanchez Date: Mon, 14 Jul 2014 09:22:10 -0400 Subject: [PATCH 2/8] Updating release tag for July 14th, 2014 --- requirements/edx/base.txt | 2 +- requirements/edx/github.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index c50c5d74b6..109ad3fd17 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -134,4 +134,4 @@ git+https://github.com/mfogel/django-settings-context-processor.git git+https://github.com/mitocw/django-cas.git # edX packages -edx-submissions==0.0.2 +edx-submissions==0.0.3 diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 4d80d5f9d8..1e4c84918b 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -27,7 +27,7 @@ -e git+https://github.com/edx/bok-choy.git@82b4e82d79b9d4c6d087ebbfa26ea23235728e62#egg=bok_choy -e git+https://github.com/edx-solutions/django-splash.git@9965a53c269666a30bb4e2b3f6037c138aef2a55#egg=django-splash -e git+https://github.com/edx/acid-block.git@459aff7b63db8f2c5decd1755706c1a64fb4ebb1#egg=acid-xblock --e git+https://github.com/edx/edx-ora2.git@release-2014-07-07T19.28#egg=edx-ora2 +-e git+https://github.com/edx/edx-ora2.git@release-2014-07-14T13.17#egg=edx-ora2 -e git+https://github.com/edx/opaque-keys.git@3f6ea9984b8a084d1a1f5a3bd50f3cdb5ff44440#egg=opaque-keys -e git+https://github.com/edx/ease.git@97de68448e5495385ba043d3091f570a699d5b5f#egg=ease -e git+https://github.com/edx/i18n-tools.git@f5303e82dff368c7595884d9325aeea1d802da25#egg=i18n-tools From df8f3bd39eec04d8b404630f96639a0fd0087624 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Tue, 15 Jul 2014 10:20:09 -0400 Subject: [PATCH 3/8] Revert "Pull out edX into variables in password reset email" This reverts commit 3a2276b5374fb63b4675ebf627f79b4eaf28c686. --- lms/templates/registration/password_reset_email.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/templates/registration/password_reset_email.html b/lms/templates/registration/password_reset_email.html index 568a140686..e7bbc6cf35 100644 --- a/lms/templates/registration/password_reset_email.html +++ b/lms/templates/registration/password_reset_email.html @@ -1,5 +1,5 @@ {% load i18n %}{% load url from future %}{% autoescape off %} -{% blocktrans with site_name=site_name %}You're receiving this e-mail because you requested a password reset for your user account at {{ site_name }}.{% endblocktrans %} +{% blocktrans %}You're receiving this e-mail because you requested a password reset for your user account at edx.org.{% endblocktrans %} {% trans "Please go to the following page and choose a new password:" %} {% block reset_link %} @@ -10,6 +10,6 @@ http{% if is_secure %}s{% endif %}://{{domain}}{% url 'student.views.password_re {% trans "Thanks for using our site!" %} -{% blocktrans with platform_name=platform_name %}The {{ platform_name }} Team{% endblocktrans %} +{% blocktrans %}The edX Team{% endblocktrans %} {% endautoescape %} From 713c2a4474e6e6bb3fb3c00926356dff2747420e Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 15 Jul 2014 20:10:17 -0400 Subject: [PATCH 4/8] Handle bad children pointers during publish rather than aborting the publish, ignore them. Needed for import --- common/lib/xmodule/xmodule/modulestore/mongo/draft.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 35e9926885..888a54bd16 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -627,7 +627,12 @@ class DraftModuleStore(MongoModuleStore): """ Depth first publishing from the given location """ - item = self.get_item(item_location) + try: + # handle child does not exist w/o killing publish + item = self.get_item(item_location) + except ItemNotFoundError: + log.warning('Cannot find: %s', item_location) + return # publish the children first if item.has_children: From 4928fad4151b943da3bc63ae62ca10c79b5aa8f2 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 17 Jul 2014 00:43:00 -0400 Subject: [PATCH 5/8] add ability to inspect a cart for the existence of a particular type of item, e.g. PaidCourseRegistration. Only show the 'Shopping Cart' button if there's a PaidCourseRegistration in it. --- .../shoppingcart/context_processor.py | 5 ++++- lms/djangoapps/shoppingcart/models.py | 18 ++++++++++++++---- .../shoppingcart/tests/test_models.py | 15 +++++++++++++++ 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/shoppingcart/context_processor.py b/lms/djangoapps/shoppingcart/context_processor.py index 95beb106d3..540ab6a2f3 100644 --- a/lms/djangoapps/shoppingcart/context_processor.py +++ b/lms/djangoapps/shoppingcart/context_processor.py @@ -19,5 +19,8 @@ def user_has_cart_context_processor(request): request.user.is_authenticated() and # user is logged in and settings.FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION') and # settings enable paid course reg and settings.FEATURES.get('ENABLE_SHOPPING_CART') and # settings enable shopping cart and - shoppingcart.models.Order.user_cart_has_items(request.user) # user's cart has items + shoppingcart.models.Order.user_cart_has_items( + request.user, + shoppingcart.models.PaidCourseRegistration + ) # user's cart has PaidCourseRegistrations )} diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 406257fd01..6839300575 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -87,15 +87,17 @@ class Order(models.Model): return cart_order @classmethod - def user_cart_has_items(cls, user): + def user_cart_has_items(cls, user, item_type=None): """ Returns true if the user (anonymous user ok) has a cart with items in it. (Which means it should be displayed. + If a item_type is passed in, then we check to see if the cart has at least one of + those types of OrderItems """ if not user.is_authenticated(): return False cart = cls.get_cart_for_user(user) - return cart.has_items() + return cart.has_items(item_type) @property def total_cost(self): @@ -105,11 +107,19 @@ class Order(models.Model): """ return sum(i.line_cost for i in self.orderitem_set.filter(status=self.status)) # pylint: disable=E1101 - def has_items(self): + def has_items(self, item_type=None): """ Does the cart have any items in it? + If an item_type is passed in then we check to see if there are any items of that class type """ - return self.orderitem_set.exists() # pylint: disable=E1101 + if not item_type: + return self.orderitem_set.exists() # pylint: disable=E1101 + else: + items = self.orderitem_set.all().select_subclasses() + for item in items: + if isinstance(item, item_type): + return True + return False def clear(self): """ diff --git a/lms/djangoapps/shoppingcart/tests/test_models.py b/lms/djangoapps/shoppingcart/tests/test_models.py index 5f4a911071..cdaeda1826 100644 --- a/lms/djangoapps/shoppingcart/tests/test_models.py +++ b/lms/djangoapps/shoppingcart/tests/test_models.py @@ -52,6 +52,21 @@ class OrderTest(ModuleStoreTestCase): item = OrderItem(order=cart, user=self.user) item.save() self.assertTrue(Order.user_cart_has_items(self.user)) + self.assertFalse(Order.user_cart_has_items(self.user, CertificateItem)) + self.assertFalse(Order.user_cart_has_items(self.user, PaidCourseRegistration)) + + def test_user_cart_has_paid_course_registration_items(self): + cart = Order.get_cart_for_user(self.user) + item = PaidCourseRegistration(order=cart, user=self.user) + item.save() + self.assertTrue(Order.user_cart_has_items(self.user, PaidCourseRegistration)) + self.assertFalse(Order.user_cart_has_items(self.user, CertificateItem)) + + def test_user_cart_has_certificate_items(self): + cart = Order.get_cart_for_user(self.user) + CertificateItem.add_to_order(cart, self.course_key, self.cost, 'honor') + self.assertTrue(Order.user_cart_has_items(self.user, CertificateItem)) + self.assertFalse(Order.user_cart_has_items(self.user, PaidCourseRegistration)) def test_cart_clear(self): cart = Order.get_cart_for_user(user=self.user) From 0fdc762452d1f9f4f4d4744a9b4ef40463a87154 Mon Sep 17 00:00:00 2001 From: swdanielli Date: Thu, 3 Jul 2014 12:17:29 -0400 Subject: [PATCH 6/8] recommender prototype --- cms/djangoapps/contentstore/views/component.py | 1 + requirements/edx/edx-private.txt | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 2948c78c6f..8953f10908 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -67,6 +67,7 @@ else: 'done', # Lets students mark things as done. See https://github.com/pmitros/DoneXBlock 'audio', # Embed an audio file. See https://github.com/pmitros/AudioXBlock SPLIT_TEST_COMPONENT_TYPE, # Adds A/B test support + 'recommender' # Crowdsourced recommender. Prototype by dli&pmitros. Intended for roll-out in one place in one course. ] + OPEN_ENDED_COMPONENT_TYPES + NOTE_COMPONENT_TYPES ADVANCED_COMPONENT_CATEGORY = 'advanced' diff --git a/requirements/edx/edx-private.txt b/requirements/edx/edx-private.txt index 990dfb6242..fe1b905d06 100644 --- a/requirements/edx/edx-private.txt +++ b/requirements/edx/edx-private.txt @@ -10,3 +10,9 @@ -e git+https://github.com/pmitros/DoneXBlock.git@1ce0ac14d9f3df3083b951262ec82e84b58d16d1#egg=done-xblock -e git+https://github.com/pmitros/AudioXBlock.git@1fbf19cc21613aead62799469e1593adb037fdd9#egg=audio-xblock -e git+https://github.com/pmitros/AnimationXBlock.git@d2b551bb8f49a138088e10298576102164145b87#egg=animation-xblock + +# This XBlock shows a list of recommendations. +# It is an R&D prototype, intended for roll-out one location in one course. +# It should not be used without learning sciences support in the current state. +-e git+https://github.com/pmitros/RecommenderXBlock.git@fae9e5bc8a8297cb15001f0d674430e3d22ffa35#egg=recommender-xblock + From 682c9fffd1cea0203140ba3816057a241e029c88 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Thu, 17 Jul 2014 21:11:20 -0400 Subject: [PATCH 7/8] fix bulk_write context manager. --- common/lib/xmodule/xmodule/modulestore/mixed.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 7dfe0f345c..3ee379febd 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -497,8 +497,8 @@ def store_bulk_write_operations_on_course(store, course_id): # request comes in for the same course. # if the caller passed in the mixed modulestore, get a direct pointer to the underlying store - if hasattr(store, '_get_modulestore_by_course_id'): - store = store._get_modulestore_by_course_id(course_id) + if hasattr(store, '_get_modulestore_for_courseid'): + store = store._get_modulestore_for_courseid(course_id) try: if hasattr(store, 'begin_bulk_write_operation_on_course'): From a501aa9400d235edf2a774835aef9fa92fb8c508 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Thu, 17 Jul 2014 17:24:52 -0400 Subject: [PATCH 8/8] don't update ancestors while bulk editing. Conflicts: common/lib/xmodule/xmodule/modulestore/mongo/base.py --- .../lib/xmodule/xmodule/modulestore/mongo/base.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 7ae03cc332..267ce2362d 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -405,6 +405,13 @@ class MongoModuleStore(ModuleStoreWriteBase): self.ignore_write_events_on_courses.remove(course_id) self.refresh_cached_metadata_inheritance_tree(course_id) + def _is_bulk_write_in_progress(self, course_id): + """ + Returns whether a bulk write operation is in progress for the given course. + """ + # check with branch set to None + return course_id in self.ignore_write_events_on_courses + def _fill_in_run(self, course_key): if course_key.run is not None: return course_key @@ -552,7 +559,7 @@ class MongoModuleStore(ModuleStoreWriteBase): If given a runtime, it replaces the cached_metadata in that runtime. NOTE: failure to provide a runtime may mean that some objects report old values for inherited data. """ - if course_id not in self.ignore_write_events_on_courses: + if not self._is_bulk_write_in_progress(course_id): # below is done for side effects when runtime is None cached_metadata = self._get_cached_metadata_inheritance_tree(course_id, force_refresh=True) if runtime: @@ -1033,7 +1040,10 @@ class MongoModuleStore(ModuleStoreWriteBase): # update subtree edited info for ancestors # don't update the subtree info for descendants of the publish root for efficiency - if (not isPublish) or (isPublish and is_publish_root): + if ( + (not isPublish or (isPublish and is_publish_root)) and + not self._is_bulk_write_in_progress(xblock.location.course_key) + ): ancestor_payload = { 'edit_info.subtree_edited_on': now, 'edit_info.subtree_edited_by': user_id