From 009c1f7901255e04dfa1eda3b290ccfcafeb9f0d Mon Sep 17 00:00:00 2001
From: Gabe Mulley <gabe@edx.org>
Date: Wed, 14 Nov 2018 16:35:34 -0500
Subject: [PATCH] REVE-81: fix errors in drag and drop and ORA2 xblocks
Also fixes REVE-82
---
lms/djangoapps/courseware/field_overrides.py | 5 +-
.../content_type_gating/field_override.py | 22 ++-
.../content_type_gating/tests/test_access.py | 126 +++++++++---------
3 files changed, 83 insertions(+), 70 deletions(-)
diff --git a/lms/djangoapps/courseware/field_overrides.py b/lms/djangoapps/courseware/field_overrides.py
index 2340f01a5ec..d2c3d93ffa8 100644
--- a/lms/djangoapps/courseware/field_overrides.py
+++ b/lms/djangoapps/courseware/field_overrides.py
@@ -102,8 +102,9 @@ class FieldOverrideProvider(object):
"""
__metaclass__ = ABCMeta
- def __init__(self, user):
+ def __init__(self, user, fallback_field_data):
self.user = user
+ self.fallback_field_data = fallback_field_data
@abstractmethod
def get(self, block, name, default): # pragma no cover
@@ -196,7 +197,7 @@ class OverrideFieldData(FieldData):
def __init__(self, user, fallback, providers):
self.fallback = fallback
- self.providers = tuple(provider(user) for provider in providers)
+ self.providers = tuple(provider(user, fallback) for provider in providers)
def get_override(self, block, name):
"""
diff --git a/openedx/features/content_type_gating/field_override.py b/openedx/features/content_type_gating/field_override.py
index c01d0c13aa0..42d22a811a5 100644
--- a/openedx/features/content_type_gating/field_override.py
+++ b/openedx/features/content_type_gating/field_override.py
@@ -4,7 +4,7 @@ students in the Unlocked Group of the ContentTypeGating partition.
"""
from django.conf import settings
-from lms.djangoapps.courseware.field_overrides import FieldOverrideProvider, disable_overrides
+from lms.djangoapps.courseware.field_overrides import FieldOverrideProvider
from openedx.features.content_type_gating.partitions import CONTENT_GATING_PARTITION_ID
from openedx.features.course_duration_limits.config import (
CONTENT_TYPE_GATING_FLAG,
@@ -31,9 +31,23 @@ class ContentTypeGatingFieldOverride(FieldOverrideProvider):
if not problem_eligible_for_content_gating:
return default
- # Read the group_access from the fallback field-data service
- with disable_overrides():
- original_group_access = block.group_access
+ # We want to fetch the value set by course authors since it should take precedence.
+ # We cannot simply call "block.group_access" to fetch that value even if we disable
+ # field overrides since it will set the group access field to "dirty" with
+ # the value read from the course content. Since most content does not have any
+ # value for this field it will usually be the default empty dict. This field
+ # override changes the value, however, resulting in the LMS thinking that the
+ # field data needs to be written back out to the store. This doesn't work,
+ # however, since this is a read-only setting in the LMS context. After this
+ # call to get() returns, the _dirty_fields dict will be set correctly to contain
+ # the value from this field override. This prevents the system from attempting
+ # to save the overridden value when it thinks it has changed when it hasn't.
+ original_group_access = None
+ if self.fallback_field_data.has(block, 'group_access'):
+ raw_value = self.fallback_field_data.get(block, 'group_access')
+ group_access_field = block.fields.get('group_access')
+ if group_access_field is not None:
+ original_group_access = group_access_field.from_json(raw_value)
if original_group_access is None:
original_group_access = {}
diff --git a/openedx/features/content_type_gating/tests/test_access.py b/openedx/features/content_type_gating/tests/test_access.py
index 2ad97ffa9ce..3fb1a8ea0f1 100644
--- a/openedx/features/content_type_gating/tests/test_access.py
+++ b/openedx/features/content_type_gating/tests/test_access.py
@@ -3,17 +3,17 @@ Test audit user's access to various content based on content-gating features.
"""
import ddt
-from django.http import Http404
+from django.conf import settings
from django.test.client import RequestFactory
from django.test.utils import override_settings
from django.urls import reverse
from mock import patch
from course_modes.tests.factories import CourseModeFactory
-from courseware.access_response import IncorrectPartitionGroupError
from lms.djangoapps.courseware.module_render import load_single_xblock
from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag
from openedx.core.lib.url_utils import quote_slashes
+from openedx.features.content_type_gating.partitions import CONTENT_GATING_PARTITION_ID
from openedx.features.course_duration_limits.config import CONTENT_TYPE_GATING_FLAG
from student.tests.factories import TEST_PASSWORD, AdminFactory, CourseEnrollmentFactory, UserFactory
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
@@ -82,14 +82,14 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase):
cls.graded_score_weight_blocks[(graded, has_score, weight)] = block
# add LTI blocks to default course
- cls.lti_block = ItemFactory.create(
+ cls.blocks_dict['lti_block'] = ItemFactory.create(
parent=cls.blocks_dict['vertical'],
category='lti_consumer',
display_name='lti_consumer',
has_score=True,
graded=True,
)
- cls.lti_block_not_scored = ItemFactory.create(
+ cls.blocks_dict['lti_block_not_scored'] = ItemFactory.create(
parent=cls.blocks_dict['vertical'],
category='lti_consumer',
display_name='lti_consumer_2',
@@ -97,19 +97,32 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase):
)
# add ungraded problem for xblock_handler test
- cls.graded_problem = ItemFactory.create(
+ cls.blocks_dict['graded_problem'] = ItemFactory.create(
parent=cls.blocks_dict['vertical'],
category='problem',
display_name='graded_problem',
graded=True,
)
- cls.ungraded_problem = ItemFactory.create(
+ cls.blocks_dict['ungraded_problem'] = ItemFactory.create(
parent=cls.blocks_dict['vertical'],
category='problem',
display_name='ungraded_problem',
graded=False,
)
+ cls.blocks_dict['audit_visible_graded_problem'] = ItemFactory.create(
+ parent=cls.blocks_dict['vertical'],
+ category='problem',
+ display_name='audit_visible_graded_problem',
+ graded=True,
+ group_access={
+ CONTENT_GATING_PARTITION_ID: [
+ settings.CONTENT_TYPE_GATE_GROUP_IDS['limited_access'],
+ settings.CONTENT_TYPE_GATE_GROUP_IDS['full_access']
+ ]
+ },
+ )
+
# audit_only course only has an audit track available
cls.courses['audit_only'] = cls._create_course(
run='audit_only_course_run_1',
@@ -205,14 +218,14 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase):
display_name='Lesson 1 Vertical - Unit 1'
)
- for problem_type in component_types:
+ for component_type in component_types:
block = ItemFactory.create(
parent=blocks_dict['vertical'],
- category=problem_type,
- display_name=problem_type,
+ category=component_type,
+ display_name=component_type,
graded=True,
)
- blocks_dict[problem_type] = block
+ blocks_dict[component_type] = block
return {
'course': course,
@@ -224,14 +237,8 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase):
"""
Asserts that a block in a specific course is gated for a specific user
- This functions asserts whether the passed in block is gated by content type gating.
- This is determined by checking whether the has_access method called the IncorrectPartitionGroupError.
- This error gets swallowed up and is raised as a 404, which is why we are checking for a 404 being raised.
- However, the 404 could also be caused by other errors, which is why the actual assertion is checking
- whether the IncorrectPartitionGroupError was called.
-
Arguments:
- block: some soft of xblock descriptor, must implement .scope_ids.usage_id
+ block: some sort of xblock descriptor, must implement .scope_ids.usage_id
is_gated (bool): if True, this user is expected to be gated from this block
user_id (int): id of user, if not set will be set to self.audit_user.id
course_id (CourseLocator): id of course, if not set will be set to self.course.id
@@ -239,61 +246,52 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase):
fake_request = self.factory.get('')
mock_get_current_request.return_value = fake_request
- with patch.object(IncorrectPartitionGroupError, '__init__',
- wraps=IncorrectPartitionGroupError.__init__) as mock_access_error:
- if is_gated:
- with self.assertRaises(Http404):
- load_single_xblock(
- request=fake_request,
- user_id=user_id,
- course_id=unicode(course_id),
- usage_key_string=unicode(block.scope_ids.usage_id),
- course=None
- )
- # check that has_access raised the IncorrectPartitionGroupError in order to gate the block
- self.assertTrue(mock_access_error.called)
- else:
- load_single_xblock(
- request=fake_request,
- user_id=user_id,
- course_id=unicode(course_id),
- usage_key_string=unicode(block.scope_ids.usage_id),
- course=None
- )
- # check that has_access did not raise the IncorrectPartitionGroupError thereby not gating the block
- self.assertFalse(mock_access_error.called)
-
- def test_lti_audit_access(self):
- """
- LTI stands for learning tools interoperability and is a 3rd party iframe that pulls in learning content from
- outside sources. This tests that audit users cannot see LTI components with graded content but can see the LTI
- components which do not have graded content.
- """
- self._assert_block_is_gated(
- block=self.lti_block,
- user_id=self.audit_user.id,
- course_id=self.course.id,
- is_gated=True
- )
- self._assert_block_is_gated(
- block=self.lti_block_not_scored,
- user_id=self.audit_user.id,
- course_id=self.course.id,
- is_gated=False
+ # Load a block we know will pass access control checks
+ vertical_xblock = load_single_xblock(
+ request=fake_request,
+ user_id=user_id,
+ course_id=unicode(course_id),
+ usage_key_string=unicode(self.blocks_dict['vertical'].scope_ids.usage_id),
+ course=None
)
+ runtime = vertical_xblock.runtime
+
+ # This method of fetching the block from the descriptor bypassess access checks
+ problem_block = runtime.get_module(block)
+
+ # Attempt to render the block, this should return different fragments if the content is gated or not.
+ frag = runtime.render(problem_block, 'student_view')
+ if is_gated:
+ assert 'content-paywall' in frag.content
+ else:
+ assert 'content-paywall' not in frag.content
+
@ddt.data(
- *PROBLEM_TYPES
+ ('problem', True),
+ ('openassessment', True),
+ ('drag-and-drop-v2', True),
+ ('done', True),
+ ('edx_sga', True),
+ ('lti_block', True),
+ ('ungraded_problem', False),
+ ('lti_block_not_scored', False),
+ ('audit_visible_graded_problem', False),
)
- def test_audit_fails_access_graded_problems(self, prob_type):
- block = self.blocks_dict[prob_type]
- is_gated = True
+ @ddt.unpack
+ def test_access_to_problems(self, prob_type, is_gated):
self._assert_block_is_gated(
- block=block,
- user_id=self.audit_user.id,
+ block=self.blocks_dict[prob_type],
+ user_id=self.users['audit'].id,
course_id=self.course.id,
is_gated=is_gated
)
+ self._assert_block_is_gated(
+ block=self.blocks_dict[prob_type],
+ user_id=self.users['verified'].id,
+ course_id=self.course.id,
+ is_gated=False
+ )
@ddt.data(
*GRADED_SCORE_WEIGHT_TEST_CASES
--
GitLab