From 325fbe4eb28a76b22c238d87c4b90cfb00b9ffb9 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 17 Dec 2018 10:45:28 -0500 Subject: [PATCH] Hide empty container blocks from the course blocks api when called by the mobile app All code in this PR should be removed after REVE-52 is merged and mobile traffic from older app versions falls to < 5% of the mobile traffic to the course_blocks api --- .../xmodule/modulestore/tests/factories.py | 1 + lms/djangoapps/course_api/blocks/api.py | 18 +++++-- .../course_api/blocks/tests/test_api.py | 51 +++++++++++++++++++ .../course_blocks/transformers/hide_empty.py | 43 ++++++++++++++++ setup.py | 1 + 5 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 lms/djangoapps/course_blocks/transformers/hide_empty.py diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 0659c682fe..8bce4a184d 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -149,6 +149,7 @@ class SampleCourseFactory(CourseFactory): block_info_tree = kwargs.pop('block_info_tree', default_block_info_tree) store = kwargs.get('modulestore') user_id = kwargs.get('user_id', ModuleStoreEnum.UserID.test) + with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, None): course = super(SampleCourseFactory, cls)._create(target_class, **kwargs) diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index dbac1db1f7..8c0c586db4 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/lms/djangoapps/course_api/blocks/api.py @@ -4,7 +4,9 @@ API function for retrieving course blocks data import lms.djangoapps.course_blocks.api as course_blocks_api from lms.djangoapps.course_blocks.transformers.hidden_content import HiddenContentTransformer +from lms.djangoapps.course_blocks.transformers.hide_empty import HideEmptyTransformer from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers +from openedx.core.lib.mobile_utils import is_request_from_mobile_app from .serializers import BlockDictSerializer, BlockSerializer from .transformers.blocks_api import BlocksAPITransformer @@ -60,10 +62,18 @@ def get_blocks( if user is not None: transformers += course_blocks_api.get_course_block_access_transformers(user) - transformers += [MilestonesAndSpecialExamsTransformer( - include_special_exams=include_special_exams, - include_gated_sections=include_gated_sections)] - transformers += [HiddenContentTransformer()] + transformers += [ + MilestonesAndSpecialExamsTransformer( + include_special_exams=include_special_exams, + include_gated_sections=include_gated_sections + ), + HiddenContentTransformer() + ] + + # TODO: Remove this after REVE-52 lands and old-mobile-app traffic falls to < 5% of mobile traffic + if is_request_from_mobile_app(request): + transformers += [HideEmptyTransformer()] + transformers += [ BlocksAPITransformer( block_counts, diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index 14eb1992b8..c721eb3b48 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -3,6 +3,7 @@ Tests for Blocks api.py """ from itertools import product +from mock import patch import ddt from django.test.client import RequestFactory @@ -16,6 +17,7 @@ from student.tests.factories import UserFactory from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import SampleCourseFactory, check_mongo_calls +from xmodule.modulestore.tests.sample_courses import BlockInfo from ..api import get_blocks @@ -109,6 +111,55 @@ class TestGetBlocks(SharedModuleStoreTestCase): self.assertEqual(block['type'], 'problem') +# TODO: Remove this class after REVE-52 lands and old-mobile-app traffic falls to < 5% of mobile traffic +@ddt.ddt +class TestGetBlocksMobileHack(SharedModuleStoreTestCase): + """ + Tests that requests from the mobile app don't receive empty containers. + """ + shard = 4 + + @classmethod + def setUpClass(cls): + super(TestGetBlocksMobileHack, cls).setUpClass() + with cls.store.default_store(ModuleStoreEnum.Type.split): + cls.course = SampleCourseFactory.create( + block_info_tree=[ + BlockInfo('empty_chapter', 'chapter', {}, [ + BlockInfo('empty_sequential', 'sequential', {}, [ + BlockInfo('empty_vertical', 'vertical', {}, []), + ]), + ]), + BlockInfo('full_chapter', 'chapter', {}, [ + BlockInfo('full_sequential', 'sequential', {}, [ + BlockInfo('full_vertical', 'vertical', {}, [ + BlockInfo('html', 'html', {}, []), + ]), + ]), + ]) + ] + ) + + def setUp(self): + super(TestGetBlocksMobileHack, self).setUp() + self.user = UserFactory.create() + self.request = RequestFactory().get("/dummy") + self.request.user = self.user + + @ddt.data( + *product([True, False], ['chapter', 'sequential', 'vertical']) + ) + @ddt.unpack + def test_empty_containers(self, is_mobile, container_type): + with patch('lms.djangoapps.course_api.blocks.api.is_request_from_mobile_app', return_value=is_mobile): + blocks = get_blocks(self.request, self.course.location) + full_container_key = self.course.id.make_usage_key(container_type, 'full_{}'.format(container_type)) + self.assertIn(str(full_container_key), blocks['blocks']) + empty_container_key = self.course.id.make_usage_key(container_type, 'empty_{}'.format(container_type)) + assert_containment = self.assertNotIn if is_mobile else self.assertIn + assert_containment(str(empty_container_key), blocks['blocks']) + + @ddt.ddt class TestGetBlocksQueryCountsBase(SharedModuleStoreTestCase): """ diff --git a/lms/djangoapps/course_blocks/transformers/hide_empty.py b/lms/djangoapps/course_blocks/transformers/hide_empty.py new file mode 100644 index 0000000000..cbae63cc04 --- /dev/null +++ b/lms/djangoapps/course_blocks/transformers/hide_empty.py @@ -0,0 +1,43 @@ +""" +Hide Empty Transformer implementation. +""" +# TODO: Remove this file after REVE-52 lands and old-mobile-app traffic falls to < 5% of mobile traffic +from openedx.core.djangoapps.content.block_structure.transformer import ( + BlockStructureTransformer +) + + +class HideEmptyTransformer(BlockStructureTransformer): + """ + A transformer that removes any block from the course that could have + children but doesn't. + """ + WRITE_VERSION = 1 + READ_VERSION = 1 + + @classmethod + def name(cls): + """ + Unique identifier for the transformer's class; + same identifier used in setup.py. + """ + return "hide_empty" + + @classmethod + def collect(cls, block_structure): + """ + Collects any information that's necessary to execute this + transformer's transform method. + """ + block_structure.request_xblock_fields('children', 'has_children') + + def transform(self, usage_info, block_structure): + def _filter(block_key): + has_children = block_structure.get_xblock_field(block_key, 'has_children') + children = block_structure.get_xblock_field(block_key, 'children') + return has_children and not any(child in block_structure for child in children) + + for _ in block_structure.post_order_traversal( + filter_func=block_structure.create_removal_filter(_filter) + ): + pass diff --git a/setup.py b/setup.py index 446b43eb1c..c469b62539 100644 --- a/setup.py +++ b/setup.py @@ -55,6 +55,7 @@ setup( "start_date = lms.djangoapps.course_blocks.transformers.start_date:StartDateTransformer", "user_partitions = lms.djangoapps.course_blocks.transformers.user_partitions:UserPartitionTransformer", "visibility = lms.djangoapps.course_blocks.transformers.visibility:VisibilityTransformer", + "hide_empty = lms.djangoapps.course_blocks.transformers.hide_empty:HideEmptyTransformer", "hidden_content = lms.djangoapps.course_blocks.transformers.hidden_content:HiddenContentTransformer", "course_blocks_api = lms.djangoapps.course_api.blocks.transformers.blocks_api:BlocksAPITransformer", "milestones = lms.djangoapps.course_api.blocks.transformers.milestones:MilestonesAndSpecialExamsTransformer",