From aa6f88382d46af43b5523b7980b26ebad742a553 Mon Sep 17 00:00:00 2001 From: Amir Qayyum Khan Date: Wed, 3 Jan 2018 18:39:46 +0500 Subject: [PATCH] Fixed edX block structure api to load override data --- lms/djangoapps/course_api/blocks/api.py | 6 +- .../course_api/blocks/tests/test_api.py | 70 +++++++++++++- .../blocks/tests/test_serializers.py | 6 +- .../tests/test_block_completion.py | 4 +- .../transformers/tests/test_milestones.py | 2 +- lms/djangoapps/course_blocks/api.py | 42 ++++++--- .../transformers/load_override_data.py | 84 +++++++++++++++++ .../tests/test_load_override_data.py | 93 +++++++++++++++++++ .../block_structure/block_structure.py | 16 ++++ .../tests/test_block_structure.py | 40 ++++++++ setup.py | 1 + 11 files changed, 341 insertions(+), 23 deletions(-) create mode 100644 lms/djangoapps/course_blocks/transformers/load_override_data.py create mode 100644 lms/djangoapps/course_blocks/transformers/tests/test_load_override_data.py diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index bb2d42dd21..b8ab703056 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/lms/djangoapps/course_api/blocks/api.py @@ -2,7 +2,7 @@ API function for retrieving course blocks data """ -from lms.djangoapps.course_blocks.api import COURSE_BLOCK_ACCESS_TRANSFORMERS, get_course_blocks +import lms.djangoapps.course_blocks.api as course_blocks_api from lms.djangoapps.course_blocks.transformers.hidden_content import HiddenContentTransformer from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers @@ -59,7 +59,7 @@ def get_blocks( include_gated_sections = 'show_gated_sections' in requested_fields if user is not None: - transformers += COURSE_BLOCK_ACCESS_TRANSFORMERS + transformers += course_blocks_api.get_course_block_access_transformers() transformers += [MilestonesAndSpecialExamsTransformer( include_special_exams=include_special_exams, include_gated_sections=include_gated_sections)] @@ -77,7 +77,7 @@ def get_blocks( transformers += [BlockCompletionTransformer()] # transform - blocks = get_course_blocks(user, usage_key, transformers) + blocks = course_blocks_api.get_course_blocks(user, usage_key, transformers) # filter blocks by types if block_types_filter: diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index 913bc1a085..884db0b8cb 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -7,6 +7,10 @@ from itertools import product import ddt import django from django.test.client import RequestFactory +from django.test.utils import override_settings + +import course_blocks.api as course_blocks_api + from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle from student.tests.factories import UserFactory @@ -14,6 +18,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import SampleCourseFactory, check_mongo_calls + from ..api import get_blocks @@ -104,14 +109,14 @@ class TestGetBlocks(SharedModuleStoreTestCase): @ddt.ddt -class TestGetBlocksQueryCounts(SharedModuleStoreTestCase): +class TestGetBlocksQueryCountsBase(SharedModuleStoreTestCase): """ - Tests query counts for the get_blocks function. + Base for the get_blocks tests. """ ENABLED_SIGNALS = ['course_published'] def setUp(self): - super(TestGetBlocksQueryCounts, self).setUp() + super(TestGetBlocksQueryCountsBase, self).setUp() self.user = UserFactory.create() self.request = RequestFactory().get("/dummy") @@ -133,6 +138,12 @@ class TestGetBlocksQueryCounts(SharedModuleStoreTestCase): with self.assertNumQueries(expected_sql_queries): get_blocks(self.request, course.location, self.user) + +@ddt.ddt +class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase): + """ + Tests query counts for the get_blocks function. + """ @ddt.data( *product( (ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split), @@ -178,3 +189,56 @@ class TestGetBlocksQueryCounts(SharedModuleStoreTestCase): expected_mongo_queries, expected_sql_queries=num_sql_queries, ) + + +@ddt.ddt +@override_settings(FIELD_OVERRIDE_PROVIDERS=(course_blocks_api.INDIVIDUAL_STUDENT_OVERRIDE_PROVIDER, )) +class TestQueryCountsWithIndividualOverrideProvider(TestGetBlocksQueryCountsBase): + """ + Tests query counts for the get_blocks function when IndividualStudentOverrideProvider is set. + """ + @ddt.data( + *product( + (ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split), + (True, False), + ) + ) + @ddt.unpack + def test_query_counts_cached(self, store_type, with_storage_backing): + with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): + course = self._create_course(store_type) + self._get_blocks( + course, + expected_mongo_queries=0, + expected_sql_queries=7 if with_storage_backing else 6, + ) + + @ddt.data( + *product( + ((ModuleStoreEnum.Type.mongo, 5), (ModuleStoreEnum.Type.split, 3)), + (True, False), + ) + ) + @ddt.unpack + def test_query_counts_uncached(self, store_type_tuple, with_storage_backing): + store_type, expected_mongo_queries = store_type_tuple + with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): + course = self._create_course(store_type) + clear_course_from_cache(course.id) + + if with_storage_backing: + # TODO: Remove Django 1.11 upgrade shim + # SHIM: Django 1.11 results in a few more SAVEPOINTs due to: + # https://github.com/django/django/commit/d44afd88#diff-5b0dda5eb9a242c15879dc9cd2121379L485 + if django.VERSION >= (1, 11): + num_sql_queries = 17 + else: + num_sql_queries = 15 + else: + num_sql_queries = 7 + + self._get_blocks( + course, + expected_mongo_queries, + expected_sql_queries=num_sql_queries, + ) diff --git a/lms/djangoapps/course_api/blocks/tests/test_serializers.py b/lms/djangoapps/course_api/blocks/tests/test_serializers.py index 943ec25096..2956eeb266 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_serializers.py +++ b/lms/djangoapps/course_api/blocks/tests/test_serializers.py @@ -3,7 +3,7 @@ Tests for Course Blocks serializers """ from mock import MagicMock -from lms.djangoapps.course_blocks.api import COURSE_BLOCK_ACCESS_TRANSFORMERS, get_course_blocks +from lms.djangoapps.course_blocks.api import get_course_block_access_transformers, get_course_blocks from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers from student.roles import CourseStaffRole from student.tests.factories import UserFactory @@ -41,7 +41,9 @@ class TestBlockSerializerBase(SharedModuleStoreTestCase): block_types_to_count=['video'], requested_student_view_data=['video'], ) - self.transformers = BlockStructureTransformers(COURSE_BLOCK_ACCESS_TRANSFORMERS + [blocks_api_transformer]) + self.transformers = BlockStructureTransformers( + get_course_block_access_transformers() + [blocks_api_transformer] + ) self.block_structure = get_course_blocks( self.user, self.course.location, diff --git a/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py b/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py index baf7e138f3..3d1312d291 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py @@ -6,15 +6,13 @@ from completion.test_utils import CompletionWaffleTestMixin from xblock.core import XBlock from xblock.completable import CompletableXBlockMixin, XBlockCompletionMode +from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.course_api.blocks.transformers.block_completion import BlockCompletionTransformer from lms.djangoapps.course_blocks.transformers.tests.helpers import ModuleStoreTestCase, TransformerRegistryTestMixin from student.tests.factories import UserFactory from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from ...api import get_course_blocks - - class StubAggregatorXBlock(XBlock): """ XBlock to test behaviour of BlockCompletionTransformer diff --git a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py index 7f097a1c10..0a589790b7 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py @@ -7,12 +7,12 @@ from mock import Mock, patch from nose.plugins.attrib import attr from gating import api as lms_gating_api +from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.course_blocks.transformers.tests.helpers import CourseStructureTestCase from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers from openedx.core.lib.gating import api as gating_api from student.tests.factories import CourseEnrollmentFactory -from ...api import get_course_blocks from ..milestones import MilestonesAndSpecialExamsTransformer diff --git a/lms/djangoapps/course_blocks/api.py b/lms/djangoapps/course_blocks/api.py index f48cdccbec..a2dda446e0 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -2,20 +2,40 @@ API entry point to the course_blocks app with top-level get_course_blocks function. """ +from django.conf import settings + from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers -from .transformers import library_content, start_date, user_partitions, visibility +from .transformers import library_content, start_date, user_partitions, visibility, load_override_data from .usage_info import CourseUsageInfo -# Default list of transformers for manipulating course block structures -# based on the user's access to the course blocks. -COURSE_BLOCK_ACCESS_TRANSFORMERS = [ - library_content.ContentLibraryTransformer(), - start_date.StartDateTransformer(), - user_partitions.UserPartitionTransformer(), - visibility.VisibilityTransformer(), -] +INDIVIDUAL_STUDENT_OVERRIDE_PROVIDER = 'courseware.student_field_overrides.IndividualStudentOverrideProvider' + + +def has_individual_student_override_provider(): + """ + check if FIELD_OVERRIDE_PROVIDERS has class + `courseware.student_field_overrides.IndividualStudentOverrideProvider` + """ + return INDIVIDUAL_STUDENT_OVERRIDE_PROVIDER in getattr(settings, 'FIELD_OVERRIDE_PROVIDERS', ()) + + +def get_course_block_access_transformers(): + """ + Default list of transformers for manipulating course block structures + based on the user's access to the course blocks. + """ + course_block_access_transformers = [ + library_content.ContentLibraryTransformer(), + start_date.StartDateTransformer(), + user_partitions.UserPartitionTransformer(), + visibility.VisibilityTransformer(), + ] + if has_individual_student_override_provider(): + course_block_access_transformers += [load_override_data.OverrideDataTransformer()] + + return course_block_access_transformers def get_course_blocks( @@ -38,7 +58,7 @@ def get_course_blocks( transformers (BlockStructureTransformers) - A collection of transformers whose transform methods are to be called. - If None, COURSE_BLOCK_ACCESS_TRANSFORMERS is used. + If None, get_course_block_access_transformers() is used. collected_block_structure (BlockStructureBlockData) - A block structure retrieved from a prior call to @@ -55,7 +75,7 @@ def get_course_blocks( access. """ if not transformers: - transformers = BlockStructureTransformers(COURSE_BLOCK_ACCESS_TRANSFORMERS) + transformers = BlockStructureTransformers(get_course_block_access_transformers()) transformers.usage_info = CourseUsageInfo(starting_block_usage_key.course_key, user) return get_block_structure_manager(starting_block_usage_key.course_key).get_transformed( diff --git a/lms/djangoapps/course_blocks/transformers/load_override_data.py b/lms/djangoapps/course_blocks/transformers/load_override_data.py new file mode 100644 index 0000000000..19662097ad --- /dev/null +++ b/lms/djangoapps/course_blocks/transformers/load_override_data.py @@ -0,0 +1,84 @@ +""" +Load Override Data Transformer +""" +import json + +from openedx.core.djangoapps.content.block_structure.transformer import BlockStructureTransformer + +from courseware.models import StudentFieldOverride + +# The list of fields are in support of Individual due dates and could be expanded for other use cases. +REQUESTED_FIELDS = [ + 'start', + 'display_name', + 'due' +] + + +def _get_override_query(course_key, location_list): + """ + returns queryset containing override data. + + Args: + course_key (CourseLocator): Course locator object + location_list (List): List of usage key of all blocks + """ + return StudentFieldOverride.objects.filter( + course_id=course_key, + location__in=location_list, + field__in=REQUESTED_FIELDS + ) + + +def override_xblock_fields(course_key, location_list, block_structure): + """ + loads override data of block + + Args: + course_key (CourseLocator): course locator object + location_list (List): list of usage key of all blocks + block_structure (BlockStructure): block structure class + """ + query = _get_override_query(course_key, location_list) + for student_field_override in query: + value = json.loads(student_field_override.value) + field = student_field_override.field + block_structure.override_xblock_field( + student_field_override.location, + field, + value + ) + + +class OverrideDataTransformer(BlockStructureTransformer): + """ + A transformer that load override data in xblock. + """ + WRITE_VERSION = 1 + READ_VERSION = 1 + + @classmethod + def name(cls): + """ + Unique identifier for the transformer's class; + same identifier used in setup.py. + """ + return "load_override_data" + + @classmethod + def collect(cls, block_structure): + """ + Collects any information that's necessary to execute this transformer's transform method. + """ + # collect basic xblock fields + block_structure.request_xblock_fields(*REQUESTED_FIELDS) + + def transform(self, usage_info, block_structure): + """ + loads override data into blocks + """ + override_xblock_fields( + usage_info.course_key, + block_structure.topological_traversal(), + block_structure + ) diff --git a/lms/djangoapps/course_blocks/transformers/tests/test_load_override_data.py b/lms/djangoapps/course_blocks/transformers/tests/test_load_override_data.py new file mode 100644 index 0000000000..bfc607b64e --- /dev/null +++ b/lms/djangoapps/course_blocks/transformers/tests/test_load_override_data.py @@ -0,0 +1,93 @@ +""" +Tests for OverrideDataTransformer. +""" +import datetime + +import ddt +import pytz +from courseware.student_field_overrides import get_override_for_user, override_field_for_user +from student.tests.factories import CourseEnrollmentFactory, UserFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import ToyCourseFactory + +from lms.djangoapps.course_blocks.transformers.load_override_data import REQUESTED_FIELDS, OverrideDataTransformer +from openedx.core.djangoapps.content.block_structure.factory import BlockStructureFactory + +expected_overrides = { + 'start': datetime.datetime( + 2017, 1, 20, 2, 42, tzinfo=pytz.UTC + ), + 'display_name': "Section", + 'due': datetime.datetime( + 2017, 2, 20, 2, 42, tzinfo=pytz.UTC + ) +} + + +@ddt.ddt +class TestOverrideDataTransformer(ModuleStoreTestCase): + """ + Test proper behavior for OverrideDataTransformer + """ + @classmethod + def setUpClass(cls): + super(TestOverrideDataTransformer, cls).setUpClass() + cls.learner = UserFactory.create(password="test") + + def setUp(self): + super(TestOverrideDataTransformer, self).setUp() + self.course_key = ToyCourseFactory.create().id + self.course_usage_key = self.store.make_course_usage_key(self.course_key) + self.block_structure = BlockStructureFactory.create_from_modulestore(self.course_usage_key, self.store) + self.course = course = modulestore().get_course(self.course_key) + section = course.get_children()[0] + subsection = section.get_children()[0] + self.block = self.store.create_child( + self.learner.id, subsection.location, 'html', 'new_component' + ) + CourseEnrollmentFactory.create(user=self.learner, course_id=self.course_key, is_active=True) + + @ddt.data(*REQUESTED_FIELDS) + def test_transform(self, field): + override_field_for_user( + self.learner, + self.block, + field, + expected_overrides.get(field) + ) + + # collect phase + OverrideDataTransformer.collect(self.block_structure) + + # transform phase + OverrideDataTransformer().transform( + usage_info=self.course_usage_key, + block_structure=self.block_structure, + ) + + # verify overridden data + assert get_override_for_user(self.learner, self.block, field) == expected_overrides.get(field) + + def test_transform_all_fields(self): + """Test overriding of all fields""" + for field in REQUESTED_FIELDS: + override_field_for_user( + self.learner, + self.block, + field, + expected_overrides.get(field) + ) + + # collect phase + OverrideDataTransformer.collect(self.block_structure) + + # transform phase + OverrideDataTransformer().transform( + usage_info=self.course_usage_key, + block_structure=self.block_structure, + ) + + # verify overridden data + for field in REQUESTED_FIELDS: + assert get_override_for_user(self.learner, self.block, field) == expected_overrides.get(field) diff --git a/openedx/core/djangoapps/content/block_structure/block_structure.py b/openedx/core/djangoapps/content/block_structure/block_structure.py index bd92216ac6..5d74fd2049 100644 --- a/openedx/core/djangoapps/content/block_structure/block_structure.py +++ b/openedx/core/djangoapps/content/block_structure/block_structure.py @@ -466,6 +466,22 @@ class BlockStructureBlockData(BlockStructure): block_data = self._block_data_map.get(usage_key) return getattr(block_data, field_name, default) if block_data else default + def override_xblock_field(self, usage_key, field_name, override_data): + """ + Set value of the XBlock field for the requested block for the requested field_name; + + Arguments: + usage_key (UsageKey) - Usage key of the block whose xBlock + field is requested. + + field_name (string) - The name of the field that is + requested. + + override_data (object) - The data you want to set + """ + block_data = self._block_data_map.get(usage_key) + setattr(block_data, field_name, override_data) + def get_transformer_data(self, transformer, key, default=None): """ Returns the value associated with the given key from the given diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_block_structure.py b/openedx/core/djangoapps/content/block_structure/tests/test_block_structure.py index e9f27e3b3c..cae3756ca7 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_block_structure.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_block_structure.py @@ -1,6 +1,7 @@ """ Tests for block_structure.py """ +from datetime import datetime # pylint: disable=protected-access from collections import namedtuple from copy import deepcopy @@ -156,6 +157,45 @@ class TestBlockStructureData(TestCase, ChildrenMapTestMixin): block.field_map.get(field), ) + def test_xblock_field_override(self): + # block field override test cases + attribute = { + "start": datetime(2017, 3, 23, 16, 38, 46, 271475), + "display_name": "Foo block", + "due": datetime(2017, 5, 23, 16, 38, 46, 271475) + } + override_due_date = datetime(2018, 2, 23, 16, 38, 46, 271475) + block = MockXBlock("Foo", attribute) + + # add each block + block_structure = BlockStructureModulestoreData(root_block_usage_key=0) + block_structure._add_xblock(block.location, block) + block_structure._get_or_create_block(block.location) + + fields = attribute.keys() + block_structure.request_xblock_fields(*fields) + + # collect fields + block_structure._collect_requested_xblock_fields() + + # verify values of collected fields + bs_block = block_structure[block.location] + for field in fields: + self.assertEquals( + getattr(bs_block, field, None), + block.field_map.get(field), + ) + + block_structure.override_xblock_field( + block.location, + "due", + override_due_date + ) + self.assertEquals( + block_structure.get_xblock_field(block.location, "due", None), + override_due_date + ) + @ddt.data( *itertools.product( [True, False], diff --git a/setup.py b/setup.py index 0e9845e2e0..08801c888e 100644 --- a/setup.py +++ b/setup.py @@ -59,6 +59,7 @@ setup( "milestones = lms.djangoapps.course_api.blocks.transformers.milestones:MilestonesAndSpecialExamsTransformer", "grades = lms.djangoapps.grades.transformer:GradesTransformer", "completion = lms.djangoapps.course_api.blocks.transformers.block_completion:BlockCompletionTransformer", + "load_override_data = lms.djangoapps.course_blocks.transformers.load_override_data:OverrideDataTransformer" ], "openedx.ace.policy": [ "bulk_email_optout = lms.djangoapps.bulk_email.policies:CourseEmailOptout"