From 16d440a4e9cd003e1608f19375385ecadcb49aeb Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 25 Jul 2024 15:15:42 -0400 Subject: [PATCH 01/16] feat: Turn ON the storage backing for cache by default. Any places where the storage backing cache for block structures was on conditionally previously it will be ON by default. --- .../content/block_structure/store.py | 45 +++++++------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/store.py b/openedx/core/djangoapps/content/block_structure/store.py index 2342079bdc6e..08a4f4acf5fd 100644 --- a/openedx/core/djangoapps/content/block_structure/store.py +++ b/openedx/core/djangoapps/content/block_structure/store.py @@ -120,13 +120,12 @@ def is_up_to_date(self, root_block_usage_key, modulestore): Returns whether the data in storage for the given key is already up-to-date with the version in the given modulestore. """ - if config.STORAGE_BACKING_FOR_CACHE.is_enabled(): - try: - bs_model = self._get_model(root_block_usage_key) - root_block = modulestore.get_item(root_block_usage_key) - return self._version_data_of_model(bs_model) == self._version_data_of_block(root_block) - except BlockStructureNotFound: - pass + try: + bs_model = self._get_model(root_block_usage_key) + root_block = modulestore.get_item(root_block_usage_key) + return self._version_data_of_model(bs_model) == self._version_data_of_block(root_block) + except BlockStructureNotFound: + pass return False @@ -134,26 +133,20 @@ def _get_model(self, root_block_usage_key): """ Returns the model associated with the given key. """ - if config.STORAGE_BACKING_FOR_CACHE.is_enabled(): - return BlockStructureModel.get(root_block_usage_key) - else: - return StubModel(root_block_usage_key) + return BlockStructureModel.get(root_block_usage_key) def _update_or_create_model(self, block_structure, serialized_data): """ Updates or creates the model for the given block_structure and serialized_data. """ - if config.STORAGE_BACKING_FOR_CACHE.is_enabled(): - root_block = block_structure[block_structure.root_block_usage_key] - bs_model, _ = BlockStructureModel.update_or_create( - serialized_data, - data_usage_key=block_structure.root_block_usage_key, - **self._version_data_of_block(root_block) - ) - return bs_model - else: - return StubModel(block_structure.root_block_usage_key) + root_block = block_structure[block_structure.root_block_usage_key] + bs_model, _ = BlockStructureModel.update_or_create( + serialized_data, + data_usage_key=block_structure.root_block_usage_key, + **self._version_data_of_block(root_block) + ) + return bs_model def _add_to_cache(self, serialized_data, bs_model): """ @@ -186,9 +179,6 @@ def _get_from_store(self, bs_model): Raises: BlockStructureNotFound if not found. """ - if not config.STORAGE_BACKING_FOR_CACHE.is_enabled(): - raise BlockStructureNotFound(bs_model.data_usage_key) - return bs_model.get_serialized_data() def _serialize(self, block_structure): @@ -228,12 +218,7 @@ def _encode_root_cache_key(bs_model): Returns the cache key to use for the given BlockStructureModel or StubModel. """ - if config.STORAGE_BACKING_FOR_CACHE.is_enabled(): - return str(bs_model) - return "v{version}.root.key.{root_usage_key}".format( - version=str(BlockStructureBlockData.VERSION), - root_usage_key=str(bs_model.data_usage_key), - ) + return str(bs_model) @staticmethod def _version_data_of_block(root_block): From 57cdbfa0134936e8b034e8def7e97390cac7c4c3 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 25 Jul 2024 15:42:40 -0400 Subject: [PATCH 02/16] test: Remove test variants that test without a cache. Since the cache is now always on, remove test cases that test the case when it's disabled. --- .../course_api/blocks/tests/test_api.py | 46 +++++++------------ .../content/block_structure/models.py | 4 +- .../block_structure/tests/test_manager.py | 22 ++++----- .../block_structure/tests/test_store.py | 43 ++++++----------- 4 files changed, 41 insertions(+), 74 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index e6fd74463aa9..93e400d4a804 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -3,16 +3,13 @@ """ -from itertools import product from unittest.mock import patch import ddt from django.test.client import RequestFactory -from edx_toggles.toggles.testutils import override_waffle_switch from common.djangoapps.student.tests.factories import UserFactory 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 from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import SampleCourseFactory, check_mongo_calls # lint-amnesty, pylint: disable=wrong-import-order @@ -209,34 +206,25 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase): Tests query counts for the get_blocks function. """ - @ddt.data( - *product( - (ModuleStoreEnum.Type.split, ), - (True, False), + @ddt.data(ModuleStoreEnum.Type.split) + def test_query_counts_cached(self, store_type): + course = self._create_course(store_type) + self._get_blocks( + course, + expected_mongo_queries=0, + expected_sql_queries=14, ) - ) - @ddt.unpack - def test_query_counts_cached(self, store_type, with_storage_backing): - with override_waffle_switch(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=14 if with_storage_backing else 13, - ) @ddt.data( - (ModuleStoreEnum.Type.split, 2, True, 24), - (ModuleStoreEnum.Type.split, 2, False, 14), + (ModuleStoreEnum.Type.split, 2, 24), ) @ddt.unpack - def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries): - with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): - course = self._create_course(store_type) - clear_course_from_cache(course.id) - - self._get_blocks( - course, - expected_mongo_queries, - expected_sql_queries=num_sql_queries, - ) + def test_query_counts_uncached(self, store_type, expected_mongo_queries, num_sql_queries): + course = self._create_course(store_type) + clear_course_from_cache(course.id) + + self._get_blocks( + course, + expected_mongo_queries, + expected_sql_queries=num_sql_queries, + ) diff --git a/openedx/core/djangoapps/content/block_structure/models.py b/openedx/core/djangoapps/content/block_structure/models.py index 4872e09346d9..c2837e1a77f0 100644 --- a/openedx/core/djangoapps/content/block_structure/models.py +++ b/openedx/core/djangoapps/content/block_structure/models.py @@ -74,7 +74,6 @@ def _bs_model_storage(): # .. setting_default: None # .. setting_description: Specifies the storage used for storage-backed block structure cache. # For more information, check https://github.com/openedx/edx-platform/pull/14571. - # .. setting_warnings: Depends on `block_structure.storage_backing_for_cache`. storage_class = settings.BLOCK_STRUCTURES_SETTINGS.get('STORAGE_CLASS') # .. setting_name: BLOCK_STRUCTURES_SETTINGS['STORAGE_KWARGS'] @@ -82,8 +81,7 @@ def _bs_model_storage(): # .. setting_description: Specifies the keyword arguments needed to setup the storage, which # would be used for storage-backed block structure cache. # For more information, check https://github.com/openedx/edx-platform/pull/14571. - # .. setting_warnings: Depends on `BLOCK_STRUCTURES_SETTINGS['STORAGE_CLASS']` and on - # `block_structure.storage_backing_for_cache`. + # .. setting_warnings: Depends on `BLOCK_STRUCTURES_SETTINGS['STORAGE_CLASS']` storage_kwargs = settings.BLOCK_STRUCTURES_SETTINGS.get('STORAGE_KWARGS', {}) return get_storage(storage_class, **storage_kwargs) diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py index 8e5b585ca879..acde986eeee1 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py @@ -5,10 +5,8 @@ import pytest import ddt from django.test import TestCase -from edx_toggles.toggles.testutils import override_waffle_switch from ..block_structure import BlockStructureBlockData -from ..config import STORAGE_BACKING_FOR_CACHE from ..exceptions import UsageKeyNotInBlockStructure from ..manager import BlockStructureManager from ..transformers import BlockStructureTransformers @@ -177,20 +175,18 @@ def test_get_collected_cached(self): self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False) assert TestTransformer1.collect_call_count == 1 - @ddt.data(True, False) - def test_update_collected_if_needed(self, with_storage_backing): - with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): - with mock_registered_transformers(self.registered_transformers): - assert TestTransformer1.collect_call_count == 0 + def test_update_collected_if_needed(self): + with mock_registered_transformers(self.registered_transformers): + assert TestTransformer1.collect_call_count == 0 - self.bs_manager.update_collected_if_needed() - assert TestTransformer1.collect_call_count == 1 + self.bs_manager.update_collected_if_needed() + assert TestTransformer1.collect_call_count == 1 - self.bs_manager.update_collected_if_needed() - expected_count = 1 if with_storage_backing else 2 - assert TestTransformer1.collect_call_count == expected_count + self.bs_manager.update_collected_if_needed() + expected_count = 1 + assert TestTransformer1.collect_call_count == expected_count - self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False) + self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False) def test_get_collected_transformer_version(self): self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True) diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_store.py b/openedx/core/djangoapps/content/block_structure/tests/test_store.py index 8d6fc8017d0f..63a02efa7af8 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_store.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_store.py @@ -4,11 +4,9 @@ import pytest import ddt -from edx_toggles.toggles.testutils import override_waffle_switch from openedx.core.djangolib.testing.utils import CacheIsolationTestCase -from ..config import STORAGE_BACKING_FOR_CACHE from ..config.models import BlockStructureConfiguration from ..exceptions import BlockStructureNotFound from ..store import BlockStructureStore @@ -46,40 +44,27 @@ def add_transformers(self): value=f'{transformer.name()} val', ) - @ddt.data(True, False) - def test_get_none(self, with_storage_backing): - with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): - with pytest.raises(BlockStructureNotFound): - self.store.get(self.block_structure.root_block_usage_key) - - @ddt.data(True, False) - def test_add_and_get(self, with_storage_backing): - with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): - self.store.add(self.block_structure) - stored_value = self.store.get(self.block_structure.root_block_usage_key) - assert stored_value is not None - self.assert_block_structure(stored_value, self.children_map) + def test_get_none(self): + with pytest.raises(BlockStructureNotFound): + self.store.get(self.block_structure.root_block_usage_key) - @ddt.data(True, False) - def test_delete(self, with_storage_backing): - with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): - self.store.add(self.block_structure) - self.store.delete(self.block_structure.root_block_usage_key) - with pytest.raises(BlockStructureNotFound): - self.store.get(self.block_structure.root_block_usage_key) + def test_add_and_get(self): + self.store.add(self.block_structure) + stored_value = self.store.get(self.block_structure.root_block_usage_key) + assert stored_value is not None + self.assert_block_structure(stored_value, self.children_map) - def test_uncached_without_storage(self): + def test_delete(self): self.store.add(self.block_structure) - self.mock_cache.map.clear() + self.store.delete(self.block_structure.root_block_usage_key) with pytest.raises(BlockStructureNotFound): self.store.get(self.block_structure.root_block_usage_key) def test_uncached_with_storage(self): - with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=True): - self.store.add(self.block_structure) - self.mock_cache.map.clear() - stored_value = self.store.get(self.block_structure.root_block_usage_key) - self.assert_block_structure(stored_value, self.children_map) + self.store.add(self.block_structure) + self.mock_cache.map.clear() + stored_value = self.store.get(self.block_structure.root_block_usage_key) + self.assert_block_structure(stored_value, self.children_map) @ddt.data(1, 5, None) def test_cache_timeout(self, timeout): From 261b4985cd7b3ebf7c158461d9c19d5222763bf7 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 25 Jul 2024 15:49:36 -0400 Subject: [PATCH 03/16] feat!: Drop an unnecessary function. Remove the `enable_storage_backing_for_cache_in_request` function and its uses in the platform. The function is no longer needed because the storage backing for the block_structure cache will be ON by default moving forward. BREAKING CHANGE: This `enable_storage_backing_for_cache_in_request` function no longer exists and any calls to it should be removed. The cache it enables is now always ON. --- .../content/block_structure/config/__init__.py | 9 --------- .../management/commands/generate_course_blocks.py | 12 +----------- .../commands/tests/test_generate_course_blocks.py | 7 ------- .../core/djangoapps/content/block_structure/tasks.py | 5 ----- 4 files changed, 1 insertion(+), 32 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/config/__init__.py b/openedx/core/djangoapps/content/block_structure/config/__init__.py index df82bfb068e2..58c00c2ebbd3 100644 --- a/openedx/core/djangoapps/content/block_structure/config/__init__.py +++ b/openedx/core/djangoapps/content/block_structure/config/__init__.py @@ -32,15 +32,6 @@ ) -def enable_storage_backing_for_cache_in_request(): - """ - Manually override the value of the STORAGE_BACKING_FOR_CACHE switch in the context of the request. - This function should not be replicated, as it accesses a protected member, and it shouldn't. - """ - # pylint: disable=protected-access - STORAGE_BACKING_FOR_CACHE._cached_switches[STORAGE_BACKING_FOR_CACHE.name] = True - - @request_cached() def num_versions_to_keep(): """ diff --git a/openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py b/openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py index 4da85f5c52bb..047c945fe406 100644 --- a/openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py +++ b/openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py @@ -10,7 +10,6 @@ import openedx.core.djangoapps.content.block_structure.api as api import openedx.core.djangoapps.content.block_structure.store as store import openedx.core.djangoapps.content.block_structure.tasks as tasks -from openedx.core.djangoapps.content.block_structure.config import enable_storage_backing_for_cache_in_request from openedx.core.lib.command_utils import ( get_mutually_exclusive_required_option, parse_course_keys, @@ -75,12 +74,6 @@ def add_arguments(self, parser): default=0, type=int, ) - parser.add_argument( - '--with_storage', - help='Store the course blocks in Storage, overriding value of the storage_backing_for_cache waffle switch', - action='store_true', - default=False, - ) def handle(self, *args, **options): @@ -129,9 +122,6 @@ def _generate_course_blocks(self, options, course_keys): """ Generates course blocks for the given course_keys per the given options. """ - if options.get('with_storage'): - enable_storage_backing_for_cache_in_request() - for course_key in course_keys: try: self._generate_for_course(options, course_key) @@ -150,7 +140,7 @@ def _generate_for_course(self, options, course_key): action = tasks.update_course_in_cache_v2 if options.get('force_update') else tasks.get_course_in_cache_v2 task_options = {'routing_key': options['routing_key']} if options.get('routing_key') else {} result = action.apply_async( - kwargs=dict(course_id=str(course_key), with_storage=options.get('with_storage')), + kwargs=dict(course_id=str(course_key)), **task_options ) log.info('BlockStructure: ENQUEUED generating for course: %s, task_id: %s.', course_key, result.id) diff --git a/openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py b/openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py index 5b08c5e95df7..430f374b07cc 100644 --- a/openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py +++ b/openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py @@ -85,15 +85,8 @@ def test_all_courses(self, force_update): assert mock_update_from_store.call_count == (self.num_courses if force_update else 0) def test_one_course(self): - self._assert_courses_not_in_block_cache(*self.course_keys) self.command.handle(courses=[str(self.course_keys[0])]) self._assert_courses_in_block_cache(self.course_keys[0]) - self._assert_courses_not_in_block_cache(*self.course_keys[1:]) - self._assert_courses_not_in_block_storage(*self.course_keys) - - def test_with_storage(self): - self.command.handle(with_storage=True, courses=[str(self.course_keys[0])]) - self._assert_courses_in_block_cache(self.course_keys[0]) self._assert_courses_in_block_storage(self.course_keys[0]) self._assert_courses_not_in_block_storage(*self.course_keys[1:]) diff --git a/openedx/core/djangoapps/content/block_structure/tasks.py b/openedx/core/djangoapps/content/block_structure/tasks.py index 7c3c2805fb10..26b8ecad0759 100644 --- a/openedx/core/djangoapps/content/block_structure/tasks.py +++ b/openedx/core/djangoapps/content/block_structure/tasks.py @@ -14,7 +14,6 @@ from xmodule.capa.responsetypes import LoncapaProblemError from openedx.core.djangoapps.content.block_structure import api -from openedx.core.djangoapps.content.block_structure.config import enable_storage_backing_for_cache_in_request from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order log = logging.getLogger('edx.celery.task') @@ -62,8 +61,6 @@ def _update_course_in_cache(self, **kwargs): """ Updates the course blocks (mongo -> BlockStructure) for the specified course. """ - if kwargs.get('with_storage'): - enable_storage_backing_for_cache_in_request() _call_and_retry_if_needed(self, api.update_course_in_cache, **kwargs) @@ -93,8 +90,6 @@ def _get_course_in_cache(self, **kwargs): """ Gets the course blocks for the specified course, updating the cache if needed. """ - if kwargs.get('with_storage'): - enable_storage_backing_for_cache_in_request() _call_and_retry_if_needed(self, api.get_course_in_cache, **kwargs) From fab0267757ca2851625707aee4c8e8e1e82a310a Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 25 Jul 2024 15:52:11 -0400 Subject: [PATCH 04/16] feat!: Drop the block_structure.storage_backing_for_cache waffle switch. This work is part of DEPR https://github.com/openedx/public-engineering/issues/32 Now that we've removed all uses for this switch remove the decleration as well. BREAKING CHANGE: The `block_structure.storage_backing_for_cache` will no longer exist and its value will be ignored. If you have this switch set in your instance you can remove it. The backing cache is now always ON. --- .../block_structure/config/__init__.py | 22 ------------------- 1 file changed, 22 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/config/__init__.py b/openedx/core/djangoapps/content/block_structure/config/__init__.py index 58c00c2ebbd3..79837df86be6 100644 --- a/openedx/core/djangoapps/content/block_structure/config/__init__.py +++ b/openedx/core/djangoapps/content/block_structure/config/__init__.py @@ -9,28 +9,6 @@ from .models import BlockStructureConfiguration -# Switches -# .. toggle_name: block_structure.storage_backing_for_cache -# .. toggle_implementation: WaffleSwitch -# .. toggle_default: False -# .. toggle_description: When enabled, block structures are stored in a more permanent storage, -# like a database, which provides an additional backup for cache misses, instead having them -# regenerated. The regenration of block structures is a time consuming process. Therefore, -# enabling this switch is recommended for Production. -# .. toggle_warning: Depends on `BLOCK_STRUCTURES_SETTINGS['STORAGE_CLASS']` and -# `BLOCK_STRUCTURES_SETTINGS['STORAGE_KWARGS']`. -# This switch will likely be deprecated and removed. -# The annotation will be updated with the DEPR ticket once that process has started. -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2017-02-23 -# .. toggle_target_removal_date: 2017-05-23 -# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/14512, -# https://github.com/openedx/edx-platform/pull/14770, -# https://openedx.atlassian.net/browse/DEPR-145 -STORAGE_BACKING_FOR_CACHE = WaffleSwitch( - "block_structure.storage_backing_for_cache", __name__ -) - @request_cached() def num_versions_to_keep(): From 9164e924652387e6044ce6e0c0500660323a7ca4 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 26 Jul 2024 15:03:24 -0400 Subject: [PATCH 05/16] test: Update a BlockStructureFactory test mixin. Previously, we were not caching BlockStructures to the database when we were adding them to the store by default. Now that we are doing that, the BlockStructureFactory test failed because it was taking a shortcut that would no longer work. It was just creating a blockstructure that had relations but not any block information. Now that we're always persisting updates to the database, this broke because to persist the structure to the database, we have to look up the block information from the block_structure which now fails. This change updates the test mixin to add more data so that the content can be persisted to the database successfully as a part of this test. --- .../content/block_structure/tests/helpers.py | 6 +++++- .../block_structure/tests/test_factory.py | 18 +++++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/tests/helpers.py b/openedx/core/djangoapps/content/block_structure/tests/helpers.py index 93655c79e16e..f0a20554e6d2 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/helpers.py +++ b/openedx/core/djangoapps/content/block_structure/tests/helpers.py @@ -274,10 +274,14 @@ def create_block_structure(self, children_map, block_structure_cls=BlockStructur # create empty block structure block_structure = block_structure_cls(root_block_usage_key=self.block_key_factory(0)) - # _add_relation + # _add_relation and blocks for parent, children in enumerate(children_map): + if isinstance(block_structure, BlockStructureBlockData): + block_structure._get_or_create_block(self.block_key_factory(parent)) # pylint: disable=protected-access for child in children: block_structure._add_relation(self.block_key_factory(parent), self.block_key_factory(child)) # pylint: disable=protected-access + if isinstance(block_structure, BlockStructureBlockData): + block_structure._get_or_create_block(self.block_key_factory(child)) # pylint: disable=protected-access return block_structure def get_parents_map(self, children_map): diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_factory.py b/openedx/core/djangoapps/content/block_structure/tests/test_factory.py index b9fb8c5e1077..00efa393d8fa 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_factory.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_factory.py @@ -5,6 +5,7 @@ import pytest from django.test import TestCase +from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.exceptions import ItemNotFoundError from ..exceptions import BlockStructureNotFound @@ -18,14 +19,22 @@ class TestBlockStructureFactory(TestCase, ChildrenMapTestMixin): Tests for BlockStructureFactory """ + def block_key_factory(self, block_id): + """ + Returns a usage_key object for the given block_id. + This overrides the method in the ChildrenMapTestMixin. + """ + return CourseKey.from_string("course-v1:org+course+run").make_usage_key("html", str(block_id)) + def setUp(self): super().setUp() self.children_map = self.SIMPLE_CHILDREN_MAP self.modulestore = MockModulestoreFactory.create(self.children_map, self.block_key_factory) def test_from_modulestore(self): + usage_key = CourseKey.from_string("course-v1:org+course+run").make_usage_key("html", "0") block_structure = BlockStructureFactory.create_from_modulestore( - root_block_usage_key=0, modulestore=self.modulestore + root_block_usage_key=usage_key, modulestore=self.modulestore ) self.assert_block_structure(block_structure, self.children_map) @@ -48,15 +57,18 @@ def test_from_cache(self): def test_from_cache_none(self): store = BlockStructureStore(MockCache()) + # Non-existent usage key + usage_key = CourseKey.from_string("course-v1:org+course+run").make_usage_key("html", "0") with pytest.raises(BlockStructureNotFound): BlockStructureFactory.create_from_store( - root_block_usage_key=0, + root_block_usage_key=usage_key, block_structure_store=store, ) def test_new(self): + usage_key = CourseKey.from_string("course-v1:org+course+run").make_usage_key("html", "0") block_structure = BlockStructureFactory.create_from_modulestore( - root_block_usage_key=0, modulestore=self.modulestore + root_block_usage_key=usage_key, modulestore=self.modulestore ) new_structure = BlockStructureFactory.create_new( block_structure.root_block_usage_key, From e52253d3e1cea8a41b99c9f56180293d97e8f407 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 29 Jul 2024 12:03:11 -0400 Subject: [PATCH 06/16] test: Change call counts with model back as default. We don't call the modulestore or update the cache here now that we are backed by the database model. Previously the cache would change because the `_encode_root_cache_key` function in `BlockStructureStore` class used the `BlockstoreBlockData.VERSION` as a part of the cache key when the data was not being cached in a DB model. --- .../djangoapps/content/block_structure/tests/test_manager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py index acde986eeee1..493e4c34714f 100644 --- a/openedx/core/djangoapps/content/block_structure/tests/test_manager.py +++ b/openedx/core/djangoapps/content/block_structure/tests/test_manager.py @@ -208,8 +208,8 @@ def test_get_collected_transformer_version(self): def test_get_collected_structure_version(self): self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True) BlockStructureBlockData.VERSION += 1 - self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True) - assert TestTransformer1.collect_call_count == 2 + self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False) + assert TestTransformer1.collect_call_count == 1 def test_clear(self): self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True) From e5f698a5a744a7e818a30542638113bbde9bacc7 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 29 Jul 2024 14:04:08 -0400 Subject: [PATCH 07/16] feat: Remove the unused `StubModel` class. Now that the model backed cache is on by default, we don't need to keep the StubModel object around. --- .../content/block_structure/store.py | 22 +------------------ 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/store.py b/openedx/core/djangoapps/content/block_structure/store.py index 08a4f4acf5fd..37a2b57449c0 100644 --- a/openedx/core/djangoapps/content/block_structure/store.py +++ b/openedx/core/djangoapps/content/block_structure/store.py @@ -19,26 +19,6 @@ logger = getLogger(__name__) # pylint: disable=C0103 -class StubModel: - """ - Stub model to use when storage backing is disabled. - By using this stub, we eliminate the need for extra - conditional statements in the code. - """ - - def __init__(self, root_block_usage_key): - self.data_usage_key = root_block_usage_key - - def __str__(self): - return str(self.data_usage_key) - - def delete(self): - """ - Noop delete method. - """ - pass # lint-amnesty, pylint: disable=unnecessary-pass - - class BlockStructureStore: """ Storage for BlockStructure objects. @@ -216,7 +196,7 @@ def _deserialize(self, serialized_data, root_block_usage_key): def _encode_root_cache_key(bs_model): """ Returns the cache key to use for the given - BlockStructureModel or StubModel. + BlockStructureModel. """ return str(bs_model) From db266e25f785a726b48bad12134e5c9a33b65663 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 29 Jul 2024 14:13:39 -0400 Subject: [PATCH 08/16] test: Update query counts. With the new block_structure model cache enabled by default, we're donig queries to it as a part of other tests. --- .../tests/test_field_override_performance.py | 2 +- .../transformers/tests/test_milestones.py | 4 ++-- lms/djangoapps/courseware/tests/test_views.py | 8 ++++---- .../grades/tests/test_course_grade_factory.py | 20 +++++++++---------- lms/djangoapps/grades/tests/test_tasks.py | 8 ++++---- .../tests/test_tasks_helper.py | 2 +- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 68926a24f2dd..a7128495eb59 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -234,7 +234,7 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 33 + QUERY_COUNT = 34 TEST_DATA = { ('no_overrides', 1, True, False): (QUERY_COUNT, 2), 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 e87e0e755685..8c7d60e93831 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py @@ -166,7 +166,7 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c self.course.enable_subsection_gating = True self.setup_gated_section(self.blocks[gated_block_ref], self.blocks[gating_block_ref]) - with self.assertNumQueries(5): + with self.assertNumQueries(14): self.get_blocks_and_check_against_expected(self.user, expected_blocks_before_completion) # clear the request cache to simulate a new request @@ -179,7 +179,7 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c self.user, ) - with self.assertNumQueries(6): + with self.assertNumQueries(4): self.get_blocks_and_check_against_expected(self.user, self.ALL_BLOCKS_EXCEPT_SPECIAL) def test_staff_access(self): diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index d38110b3b045..126233880ae1 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1472,8 +1472,8 @@ def test_view_certificate_link(self): self.assertContains(resp, "earned a certificate for this course.") @ddt.data( - (True, 53), - (False, 53), + (True, 54), + (False, 54), ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1488,13 +1488,13 @@ def test_progress_queries(self): ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) self.setup_course() with self.assertNumQueries( - 53, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST + 54, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST ), check_mongo_calls(2): self._get_progress_page() for _ in range(2): with self.assertNumQueries( - 38, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST + 39, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST ), check_mongo_calls(2): self._get_progress_page() diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index 2066b6cd0d7c..c47e80a3dae0 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -68,35 +68,35 @@ def _assert_section_order(course_grade): self.sequence2.display_name ] - with self.assertNumQueries(4), mock_get_score(1, 2): + with self.assertNumQueries(5), mock_get_score(1, 2): _assert_read(expected_pass=False, expected_percent=0) # start off with grade of 0 - num_queries = 42 + num_queries = 43 with self.assertNumQueries(num_queries), mock_get_score(1, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) - with self.assertNumQueries(3): + with self.assertNumQueries(4): _assert_read(expected_pass=True, expected_percent=0.5) # updated to grade of .5 - num_queries = 6 + num_queries = 7 with self.assertNumQueries(num_queries), mock_get_score(1, 4): grade_factory.update(self.request.user, self.course, force_update_subsections=False) - with self.assertNumQueries(3): + with self.assertNumQueries(4): _assert_read(expected_pass=True, expected_percent=0.5) # NOT updated to grade of .25 - num_queries = 18 + num_queries = 19 with self.assertNumQueries(num_queries), mock_get_score(2, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) - with self.assertNumQueries(3): + with self.assertNumQueries(4): _assert_read(expected_pass=True, expected_percent=1.0) # updated to grade of 1.0 - num_queries = 28 + num_queries = 29 with self.assertNumQueries(num_queries), mock_get_score(0, 0): # the subsection now is worth zero grade_factory.update(self.request.user, self.course, force_update_subsections=True) - with self.assertNumQueries(3): + with self.assertNumQueries(4): _assert_read(expected_pass=False, expected_percent=0.0) # updated to grade of 0.0 @ddt.data((True, False)) @@ -286,7 +286,7 @@ def test_grading_exception(self, mock_course_grade): else mock_course_grade.return_value for student in self.students ] - with self.assertNumQueries(11): + with self.assertNumQueries(20): all_course_grades, all_errors = self._course_grades_and_errors_for(self.course, self.students) assert {student: str(all_errors[student]) for student in all_errors} == { student3: 'Error for student3.', diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 4dfd2dbef97f..347879105aad 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -156,8 +156,8 @@ def test_block_structure_created_only_once(self): assert mock_block_structure_create.call_count == 1 @ddt.data( - (ModuleStoreEnum.Type.split, 2, 41, True), - (ModuleStoreEnum.Type.split, 2, 41, False), + (ModuleStoreEnum.Type.split, 2, 42, True), + (ModuleStoreEnum.Type.split, 2, 42, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -168,7 +168,7 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.split, 2, 41), + (ModuleStoreEnum.Type.split, 2, 42), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -255,7 +255,7 @@ def test_problem_block_with_restricted_access(self, mock_subsection_signal): UserPartition.scheme_extensions = None @ddt.data( - (ModuleStoreEnum.Type.split, 2, 41), + (ModuleStoreEnum.Type.split, 2, 42), ) @ddt.unpack def test_persistent_grades_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 1fb25aeb8c07..428d6ddc2d6f 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -406,7 +406,7 @@ def test_query_counts(self): with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): with check_mongo_calls(2): - with self.assertNumQueries(54): + with self.assertNumQueries(63): CourseGradeReport.generate(None, None, course.id, {}, 'graded') def test_inactive_enrollments(self): From cf37ca48b72656c613e7242658a823eed78cae07 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 2 Aug 2024 13:18:18 -0400 Subject: [PATCH 09/16] test: Update the course in the cache after it's got new content. Because signals are disabled by default for performance reasons, this doesn't happen automatically. So we manually refresh the course in the cache after all the changes have been made so that the course in the cache matches the latest version in the modulestore. --- .../transformers/tests/test_library_content.py | 5 +++++ .../grades/rest_api/v1/tests/test_gradebook_views.py | 7 +++++++ lms/djangoapps/grades/tests/integration/test_events.py | 4 ++++ lms/djangoapps/grades/tests/test_transformer.py | 2 ++ 4 files changed, 18 insertions(+) diff --git a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py index 7fdb58f7f612..5a4d7a0de11a 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_library_content.py @@ -8,6 +8,7 @@ from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers +import openedx.core.djangoapps.content.block_structure.api as bs_api from ...api import get_course_blocks from ..library_content import ContentLibraryOrderTransformer, ContentLibraryTransformer from .helpers import CourseStructureTestCase @@ -41,6 +42,8 @@ def setUp(self): self.course_hierarchy = self.get_course_hierarchy() self.blocks = self.build_course(self.course_hierarchy) self.course = self.blocks['course'] + # Do this manually because publish signals are not fired by default in tests. + bs_api.update_course_in_cache(self.course.id) clear_course_from_cache(self.course.id) # Enroll user in course. @@ -122,6 +125,7 @@ def test_content_library(self): ) assert len(list(raw_block_structure.get_block_keys())) == len(self.blocks) + bs_api.update_course_in_cache(self.course.id) clear_course_from_cache(self.course.id) trans_block_structure = get_course_blocks( self.user, @@ -175,6 +179,7 @@ def setUp(self): self.course_hierarchy = self.get_course_hierarchy() self.blocks = self.build_course(self.course_hierarchy) self.course = self.blocks['course'] + bs_api.update_course_in_cache(self.course.id) clear_course_from_cache(self.course.id) # Enroll user in course. diff --git a/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py b/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py index 5d7c428c2221..ab11daf36576 100644 --- a/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py +++ b/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py @@ -22,6 +22,7 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory +import openedx.core.djangoapps.content.block_structure.api as bs_api from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.roles import ( CourseBetaTesterRole, @@ -2228,6 +2229,12 @@ def test_get_override_for_unreleased_block(self): display_name='Unreleased Section', ) + # We need to update the course in the cache after we create the new block. + # Review Question: Should we be doing this here? Does it make sense to do + # this in the xmodule/modulestore/tests/factories.py BlockFactory class + # as a part of the publish there? + bs_api.update_course_in_cache(self.course_data.course_key) + resp = self.client.get( self.get_url(subsection_id=unreleased_subsection.location) ) diff --git a/lms/djangoapps/grades/tests/integration/test_events.py b/lms/djangoapps/grades/tests/integration/test_events.py index f058cc81799e..52c1f14bacd8 100644 --- a/lms/djangoapps/grades/tests/integration/test_events.py +++ b/lms/djangoapps/grades/tests/integration/test_events.py @@ -7,6 +7,7 @@ from crum import set_current_request +import openedx.core.djangoapps.content.block_structure.api as bs_api from xmodule.capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.tests.factories import UserFactory @@ -76,6 +77,9 @@ def setUp(self): CourseEnrollment.enroll(self.student, self.course.id) self.instructor = UserFactory.create(is_staff=True, username='test_instructor', password=self.TEST_PASSWORD) self.refresh_course() + # Since this doesn't happen automatically and we don't want to run all the publish signal handlers + # Just make sure we have the latest version of the course in cache before we test the problem. + bs_api.update_course_in_cache(self.course.id) @patch('lms.djangoapps.grades.events.tracker') def test_submit_answer(self, events_tracker): diff --git a/lms/djangoapps/grades/tests/test_transformer.py b/lms/djangoapps/grades/tests/test_transformer.py index b0cb6c0d49fb..7ef13e5b5b7c 100644 --- a/lms/djangoapps/grades/tests/test_transformer.py +++ b/lms/djangoapps/grades/tests/test_transformer.py @@ -13,6 +13,7 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import check_mongo_calls_range +import openedx.core.djangoapps.content.block_structure.api as bs_api from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.course_blocks.transformers.tests.helpers import CourseStructureTestCase @@ -462,6 +463,7 @@ def test_modulestore_performance(self, store_type, max_mongo_calls, min_mongo_ca ) with self.store.default_store(store_type): blocks = self.build_course(course) + bs_api.update_course_in_cache(blocks['course'].id) clear_course_from_cache(blocks['course'].id) with check_mongo_calls_range(max_mongo_calls, min_mongo_calls): get_course_blocks(self.student, blocks['course'].location, self.transformers) From f0f18f840579429916ba38ce5b71d42212d7928a Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 2 Aug 2024 13:24:20 -0400 Subject: [PATCH 10/16] refactor: Make the test easier to debug in the future. This test had a redundant call to get the course data from the store because that already happens at the end of the setup function. And also because expected call structure was being built inside the assert, it made it harder to inspect when debugging. Make the code a little bit easire to debug in case we're back here in the future. --- .../grades/tests/integration/test_events.py | 62 +++++++++---------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/lms/djangoapps/grades/tests/integration/test_events.py b/lms/djangoapps/grades/tests/integration/test_events.py index 52c1f14bacd8..315ffd65ef25 100644 --- a/lms/djangoapps/grades/tests/integration/test_events.py +++ b/lms/djangoapps/grades/tests/integration/test_events.py @@ -84,40 +84,38 @@ def setUp(self): @patch('lms.djangoapps.grades.events.tracker') def test_submit_answer(self, events_tracker): self.submit_question_answer('p1', {'2_1': 'choice_choice_2'}) - course = self.store.get_course(self.course.id, depth=0) event_transaction_id = events_tracker.emit.mock_calls[0][1][1]['event_transaction_id'] - events_tracker.emit.assert_has_calls( - [ - mock_call( - events.PROBLEM_SUBMITTED_EVENT_TYPE, - { - 'user_id': str(self.student.id), - 'event_transaction_id': event_transaction_id, - 'event_transaction_type': events.PROBLEM_SUBMITTED_EVENT_TYPE, - 'course_id': str(self.course.id), - 'problem_id': str(self.problem.location), - 'weighted_earned': 2.0, - 'weighted_possible': 2.0, - }, - ), - mock_call( - events.COURSE_GRADE_CALCULATED, - { - 'course_version': str(course.course_version), - 'percent_grade': 0.02, - 'grading_policy_hash': 'ChVp0lHGQGCevD0t4njna/C44zQ=', - 'user_id': str(self.student.id), - 'letter_grade': '', - 'event_transaction_id': event_transaction_id, - 'event_transaction_type': events.PROBLEM_SUBMITTED_EVENT_TYPE, - 'course_id': str(self.course.id), - 'course_edited_timestamp': str(course.subtree_edited_on), - } - ), - ], - any_order=True, - ) + expected_calls = [ + mock_call( + events.PROBLEM_SUBMITTED_EVENT_TYPE, + { + 'user_id': str(self.student.id), + 'event_transaction_id': event_transaction_id, + 'event_transaction_type': events.PROBLEM_SUBMITTED_EVENT_TYPE, + 'course_id': str(self.course.id), + 'problem_id': str(self.problem.location), + 'weighted_earned': 2.0, + 'weighted_possible': 2.0, + }, + ), + mock_call( + events.COURSE_GRADE_CALCULATED, + { + 'course_version': str(self.course.course_version), + 'percent_grade': 0.02, + 'grading_policy_hash': 'ChVp0lHGQGCevD0t4njna/C44zQ=', + 'user_id': str(self.student.id), + 'letter_grade': '', + 'event_transaction_id': event_transaction_id, + 'event_transaction_type': events.PROBLEM_SUBMITTED_EVENT_TYPE, + 'course_id': str(self.course.id), + 'course_edited_timestamp': str(self.course.subtree_edited_on), + } + ), + ] + + events_tracker.emit.assert_has_calls(expected_calls, any_order=True) @ddt.data(True, False) def test_delete_student_state(self, emit_signals): From 8741417806bc363ab3cc6bab48e9c08c35320bca Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 13 Aug 2024 14:44:22 -0400 Subject: [PATCH 11/16] docs: Cross link to wiki docs related to block structures. Provide a link to more docs from the code in case we need more context in the future. --- openedx/core/djangoapps/content/block_structure/__init__.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/openedx/core/djangoapps/content/block_structure/__init__.py b/openedx/core/djangoapps/content/block_structure/__init__.py index f77cf27f041f..00ed1b12d2ef 100644 --- a/openedx/core/djangoapps/content/block_structure/__init__.py +++ b/openedx/core/djangoapps/content/block_structure/__init__.py @@ -59,4 +59,10 @@ Note: A partial subset (as an ordered list) of the registered transformers can be requested during the Transform phase, allowing the client to manipulate exactly which transformers to call. + +Links to Other Block Structure Related Documentation: + + * https://openedx.atlassian.net/wiki/spaces/AC/pages/154861855/Block+Structure+Cache+Invalidation+Proposal + * https://openedx.atlassian.net/wiki/spaces/AC/pages/34734111/Course+Block+Transformers + * https://openedx.atlassian.net/wiki/spaces/AC/pages/41910826/Course+Blocks+API+Storage+Cache+Requirements """ From 80ed5ee2a45bcf5acf874b83913febf33f251785 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 20 Aug 2024 10:55:45 -0400 Subject: [PATCH 12/16] fix: Send the course publish signal on course import. --- cms/djangoapps/contentstore/management/commands/import.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/management/commands/import.py b/cms/djangoapps/contentstore/management/commands/import.py index 4ff9e7ce8d43..346f10d933b1 100644 --- a/cms/djangoapps/contentstore/management/commands/import.py +++ b/cms/djangoapps/contentstore/management/commands/import.py @@ -8,7 +8,7 @@ from openedx.core.djangoapps.django_comment_common.utils import are_permissions_roles_seeded, seed_permissions_roles from xmodule.contentstore.django import contentstore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore.django import SignalHandler, modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.xml_importer import import_course_from_xml # lint-amnesty, pylint: disable=wrong-import-order from xmodule.util.sandboxing import DEFAULT_PYTHON_LIB_FILENAME # lint-amnesty, pylint: disable=wrong-import-order @@ -73,6 +73,10 @@ def handle(self, *args, **options): for course in course_items: course_id = course.id + # Importing is an act of publishing so send the course published signal. + SignalHandler.course_published.send_robust(sender=self, course_key=course_id) + + # Seed forum permission roles if we need to. if not are_permissions_roles_seeded(course_id): self.stdout.write(f'Seeding forum roles for course {course_id}\n') seed_permissions_roles(course_id) From d58dea336c051d4a93e7ccf1a896a7fcbca874ee Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 23 Sep 2024 09:42:49 -0400 Subject: [PATCH 13/16] fixup! test: Update the course in the cache after it's got new content. --- .../blocks/transformers/tests/test_milestones.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 8c7d60e93831..5efea79c1986 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_milestones.py @@ -13,6 +13,7 @@ from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.course_blocks.transformers.tests.helpers import CourseStructureTestCase from lms.djangoapps.gating import api as lms_gating_api +import openedx.core.djangoapps.content.block_structure.api as bs_api from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers from openedx.core.djangoapps.course_apps.toggles import EXAMS_IDA from openedx.core.lib.gating import api as gating_api @@ -166,7 +167,11 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c self.course.enable_subsection_gating = True self.setup_gated_section(self.blocks[gated_block_ref], self.blocks[gating_block_ref]) - with self.assertNumQueries(14): + # Cache the course blocks so that they don't need to be generated when we're trying to + # get data back. This would happen as a part of publishing in a production system. + bs_api.update_course_in_cache(self.course.id) + + with self.assertNumQueries(4): self.get_blocks_and_check_against_expected(self.user, expected_blocks_before_completion) # clear the request cache to simulate a new request From a6e5b818e243a93161bae2abd334a58264948212 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 23 Sep 2024 09:57:42 -0400 Subject: [PATCH 14/16] fixup! test: Update the course in the cache after it's got new content. --- lms/djangoapps/instructor_task/tests/test_tasks_helper.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 428d6ddc2d6f..0a8c5e788bfc 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -25,6 +25,7 @@ from pytz import UTC import openedx.core.djangoapps.user_api.course_tag.api as course_tag_api +import openedx.core.djangoapps.content.block_structure.api as bs_api from xmodule.capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory # lint-amnesty, pylint: disable=wrong-import-order from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentAllowed @@ -396,6 +397,8 @@ def test_query_counts(self): ) _ = CreditCourseFactory(course_key=course.id) + bs_api.update_course_in_cache(course.id) + num_users = 5 for _ in range(num_users): user = UserFactory.create() @@ -406,7 +409,7 @@ def test_query_counts(self): with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): with check_mongo_calls(2): - with self.assertNumQueries(63): + with self.assertNumQueries(46): CourseGradeReport.generate(None, None, course.id, {}, 'graded') def test_inactive_enrollments(self): From 398160c2c4bc3146676a0703eb90e6513bfb4ee2 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 23 Sep 2024 13:28:39 -0400 Subject: [PATCH 15/16] fixup! test: Update the course in the cache after it's got new content. --- .../grades/rest_api/v1/tests/test_gradebook_views.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py b/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py index ab11daf36576..42dd9139ee21 100644 --- a/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py +++ b/lms/djangoapps/grades/rest_api/v1/tests/test_gradebook_views.py @@ -2230,9 +2230,6 @@ def test_get_override_for_unreleased_block(self): ) # We need to update the course in the cache after we create the new block. - # Review Question: Should we be doing this here? Does it make sense to do - # this in the xmodule/modulestore/tests/factories.py BlockFactory class - # as a part of the publish there? bs_api.update_course_in_cache(self.course_data.course_key) resp = self.client.get( From 4768568b5ad34fa93c760cd47964f6566acdda7a Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 24 Sep 2024 09:58:01 -0400 Subject: [PATCH 16/16] fixup! feat!: Drop the block_structure.storage_backing_for_cache waffle switch. --- openedx/core/djangoapps/content/block_structure/tasks.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/openedx/core/djangoapps/content/block_structure/tasks.py b/openedx/core/djangoapps/content/block_structure/tasks.py index 26b8ecad0759..5796b8167b2b 100644 --- a/openedx/core/djangoapps/content/block_structure/tasks.py +++ b/openedx/core/djangoapps/content/block_structure/tasks.py @@ -42,8 +42,6 @@ def update_course_in_cache_v2(self, **kwargs): Updates the course blocks (mongo -> BlockStructure) for the specified course. Keyword Arguments: course_id (string) - The string serialized value of the course key. - with_storage (boolean) - Whether or not storage backing should be - enabled for the generated block structure(s). """ _update_course_in_cache(self, **kwargs) @@ -71,8 +69,6 @@ def get_course_in_cache_v2(self, **kwargs): Gets the course blocks for the specified course, updating the cache if needed. Keyword Arguments: course_id (string) - The string serialized value of the course key. - with_storage (boolean) - Whether or not storage backing should be - enabled for any generated block structure(s). """ _get_course_in_cache(self, **kwargs)