diff --git a/common/djangoapps/util/enterprise_helpers.py b/common/djangoapps/util/enterprise_helpers.py index 7edcb7ff2a..be3ff19751 100644 --- a/common/djangoapps/util/enterprise_helpers.py +++ b/common/djangoapps/util/enterprise_helpers.py @@ -3,9 +3,11 @@ Helpers to access the enterprise app """ import logging +from functools import wraps from django.conf import settings from django.contrib.auth.models import User from django.core.urlresolvers import reverse +from django.shortcuts import redirect from django.utils.http import urlencode from edx_rest_api_client.client import EdxRestApiClient try: @@ -70,6 +72,36 @@ class EnterpriseApiClient(object): raise EnterpriseApiException(message) +def data_sharing_consent_required(view_func): + """ + Decorator which makes a view method redirect to the Data Sharing Consent form if: + + * The wrapped method is passed request, course_id as the first two arguments. + * Enterprise integration is enabled + * Data sharing consent is required before accessing this course view. + * The request.user has not yet given data sharing consent for this course. + + After granting consent, the user will be redirected back to the original request.path. + + """ + @wraps(view_func) + def inner(request, course_id, *args, **kwargs): + """ + Redirect to the consent page if the request.user must consent to data sharing before viewing course_id. + + Otherwise, just call the wrapped view function. + """ + # Redirect to the consent URL, if consent is required. + consent_url = get_enterprise_consent_url(request, course_id) + if consent_url: + return redirect(consent_url) + + # Otherwise, drop through to wrapped view + return view_func(request, course_id, *args, **kwargs) + + return inner + + def enterprise_enabled(): """ Determines whether the Enterprise app is installed @@ -87,14 +119,32 @@ def consent_needed_for_course(user, course_id): return consent_necessary_for_course(user, course_id) -def get_course_specific_consent_url(request, course_id, return_to): +def get_enterprise_consent_url(request, course_id, user=None, return_to=None): """ Build a URL to redirect the user to the Enterprise app to provide data sharing consent for a specific course ID. + + Arguments: + * request: Request object + * course_id: Course key/identifier string. + * user: user to check for consent. If None, uses request.user + * return_to: url name label for the page to return to after consent is granted. + If None, return to request.path instead. """ + if user is None: + user = request.user + + if not consent_needed_for_course(user, course_id): + return None + + if return_to is None: + return_path = request.path + else: + return_path = reverse(return_to, args=(course_id,)) + url_params = { 'course_id': course_id, - 'next': request.build_absolute_uri(reverse(return_to, args=(course_id,))) + 'next': request.build_absolute_uri(return_path) } querystring = urlencode(url_params) full_url = reverse('grant_data_sharing_permissions') + '?' + querystring diff --git a/common/djangoapps/util/tests/mixins/enterprise.py b/common/djangoapps/util/tests/mixins/enterprise.py index de8342e210..48285fb39d 100644 --- a/common/djangoapps/util/tests/mixins/enterprise.py +++ b/common/djangoapps/util/tests/mixins/enterprise.py @@ -2,6 +2,7 @@ Mixins for the EnterpriseApiClient. """ import json +import mock import httpretty from django.conf import settings @@ -55,3 +56,37 @@ class EnterpriseServiceMockMixin(object): content_type='application/json', status=500 ) + + +class EnterpriseTestConsentRequired(object): + """ + Mixin to help test the data_sharing_consent_required decorator. + """ + def verify_consent_required(self, client, url, status_code=200): + """ + Verify that the given URL redirects to the consent page when consent is required, + and doesn't redirect to the consent page when consent is not required. + + Arguments: + * self: ignored + * client: the TestClient instance to be used + * url: URL to test + * status_code: expected status code of URL when no data sharing consent is required. + """ + with mock.patch('util.enterprise_helpers.enterprise_enabled', return_value=True): + with mock.patch('util.enterprise_helpers.consent_necessary_for_course') as mock_consent_necessary: + # Ensure that when consent is necessary, the user is redirected to the consent page. + mock_consent_necessary.return_value = True + response = client.get(url) + assert response.status_code == 302 + assert 'grant_data_sharing_permissions' in response.url # pylint: disable=no-member + + # Ensure that when consent is not necessary, the user continues through to the requested page. + mock_consent_necessary.return_value = False + response = client.get(url) + assert response.status_code == status_code + + # If we were expecting a redirect, ensure it's not to the data sharing permission page + if status_code == 302: + assert 'grant_data_sharing_permissions' not in response.url # pylint: disable=no-member + return response diff --git a/common/djangoapps/util/tests/test_enterprise_helpers.py b/common/djangoapps/util/tests/test_enterprise_helpers.py index d5a596bdd2..ed76b80817 100644 --- a/common/djangoapps/util/tests/test_enterprise_helpers.py +++ b/common/djangoapps/util/tests/test_enterprise_helpers.py @@ -4,11 +4,14 @@ Test the enterprise app helpers import unittest from django.conf import settings +from django.http import HttpResponseRedirect +from django.test.utils import override_settings import mock from util.enterprise_helpers import ( enterprise_enabled, insert_enterprise_pipeline_elements, + data_sharing_consent_required, set_enterprise_branding_filter_param, get_enterprise_branding_filter_param, get_enterprise_customer_logo_url @@ -21,21 +24,28 @@ class TestEnterpriseHelpers(unittest.TestCase): Test enterprise app helpers """ - @mock.patch('util.enterprise_helpers.enterprise_enabled') - def test_utils_with_enterprise_disabled(self, mock_enterprise_enabled): + @override_settings(ENABLE_ENTERPRISE_INTEGRATION=False) + def test_utils_with_enterprise_disabled(self): """ - Test that the enterprise app not being available causes + Test that disabling the enterprise integration flag causes the utilities to return the expected default values. """ - mock_enterprise_enabled.return_value = False + self.assertFalse(enterprise_enabled()) self.assertEqual(insert_enterprise_pipeline_elements(None), None) - def test_enterprise_enabled(self): + @override_settings(ENABLE_ENTERPRISE_INTEGRATION=True) + def test_utils_with_enterprise_enabled(self): """ - The test settings inherit from common, which have the enterprise - app installed; therefore, it should appear installed here. + Test that enabling enterprise integration (which is currently on by default) causes the + the utilities to return the expected values. """ self.assertTrue(enterprise_enabled()) + pipeline = ['abc', 'social.pipeline.social_auth.load_extra_data', 'def'] + insert_enterprise_pipeline_elements(pipeline) + self.assertEqual(pipeline, ['abc', + 'enterprise.tpa_pipeline.handle_enterprise_logistration', + 'social.pipeline.social_auth.load_extra_data', + 'def']) def test_set_enterprise_branding_filter_param(self): """ @@ -51,7 +61,7 @@ class TestEnterpriseHelpers(unittest.TestCase): set_enterprise_branding_filter_param(request, provider_id=provider_id) self.assertEqual(get_enterprise_branding_filter_param(request), {'provider_id': provider_id}) - @mock.patch('util.enterprise_helpers.enterprise_enabled', mock.Mock(return_value=True)) + @override_settings(ENABLE_ENTERPRISE_INTEGRATION=True) def test_get_enterprise_customer_logo_url(self): """ Test test_get_enterprise_customer_logo_url return the logo url as desired. @@ -75,7 +85,7 @@ class TestEnterpriseHelpers(unittest.TestCase): logo_url = get_enterprise_customer_logo_url(request) self.assertEqual(logo_url, '/test/image.png') - @mock.patch('util.enterprise_helpers.enterprise_enabled', mock.Mock(return_value=False)) + @override_settings(ENABLE_ENTERPRISE_INTEGRATION=False) def test_get_enterprise_customer_logo_url_return_none(self): """ Test get_enterprise_customer_logo_url return 'None' when enterprise application is not installed. @@ -88,7 +98,7 @@ class TestEnterpriseHelpers(unittest.TestCase): logo_url = get_enterprise_customer_logo_url(request) self.assertEqual(logo_url, None) - @mock.patch('util.enterprise_helpers.enterprise_enabled', mock.Mock(return_value=True)) + @override_settings(ENABLE_ENTERPRISE_INTEGRATION=True) @mock.patch('util.enterprise_helpers.get_enterprise_branding_filter_param', mock.Mock(return_value=None)) def test_get_enterprise_customer_logo_url_return_none_when_param_missing(self): """ @@ -101,3 +111,84 @@ class TestEnterpriseHelpers(unittest.TestCase): with mock.patch('enterprise.utils.get_enterprise_branding_info_by_provider_id', return_value=branding_info): logo_url = get_enterprise_customer_logo_url(request) self.assertEqual(logo_url, None) + + def check_data_sharing_consent(self, consent_required=False, consent_url=None): + """ + Used to test the data_sharing_consent_required view decorator. + """ + + # Test by wrapping a function that has the expected signature + @data_sharing_consent_required + def view_func(request, course_id, *args, **kwargs): + """ + Return the function arguments, so they can be tested. + """ + return ((request, course_id,) + args, kwargs) + + # Call the wrapped function + args = (mock.MagicMock(), 'course-id', 'another arg', 'and another') + kwargs = dict(a=1, b=2, c=3) + response = view_func(*args, **kwargs) + + # If consent required, then the response should be a redirect to the consent URL, and the view function would + # not be called. + if consent_required: + self.assertIsInstance(response, HttpResponseRedirect) + self.assertEquals(response.url, consent_url) # pylint: disable=no-member + + # Otherwise, the view function should have been called with the expected arguments. + else: + self.assertEqual(response, (args, kwargs)) + + @mock.patch('util.enterprise_helpers.enterprise_enabled') + @mock.patch('util.enterprise_helpers.consent_necessary_for_course') + def test_data_consent_required_enterprise_disabled(self, + mock_consent_necessary, + mock_enterprise_enabled): + """ + Verify that the wrapped view is called directly when enterprise integration is disabled, + without checking for course consent necessary. + """ + mock_enterprise_enabled.return_value = False + + self.check_data_sharing_consent(consent_required=False) + + mock_enterprise_enabled.assert_called_once() + mock_consent_necessary.assert_not_called() + + @mock.patch('util.enterprise_helpers.enterprise_enabled') + @mock.patch('util.enterprise_helpers.consent_necessary_for_course') + def test_no_course_data_consent_required(self, + mock_consent_necessary, + mock_enterprise_enabled): + + """ + Verify that the wrapped view is called directly when enterprise integration is enabled, + and no course consent is required. + """ + mock_enterprise_enabled.return_value = True + mock_consent_necessary.return_value = False + + self.check_data_sharing_consent(consent_required=False) + + mock_enterprise_enabled.assert_called_once() + mock_consent_necessary.assert_called_once() + + @mock.patch('util.enterprise_helpers.enterprise_enabled') + @mock.patch('util.enterprise_helpers.consent_necessary_for_course') + @mock.patch('util.enterprise_helpers.get_enterprise_consent_url') + def test_data_consent_required(self, mock_get_consent_url, mock_consent_necessary, mock_enterprise_enabled): + """ + Verify that the wrapped function returns a redirect to the consent URL when enterprise integration is enabled, + and course consent is required. + """ + mock_enterprise_enabled.return_value = True + mock_consent_necessary.return_value = True + consent_url = '/abc/def' + mock_get_consent_url.return_value = consent_url + + self.check_data_sharing_consent(consent_required=True, consent_url=consent_url) + + mock_get_consent_url.assert_called_once() + mock_enterprise_enabled.assert_called_once() + mock_consent_necessary.assert_called_once() diff --git a/lms/djangoapps/course_wiki/middleware.py b/lms/djangoapps/course_wiki/middleware.py index 76de84234c..a7e3827f9b 100644 --- a/lms/djangoapps/course_wiki/middleware.py +++ b/lms/djangoapps/course_wiki/middleware.py @@ -10,6 +10,7 @@ from courseware.courses import get_course_with_access, get_course_overview_with_ from courseware.access import has_access from student.models import CourseEnrollment from util.request import course_id_from_url +from util.enterprise_helpers import get_enterprise_consent_url class WikiAccessMiddleware(object): @@ -75,6 +76,12 @@ class WikiAccessMiddleware(object): # if a user is logged in, but not authorized to see a page, # we'll redirect them to the course about page return redirect('about_course', course_id.to_deprecated_string()) + + # If we need enterprise data sharing consent for this course, then redirect to the form. + consent_url = get_enterprise_consent_url(request, course_id) + if consent_url: + return redirect(consent_url) + # set the course onto here so that the wiki template can show the course navigation request.course = course else: diff --git a/lms/djangoapps/course_wiki/tests/tests.py b/lms/djangoapps/course_wiki/tests/tests.py index 511e3c24d3..85305ecb04 100644 --- a/lms/djangoapps/course_wiki/tests/tests.py +++ b/lms/djangoapps/course_wiki/tests/tests.py @@ -2,6 +2,7 @@ from django.core.urlresolvers import reverse from nose.plugins.attrib import attr from courseware.tests.tests import LoginEnrollmentTestCase +from util.tests.mixins.enterprise import EnterpriseTestConsentRequired from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -9,7 +10,7 @@ from mock import patch @attr(shard=1) -class WikiRedirectTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): +class WikiRedirectTestCase(EnterpriseTestConsentRequired, LoginEnrollmentTestCase, ModuleStoreTestCase): """ Tests for wiki course redirection. """ @@ -196,3 +197,25 @@ class WikiRedirectTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): resp = self.client.get(course_wiki_page, follow=True, HTTP_REFERER=referer) self.assertEqual(resp.status_code, 200) + + @patch.dict("django.conf.settings.FEATURES", {'ALLOW_WIKI_ROOT_ACCESS': True}) + def test_consent_required(self): + """ + Test that enterprise data sharing consent is required when enabled for the various courseware views. + """ + # Public wikis can be accessed by non-enrolled users, and so direct access is not gated by the consent page + course = CourseFactory.create() + course.allow_public_wiki_access = False + course.save() + + # However, for private wikis, enrolled users must pass through the consent gate + # (Unenrolled users are redirected to course/about) + course_id = unicode(course.id) + self.login(self.student, self.password) + self.enroll(course) + + for (url, status_code) in ( + (reverse('course_wiki', kwargs={'course_id': course_id}), 302), + ('/courses/{}/wiki/'.format(course_id), 200), + ): + self.verify_consent_required(self.client, url, status_code) diff --git a/lms/djangoapps/course_wiki/views.py b/lms/djangoapps/course_wiki/views.py index 319c7e812a..f14eaa4daf 100644 --- a/lms/djangoapps/course_wiki/views.py +++ b/lms/djangoapps/course_wiki/views.py @@ -16,6 +16,7 @@ from courseware.courses import get_course_by_id from course_wiki.utils import course_wiki_slug from opaque_keys.edx.locations import SlashSeparatedCourseKey from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from util.enterprise_helpers import data_sharing_consent_required log = logging.getLogger(__name__) @@ -29,7 +30,8 @@ def root_create(request): # pylint: disable=unused-argument return redirect('wiki:get', path=root.path) -def course_wiki_redirect(request, course_id): # pylint: disable=unused-argument +@data_sharing_consent_required +def course_wiki_redirect(request, course_id, wiki_path=""): # pylint: disable=unused-argument """ This redirects to whatever page on the wiki that the course designates as it's home page. A course's wiki must be an article on the root (for @@ -50,7 +52,7 @@ def course_wiki_redirect(request, course_id): # pylint: disable=unused-argument return redirect("wiki:get", path="") try: - urlpath = URLPath.get_by_path(course_slug, select_related=True) + urlpath = URLPath.get_by_path(wiki_path or course_slug, select_related=True) results = list(Article.objects.filter(id=urlpath.article.id)) if results: diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index 510b269c8f..b7dbdd7541 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -60,9 +60,8 @@ class CourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTestCase): resp = self.client.get(url) self.assertNotIn("You are not currently enrolled in this course", resp.content) - @mock.patch('courseware.views.views.get_course_specific_consent_url') - @mock.patch('courseware.views.views.consent_needed_for_course') - def test_redirection_missing_enterprise_consent(self, mock_consent_needed, mock_get_url): + @mock.patch('courseware.views.views.get_enterprise_consent_url') + def test_redirection_missing_enterprise_consent(self, mock_get_url): """ Verify that users viewing the course info who are enrolled, but have not provided data sharing consent, are first redirected to a consent page, and then, once they've @@ -70,7 +69,6 @@ class CourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTestCase): """ self.setup_user() self.enroll(self.course) - mock_consent_needed.return_value = True mock_get_url.return_value = reverse('dashboard') url = reverse('info', args=[self.course.id.to_deprecated_string()]) @@ -80,8 +78,8 @@ class CourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTestCase): response, reverse('dashboard') ) - mock_consent_needed.assert_called_once_with(self.user, unicode(self.course.id)) - mock_consent_needed.return_value = False + mock_get_url.assert_called_once() + mock_get_url.return_value = None response = self.client.get(url) self.assertNotIn("You are not currently enrolled in this course", response.content) diff --git a/lms/djangoapps/courseware/tests/test_view_authentication.py b/lms/djangoapps/courseware/tests/test_view_authentication.py index 24fe5fd527..38d69e85a0 100644 --- a/lms/djangoapps/courseware/tests/test_view_authentication.py +++ b/lms/djangoapps/courseware/tests/test_view_authentication.py @@ -197,28 +197,28 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): ) ) - @patch('courseware.views.index.get_course_specific_consent_url') - @patch('courseware.views.index.consent_needed_for_course') - def test_redirection_missing_enterprise_consent(self, mock_consent_needed, mock_get_url): + @patch('courseware.views.index.get_enterprise_consent_url') + def test_redirection_missing_enterprise_consent(self, mock_get_url): """ Verify that enrolled students are redirected to the Enterprise consent URL if a linked Enterprise Customer requires data sharing consent and it has not yet been provided. """ - mock_consent_needed.return_value = True mock_get_url.return_value = reverse('dashboard') self.login(self.enrolled_user) - response = self.client.get( - reverse( - 'courseware', - kwargs={'course_id': self.course.id.to_deprecated_string()} - ) + url = reverse( + 'courseware', + kwargs={'course_id': self.course.id.to_deprecated_string()} ) + response = self.client.get(url) self.assertRedirects( response, reverse('dashboard') ) - mock_consent_needed.assert_called_once_with(self.enrolled_user, unicode(self.course.id)) + mock_get_url.assert_called_once() + mock_get_url.return_value = None + response = self.client.get(url) + self.assertNotIn("You are not currently enrolled in this course", response.content) def test_instructor_page_access_nonstaff(self): """ diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 30ffb45327..5bfd709857 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -54,6 +54,7 @@ from openedx.core.lib.gating import api as gating_api from openedx.core.djangoapps.crawlers.models import CrawlersConfig from student.models import CourseEnrollment from student.tests.factories import AdminFactory, UserFactory, CourseEnrollmentFactory +from util.tests.mixins.enterprise import EnterpriseTestConsentRequired from util.tests.test_date_utils import fake_ugettext, fake_pgettext from util.url import reload_django_url_config from util.views import ensure_valid_course_key @@ -2201,3 +2202,28 @@ class TestIndexViewCrawlerStudentStateWrites(SharedModuleStoreTestCase): # Make sure we get back an actual 200, and aren't redirected because we # messed up the setup somehow (e.g. didn't enroll properly) self.assertEqual(response.status_code, 200) + + +@attr(shard=1) +class EnterpriseConsentTestCase(EnterpriseTestConsentRequired, ModuleStoreTestCase): + """ + Ensure that the Enterprise Data Consent redirects are in place only when consent is required. + """ + def setUp(self): + super(EnterpriseConsentTestCase, self).setUp() + self.user = UserFactory.create() + self.assertTrue(self.client.login(username=self.user.username, password='test')) + self.course = CourseFactory.create() + CourseEnrollmentFactory(user=self.user, course_id=self.course.id) + + def test_consent_required(self): + """ + Test that enterprise data sharing consent is required when enabled for the various courseware views. + """ + course_id = unicode(self.course.id) + for url in ( + reverse("courseware", kwargs=dict(course_id=course_id)), + reverse("progress", kwargs=dict(course_id=course_id)), + reverse("student_progress", kwargs=dict(course_id=course_id, student_id=str(self.user.id))), + ): + self.verify_consent_required(self.client, url) diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index e07f055760..40a36cee54 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -31,7 +31,7 @@ from shoppingcart.models import CourseRegistrationCode from student.models import CourseEnrollment from student.views import is_course_blocked from student.roles import GlobalStaff -from util.enterprise_helpers import consent_needed_for_course, get_course_specific_consent_url +from util.enterprise_helpers import get_enterprise_consent_url from util.views import ensure_valid_course_key from xmodule.modulestore.django import modulestore from xmodule.x_module import STUDENT_VIEW @@ -203,13 +203,14 @@ class CoursewareIndex(View): the course, and redirect the user to provide consent if needed. """ course_id = unicode(self.course_key) - if consent_needed_for_course(self.real_user, course_id): + consent_url = get_enterprise_consent_url(self.request, course_id, user=self.real_user, return_to='courseware') + if consent_url: log.warning( u'User %s cannot access the course %s because they have not granted consent', self.real_user, course_id, ) - raise Redirect(get_course_specific_consent_url(self.request, course_id, 'courseware')) + raise Redirect(consent_url) def _redirect_if_needed_to_pay_for_course(self): """ diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index b353cf310e..6a01baf548 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -93,7 +93,7 @@ from student.roles import GlobalStaff from util.cache import cache, cache_if_anonymous from util.date_utils import strftime_localized from util.db import outer_atomic -from util.enterprise_helpers import consent_needed_for_course, get_course_specific_consent_url +from util.enterprise_helpers import get_enterprise_consent_url from util.milestones_helpers import get_prerequisite_courses_display from util.views import _record_feedback_in_zendesk from util.views import ensure_valid_course_key, ensure_valid_usage_key @@ -330,8 +330,9 @@ def course_info(request, course_id): # If the user is sponsored by an enterprise customer, and we still need to get data # sharing consent, redirect to do that first. - if consent_needed_for_course(user, course_id): - return redirect(get_course_specific_consent_url(request, course_id, 'info')) + consent_url = get_enterprise_consent_url(request, course_id, user=user, return_to='info') + if consent_url: + return redirect(consent_url) # If the user needs to take an entrance exam to access this course, then we'll need # to send them to that specific course module before allowing them into other areas @@ -818,8 +819,9 @@ def _progress(request, course_key, student_id): # If the user is sponsored by an enterprise customer, and we still need to get data # sharing consent, redirect to do that first. - if consent_needed_for_course(request.user, unicode(course.id)): - return redirect(get_course_specific_consent_url(request, unicode(course.id), 'progress')) + consent_url = get_enterprise_consent_url(request, unicode(course.id), return_to='progress') + if consent_url: + return redirect(consent_url) # check to see if there is a required survey that must be taken before # the user can access the course. diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index cd727a4ced..518ddb6a47 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -23,6 +23,7 @@ from django_comment_client.utils import strip_none from lms.djangoapps.discussion import views from student.tests.factories import UserFactory, CourseEnrollmentFactory from util.testing import UrlResetMixin +from util.tests.mixins.enterprise import EnterpriseTestConsentRequired from openedx.core.djangoapps.util.testing import ContentGroupTestCase from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore @@ -356,16 +357,26 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): # course is outside the context manager that is verifying the number of queries, # and with split mongo, that method ends up querying disabled_xblocks (which is then # cached and hence not queried as part of call_single_thread). - (ModuleStoreEnum.Type.mongo, 1, 5, 3, 13, 1), - (ModuleStoreEnum.Type.mongo, 50, 5, 3, 13, 1), + (ModuleStoreEnum.Type.mongo, False, 1, 5, 3, 13, 1), + (ModuleStoreEnum.Type.mongo, False, 50, 5, 3, 13, 1), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, 1, 3, 3, 12, 1), - (ModuleStoreEnum.Type.split, 50, 3, 3, 12, 1), + (ModuleStoreEnum.Type.split, False, 1, 3, 3, 12, 1), + (ModuleStoreEnum.Type.split, False, 50, 3, 3, 12, 1), + + # Enabling Enterprise integration increases the number of (cached and uncached) SQL queries by 1, + # because the presence of the user's consent for the course must be checked. + # But there should be no effect on the number of mongo queries made. + (ModuleStoreEnum.Type.mongo, True, 1, 5, 3, 14, 2), + (ModuleStoreEnum.Type.mongo, True, 50, 5, 3, 14, 2), + # split mongo: 3 queries, regardless of thread response size. + (ModuleStoreEnum.Type.split, True, 1, 3, 3, 13, 2), + (ModuleStoreEnum.Type.split, True, 50, 3, 3, 13, 2), ) @ddt.unpack def test_number_of_mongo_queries( self, default_store, + enterprise_enabled, num_thread_responses, num_uncached_mongo_calls, num_cached_mongo_calls, @@ -393,12 +404,13 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): """ Call single_thread and assert that it returns what we expect. """ - response = views.single_thread( - request, - course.id.to_deprecated_string(), - "dummy_discussion_id", - test_thread_id - ) + with override_settings(ENABLE_ENTERPRISE_INTEGRATION=enterprise_enabled): + response = views.single_thread( + request, + course.id.to_deprecated_string(), + "dummy_discussion_id", + test_thread_id + ) self.assertEquals(response.status_code, 200) self.assertEquals(len(json.loads(response.content)["content"]["children"]), num_thread_responses) @@ -1566,3 +1578,47 @@ class EnrollmentTestCase(ForumsEnableMixin, ModuleStoreTestCase): request.user = self.student with self.assertRaises(UserNotEnrolled): views.forum_form_discussion(request, course_id=self.course.id.to_deprecated_string()) + + +@patch('requests.request', autospec=True) +class EnterpriseConsentTestCase(EnterpriseTestConsentRequired, ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): + """ + Ensure that the Enterprise Data Consent redirects are in place only when consent is required. + """ + CREATE_USER = False + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + # Invoke UrlResetMixin setUp + super(EnterpriseConsentTestCase, self).setUp() + + username = "foo" + password = "bar" + + self.discussion_id = 'dummy_discussion_id' + self.course = CourseFactory.create(discussion_topics={'dummy discussion': {'id': self.discussion_id}}) + self.student = UserFactory.create(username=username, password=password) + CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) + self.assertTrue( + self.client.login(username=username, password=password) + ) + + self.addCleanup(translation.deactivate) + + def test_consent_required(self, mock_request): + """ + Test that enterprise data sharing consent is required when enabled for the various discussion views. + """ + thread_id = 'dummy' + course_id = unicode(self.course.id) + mock_request.side_effect = make_mock_request_impl(course=self.course, text='dummy', thread_id=thread_id) + + for url in ( + reverse('discussion.views.forum_form_discussion', + kwargs=dict(course_id=course_id)), + reverse('discussion.views.inline_discussion', + kwargs=dict(course_id=course_id, discussion_id=self.discussion_id)), + reverse('discussion.views.single_thread', + kwargs=dict(course_id=course_id, discussion_id=self.discussion_id, thread_id=thread_id)), + ): + self.verify_consent_required(self.client, url) diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index db5df8d243..47fe80cc8b 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -47,6 +47,7 @@ from django_comment_client.utils import ( ) import django_comment_client.utils as utils import lms.lib.comment_client as cc +from util.enterprise_helpers import data_sharing_consent_required from opaque_keys.edx.keys import CourseKey @@ -179,6 +180,7 @@ def use_bulk_ops(view_func): @login_required +@data_sharing_consent_required @use_bulk_ops def inline_discussion(request, course_key, discussion_id): """ @@ -214,6 +216,7 @@ def inline_discussion(request, course_key, discussion_id): @login_required +@data_sharing_consent_required @use_bulk_ops def forum_form_discussion(request, course_key): """ @@ -256,6 +259,7 @@ def forum_form_discussion(request, course_key): @require_GET @login_required +@data_sharing_consent_required @use_bulk_ops def single_thread(request, course_key, discussion_id, thread_id): """