refactor: update sync model helper function docs and minor cleanup (#36599)

* refactor: update sync model helper function docs

Adds some comments to explain certain confusing sections

* refactor: sync library content method

* feat: use edited_on block field

* test: modified field in course block index

* fix: extract uncommon methods to child class from base

Author: navinkarkera

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,
@@ -232,25 +232,15 @@ 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.
-            # 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):
+            has_changes = False
+            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()
@@ -290,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,
@@ -317,25 +378,15 @@ 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.
-            # 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):
+            has_changes = False
+            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()

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)

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)

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

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, "<problem />")
 

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="<problem>What is a test?<multiplechoiceresponse></multiplechoiceresponse></problem>",
-        )
-
         # 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="<problem>What is a test?<multiplechoiceresponse></multiplechoiceresponse></problem>",
+            )
+
             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.
         }