From 29cdf311af7381d1e163892ac294b8c89064e703 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 25 Apr 2025 11:56:16 +0530 Subject: [PATCH 1/5] refactor: update sync model helper function docs Adds some comments to explain certain confusing sections --- cms/djangoapps/contentstore/models.py | 52 +++++++++++++-------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 24dc6748d2c7..1ee00c9412db 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -232,25 +232,25 @@ def update_or_create( 'version_declined': version_declined, } if upstream_block: - new_values.update( - { - 'upstream_block': upstream_block, - } - ) + new_values['upstream_block'] = upstream_block try: link = cls.objects.get(downstream_usage_key=downstream_usage_key) # TODO: until we save modified datetime for course xblocks in index, the modified time for links are updated - # everytime a downstream/course block is updated. This allows us to order links[1] based on recently - # modified downstream version. + # everytime a downstream/course block is updated. + # Hardcoding has_changes = True makes sure that the link modified time is updated whenever downstream block + # is updated and this allows us to sort upstream links in course libraries page by recently modified + # downstream blocks: [1]. # pylint: disable=line-too-long # 1. https://github.com/open-craft/frontend-app-course-authoring/blob/0443d88824095f6f65a3a64b77244af590d4edff/src/course-libraries/ReviewTabContent.tsx#L222-L233 - has_changes = True # change to false once above condition is met. - for key, value in new_values.items(): - prev = getattr(link, key) - # None != None is True, so we need to check for it specially - if prev != value and ~(prev is None and value is None): + # Ideally, in the future, whenever you modify a downstream XBlock, the code will change the ComponentLink + # "last modified at" time only if something relevant to the upstream sync was modified, like the upstream + # source was changed, or the sync was dismissed (skip the update for this particular version) or accepted. + has_changes = True # change to false once we save modified time in course xblocks + for key, new_value in new_values.items(): + prev_value = getattr(link, key) + if prev_value != new_value: has_changes = True - setattr(link, key, value) + setattr(link, key, new_value) if has_changes: link.updated = created link.save() @@ -317,25 +317,25 @@ def update_or_create( 'version_declined': version_declined, } if upstream_container: - new_values.update( - { - 'upstream_container': upstream_container, - } - ) + new_values['upstream_container'] = upstream_container try: link = cls.objects.get(downstream_usage_key=downstream_usage_key) # TODO: until we save modified datetime for course xblocks in index, the modified time for links are updated - # everytime a downstream/course block is updated. This allows us to order links[1] based on recently - # modified downstream version. + # everytime a downstream/course block is updated. + # Hardcoding has_changes = True makes sure that the link modified time is updated whenever downstream block + # is updated and this allows us to sort upstream links in course libraries page by recently modified + # downstream blocks: [1]. # pylint: disable=line-too-long # 1. https://github.com/open-craft/frontend-app-course-authoring/blob/0443d88824095f6f65a3a64b77244af590d4edff/src/course-libraries/ReviewTabContent.tsx#L222-L233 - has_changes = True # change to false once above condition is met. - for key, value in new_values.items(): - prev = getattr(link, key) - # None != None is True, so we need to check for it specially - if prev != value and ~(prev is None and value is None): + # Ideally, in the future, whenever you modify a downstream XBlock, the code will change the ComponentLink + # "last modified at" time only if something relevant to the upstream sync was modified, like the upstream + # source was changed, or the sync was dismissed (skip the update for this particular version) or accepted. + has_changes = True # change to false once we save modified time in course xblocks + for key, new_value in new_values.items(): + prev_value = getattr(link, key) + if prev_value != new_value: has_changes = True - setattr(link, key, value) + setattr(link, key, new_value) if has_changes: link.updated = created link.save() From d8de203a90c0880df34836ca85d2acbc2d6c8ae3 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 25 Apr 2025 11:56:52 +0530 Subject: [PATCH 2/5] refactor: sync library content method --- cms/djangoapps/contentstore/utils.py | 7 ++----- .../contentstore/xblock_storage_handlers/view_handlers.py | 4 ++-- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index ce244f616ddb..21270a188aba 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -2429,8 +2429,5 @@ def create_or_update_xblock_upstream_link(xblock, course_key: CourseKey, created _create_or_update_component_link(course_key, created, xblock) except InvalidKeyError: # It is possible that the upstream is a container and UsageKeyV2 parse failed - # Create upstream container link - try: - _create_or_update_container_link(course_key, created, xblock) - except InvalidKeyError: - log.error(f"Invalid key: {xblock.upstream}") + # Create upstream container link and raise InvalidKeyError if xblock.upstream is a valid key. + _create_or_update_container_link(course_key, created, xblock) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 54beb783f3b0..31a17466769d 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -548,8 +548,7 @@ def sync_library_content(downstream: XBlock, request, store) -> StaticFileNotice notices = [] # Store final children keys to update order of components in unit children = [] - for i in range(len(upstream_children)): - upstream_child = upstream_children[i] + for i, upstream_child in enumerate(upstream_children): assert isinstance(upstream_child, LibraryXBlockMetadata) # for now we only support units if upstream_child.usage_key not in downstream_children_keys: # This upstream_child is new, create it. @@ -574,6 +573,7 @@ def sync_library_content(downstream: XBlock, request, store) -> StaticFileNotice for child in downstream_children: if child.usage_key not in children: # This downstream block was added, or deleted from upstream block. + # NOTE: This will also delete any local additions to a unit in the next upstream sync. store.delete_item(child.usage_key, user_id=request.user.id) downstream.children = children store.update_item(downstream, request.user.id) From ae4ff4fe5cf86376992f45dbadcf024fa98f3afb Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 25 Apr 2025 19:59:32 +0530 Subject: [PATCH 3/5] feat: use edited_on block field --- cms/djangoapps/contentstore/models.py | 24 ++----------------- .../djangoapps/content/search/documents.py | 2 ++ 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 1ee00c9412db..26fdf75d29d2 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -235,17 +235,7 @@ def update_or_create( new_values['upstream_block'] = upstream_block try: link = cls.objects.get(downstream_usage_key=downstream_usage_key) - # TODO: until we save modified datetime for course xblocks in index, the modified time for links are updated - # everytime a downstream/course block is updated. - # Hardcoding has_changes = True makes sure that the link modified time is updated whenever downstream block - # is updated and this allows us to sort upstream links in course libraries page by recently modified - # downstream blocks: [1]. - # pylint: disable=line-too-long - # 1. https://github.com/open-craft/frontend-app-course-authoring/blob/0443d88824095f6f65a3a64b77244af590d4edff/src/course-libraries/ReviewTabContent.tsx#L222-L233 - # Ideally, in the future, whenever you modify a downstream XBlock, the code will change the ComponentLink - # "last modified at" time only if something relevant to the upstream sync was modified, like the upstream - # source was changed, or the sync was dismissed (skip the update for this particular version) or accepted. - has_changes = True # change to false once we save modified time in course xblocks + has_changes = False for key, new_value in new_values.items(): prev_value = getattr(link, key) if prev_value != new_value: @@ -320,17 +310,7 @@ def update_or_create( new_values['upstream_container'] = upstream_container try: link = cls.objects.get(downstream_usage_key=downstream_usage_key) - # TODO: until we save modified datetime for course xblocks in index, the modified time for links are updated - # everytime a downstream/course block is updated. - # Hardcoding has_changes = True makes sure that the link modified time is updated whenever downstream block - # is updated and this allows us to sort upstream links in course libraries page by recently modified - # downstream blocks: [1]. - # pylint: disable=line-too-long - # 1. https://github.com/open-craft/frontend-app-course-authoring/blob/0443d88824095f6f65a3a64b77244af590d4edff/src/course-libraries/ReviewTabContent.tsx#L222-L233 - # Ideally, in the future, whenever you modify a downstream XBlock, the code will change the ComponentLink - # "last modified at" time only if something relevant to the upstream sync was modified, like the upstream - # source was changed, or the sync was dismissed (skip the update for this particular version) or accepted. - has_changes = True # change to false once we save modified time in course xblocks + has_changes = False for key, new_value in new_values.items(): prev_value = getattr(link, key) if prev_value != new_value: diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 6a40f049bf96..134899a819c2 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -212,6 +212,8 @@ class implementation returns only: Fields.access_id: _meili_access_id_from_context_key(block.usage_key.context_key), Fields.breadcrumbs: [], } + if hasattr(block, "edited_on"): + block_data[Fields.modified] = block.edited_on.timestamp() # Get the breadcrumbs (course, section, subsection, etc.): if block.usage_key.context_key.is_course: # Getting parent is not yet implemented in Learning Core (for libraries). cur_block = block From 9053c8630a8d3a1ac208aeadda4c737db52b652b Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 25 Apr 2025 20:24:46 +0530 Subject: [PATCH 4/5] test: modified field in course block index --- .../content/search/tests/test_api.py | 36 +++++++++++-------- .../content/search/tests/test_documents.py | 35 +++++++++--------- .../content/search/tests/test_handlers.py | 15 +++++--- 3 files changed, 52 insertions(+), 34 deletions(-) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index ee70bd444721..46354d6eb3cd 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -61,19 +61,27 @@ def setUp(self): # Clear the Meilisearch client to avoid side effects from other tests api.clear_meilisearch_client() + modified_date = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) # Create course - self.course = self.store.create_course( - "org1", - "test_course", - "test_run", - self.user_id, - fields={"display_name": "Test Course"}, - ) - course_access, _ = SearchAccess.objects.get_or_create(context_key=self.course.id) - self.course_block_key = "block-v1:org1+test_course+test_run+type@course+block@course" - - # Create XBlocks - self.sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") + with freeze_time(modified_date): + self.course = self.store.create_course( + "org1", + "test_course", + "test_run", + self.user_id, + fields={"display_name": "Test Course"}, + ) + course_access, _ = SearchAccess.objects.get_or_create(context_key=self.course.id) + self.course_block_key = "block-v1:org1+test_course+test_run+type@course+block@course" + + # Create XBlocks + self.sequential = self.store.create_child( + self.user_id, + self.course.location, + "sequential", + "test_sequential" + ) + self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical") self.doc_sequential = { "id": "block-v1org1test_coursetest_runtypesequentialblocktest_sequential-f702c144", "type": "course_block", @@ -90,8 +98,8 @@ def setUp(self): ], "content": {}, "access_id": course_access.id, + "modified": modified_date.timestamp(), } - self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical") self.doc_vertical = { "id": "block-v1org1test_coursetest_runtypeverticalblocktest_vertical-e76a10a4", "type": "course_block", @@ -112,6 +120,7 @@ def setUp(self): ], "content": {}, "access_id": course_access.id, + "modified": modified_date.timestamp(), } # Make sure the CourseOverview for the course is created: CourseOverview.get_from_id(self.course.id) @@ -130,7 +139,6 @@ def setUp(self): self.problem1 = library_api.create_library_block(self.library.key, "problem", "p1") self.problem2 = library_api.create_library_block(self.library.key, "problem", "p2") # Update problem1, freezing the date so we can verify modified date serializes correctly. - modified_date = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) with freeze_time(modified_date): library_api.set_library_block_olx(self.problem1.usage_key, "") diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 0a5d871fbb96..b70594eff0eb 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -52,23 +52,23 @@ class StudioDocumentsTest(SharedModuleStoreTestCase): def setUpClass(cls): super().setUpClass() cls.store = modulestore() - cls.org = Organization.objects.create(name="edX", short_name="edX") - cls.toy_course = ToyCourseFactory.create() # See xmodule/modulestore/tests/sample_courses.py - cls.toy_course_key = cls.toy_course.id - - # Get references to some blocks in the toy course - cls.html_block_key = cls.toy_course_key.make_usage_key("html", "toyjumpto") - # Create a problem in course - cls.problem_block = BlockFactory.create( - category="problem", - parent_location=cls.toy_course_key.make_usage_key("vertical", "vertical_test"), - display_name='Test Problem', - data="What is a test?", - ) - # Create a library and collection with a block - created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) - with freeze_time(created_date): + cls.created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) + with freeze_time(cls.created_date): + # Get references to some blocks in the toy course + cls.org = Organization.objects.create(name="edX", short_name="edX") + cls.toy_course = ToyCourseFactory.create() # See xmodule/modulestore/tests/sample_courses.py + cls.toy_course_key = cls.toy_course.id + + cls.html_block_key = cls.toy_course_key.make_usage_key("html", "toyjumpto") + # Create a problem in course + cls.problem_block = BlockFactory.create( + category="problem", + parent_location=cls.toy_course_key.make_usage_key("vertical", "vertical_test"), + display_name='Test Problem', + data="What is a test?", + ) + cls.library = library_api.create_library( org=cls.org, slug="2012_Fall", @@ -190,6 +190,7 @@ def test_problem_block(self): 'usage_key': 'block-v1:edX+toy+2012_Fall+type@vertical+block@vertical_test', }, ], + "modified": self.created_date.timestamp(), "content": { "capa_content": "What is a test?", "problem_types": ["multiplechoiceresponse"], @@ -223,6 +224,7 @@ def test_html_block(self): "display_name": "Text", "description": "This is a link to another page and some Chinese 四節比分和七年前 Some " "more Chinese 四節比分和七年前 ", + "modified": self.created_date.timestamp(), "breadcrumbs": [ { 'display_name': 'Toy Course', @@ -276,6 +278,7 @@ def test_video_block_untagged(self): }, ], "content": {}, + "modified": self.created_date.timestamp(), # This video has no tags. } diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index 95b2ecb6f52a..33d0e4db8378 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -59,7 +59,9 @@ def test_create_delete_xblock(self, meilisearch_client): course_access, _ = SearchAccess.objects.get_or_create(context_key=course.id) # Create XBlocks - sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") + created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) + with freeze_time(created_date): + sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") doc_sequential = { "id": "block-v1orgatest_coursetest_runtypesequentialblocktest_sequential-0cdb9395", "type": "course_block", @@ -76,10 +78,11 @@ def test_create_delete_xblock(self, meilisearch_client): ], "content": {}, "access_id": course_access.id, - + "modified": created_date.timestamp(), } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential]) - vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") + with freeze_time(created_date): + vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") doc_vertical = { "id": "block-v1orgatest_coursetest_runtypeverticalblocktest_vertical-011f143b", "type": "course_block", @@ -100,6 +103,7 @@ def test_create_delete_xblock(self, meilisearch_client): ], "content": {}, "access_id": course_access.id, + "modified": created_date.timestamp(), } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_vertical]) @@ -107,11 +111,14 @@ def test_create_delete_xblock(self, meilisearch_client): # Update the XBlock sequential = self.store.get_item(sequential.location, self.user_id) # Refresh the XBlock sequential.display_name = "Updated Sequential" - self.store.update_item(sequential, self.user_id) + modified_date = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) + with freeze_time(modified_date): + self.store.update_item(sequential, self.user_id) # The display name and the child's breadcrumbs should be updated doc_sequential["display_name"] = "Updated Sequential" doc_vertical["breadcrumbs"][1]["display_name"] = "Updated Sequential" + doc_sequential["modified"] = modified_date.timestamp() meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([ doc_sequential, doc_vertical, From 237ba1c02bc497e4ee5d51dba414551137032586 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 29 Apr 2025 16:24:36 +0530 Subject: [PATCH 5/5] fix: extract uncommon methods to child class from base --- cms/djangoapps/contentstore/models.py | 127 ++++++++++++++++++++------ 1 file changed, 99 insertions(+), 28 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 26fdf75d29d2..9782a37057c1 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -106,6 +106,34 @@ class EntityLinkBase(models.Model): class Meta: abstract = True + +class ComponentLink(EntityLinkBase): + """ + This represents link between any two publishable entities or link between publishable entity and a course + XBlock. It helps in tracking relationship between XBlocks imported from libraries and used in different courses. + """ + upstream_block = models.ForeignKey( + Component, + on_delete=models.SET_NULL, + related_name="links", + null=True, + blank=True, + ) + upstream_usage_key = UsageKeyField( + max_length=255, + help_text=_( + "Upstream block usage key, this value cannot be null" + " and useful to track upstream library blocks that do not exist yet" + ) + ) + + class Meta: + verbose_name = _("Component Link") + verbose_name_plural = _("Component Links") + + def __str__(self): + return f"ComponentLink<{self.upstream_usage_key}->{self.downstream_usage_key}>" + @property def upstream_version_num(self) -> int | None: """ @@ -177,34 +205,6 @@ def summarize_by_downstream_context(cls, downstream_context_key: CourseKey) -> Q ) return result - -class ComponentLink(EntityLinkBase): - """ - This represents link between any two publishable entities or link between publishable entity and a course - XBlock. It helps in tracking relationship between XBlocks imported from libraries and used in different courses. - """ - upstream_block = models.ForeignKey( - Component, - on_delete=models.SET_NULL, - related_name="links", - null=True, - blank=True, - ) - upstream_usage_key = UsageKeyField( - max_length=255, - help_text=_( - "Upstream block usage key, this value cannot be null" - " and useful to track upstream library blocks that do not exist yet" - ) - ) - - class Meta: - verbose_name = _("Component Link") - verbose_name_plural = _("Component Links") - - def __str__(self): - return f"ComponentLink<{self.upstream_usage_key}->{self.downstream_usage_key}>" - @classmethod def update_or_create( cls, @@ -280,6 +280,77 @@ class Meta: def __str__(self): return f"ContainerLink<{self.upstream_container_key}->{self.downstream_usage_key}>" + @property + def upstream_version_num(self) -> int | None: + """ + Returns upstream container version number if available. + """ + published_version = get_published_version(self.upstream_container.publishable_entity.id) + return published_version.version_num if published_version else None + + @property + def upstream_context_title(self) -> str: + """ + Returns upstream context title. + """ + return self.upstream_container.publishable_entity.learning_package.title + + @classmethod + def filter_links( + cls, + **link_filter, + ) -> QuerySet["EntityLinkBase"]: + """ + Get all links along with sync flag, upstream context title and version, with optional filtering. + """ + ready_to_sync = link_filter.pop('ready_to_sync', None) + result = cls.objects.filter(**link_filter).select_related( + "upstream_container__publishable_entity__published__version", + "upstream_container__publishable_entity__learning_package" + ).annotate( + ready_to_sync=( + GreaterThan( + Coalesce("upstream_container__publishable_entity__published__version__version_num", 0), + Coalesce("version_synced", 0) + ) & GreaterThan( + Coalesce("upstream_container__publishable_entity__published__version__version_num", 0), + Coalesce("version_declined", 0) + ) + ) + ) + if ready_to_sync is not None: + result = result.filter(ready_to_sync=ready_to_sync) + return result + + @classmethod + def summarize_by_downstream_context(cls, downstream_context_key: CourseKey) -> QuerySet: + """ + Returns a summary of links by upstream context for given downstream_context_key. + Example: + [ + { + "upstream_context_title": "CS problems 3", + "upstream_context_key": "lib:OpenedX:CSPROB3", + "ready_to_sync_count": 11, + "total_count": 14 + }, + { + "upstream_context_title": "CS problems 2", + "upstream_context_key": "lib:OpenedX:CSPROB2", + "ready_to_sync_count": 15, + "total_count": 24 + }, + ] + """ + result = cls.filter_links(downstream_context_key=downstream_context_key).values( + "upstream_context_key", + upstream_context_title=F("upstream_container__publishable_entity__learning_package__title"), + ).annotate( + ready_to_sync_count=Count("id", Q(ready_to_sync=True)), + total_count=Count('id') + ) + return result + @classmethod def update_or_create( cls,