fix: mark container as ready to sync if any child block is deleted
Before this fix, if only a component is deleted, then its parent blocks were not marked as ready to sync.
This commit is contained in:
committed by
Navin Karkera
parent
bbd53a0399
commit
f2f0d87e9e
@@ -7,12 +7,12 @@ from itertools import chain
|
||||
|
||||
from config_models.models import ConfigurationModel
|
||||
from django.db import models
|
||||
from django.db.models import QuerySet, OuterRef, Case, When, Exists, Value, ExpressionWrapper
|
||||
from django.db.models.fields import IntegerField, TextField, BooleanField
|
||||
from django.db.models import Case, Exists, ExpressionWrapper, OuterRef, Q, QuerySet, Value, When
|
||||
from django.db.models.fields import BooleanField, IntegerField, TextField
|
||||
from django.db.models.functions import Coalesce
|
||||
from django.db.models.lookups import GreaterThan
|
||||
from django.utils.translation import gettext_lazy as _
|
||||
from opaque_keys.edx.django.models import CourseKeyField, ContainerKeyField, UsageKeyField
|
||||
from opaque_keys.edx.django.models import ContainerKeyField, CourseKeyField, UsageKeyField
|
||||
from opaque_keys.edx.keys import CourseKey, UsageKey
|
||||
from opaque_keys.edx.locator import LibraryContainerLocator
|
||||
from openedx_learning.api.authoring import get_published_version
|
||||
@@ -23,7 +23,6 @@ from openedx_learning.lib.fields import (
|
||||
manual_date_time_field,
|
||||
)
|
||||
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
@@ -391,7 +390,7 @@ class ContainerLink(EntityLinkBase):
|
||||
cls.objects.filter(**link_filter).select_related(*RELATED_FIELDS),
|
||||
)
|
||||
if ready_to_sync is not None:
|
||||
result = result.filter(ready_to_sync=ready_to_sync)
|
||||
result = result.filter(Q(ready_to_sync=ready_to_sync) | Q(ready_to_sync_from_children=ready_to_sync))
|
||||
|
||||
# Handle top-level parents logic
|
||||
if use_top_level_parents:
|
||||
@@ -436,6 +435,11 @@ class ContainerLink(EntityLinkBase):
|
||||
),
|
||||
then=1
|
||||
),
|
||||
# If upstream block was deleted, set ready_to_sync = True
|
||||
When(
|
||||
Q(upstream_container__publishable_entity__published__version__version_num__isnull=True),
|
||||
then=1
|
||||
),
|
||||
default=0,
|
||||
output_field=models.IntegerField()
|
||||
)
|
||||
@@ -457,6 +461,11 @@ class ContainerLink(EntityLinkBase):
|
||||
),
|
||||
then=1
|
||||
),
|
||||
# If upstream block was deleted, set ready_to_sync = True
|
||||
When(
|
||||
Q(upstream_block__publishable_entity__published__version__version_num__isnull=True),
|
||||
then=1
|
||||
),
|
||||
default=0,
|
||||
output_field=models.IntegerField()
|
||||
)
|
||||
|
||||
@@ -23,7 +23,7 @@ from common.djangoapps.student.roles import CourseStaffRole
|
||||
from common.djangoapps.student.tests.factories import UserFactory
|
||||
from openedx.core.djangoapps.content_libraries import api as lib_api
|
||||
from xmodule.modulestore.django import modulestore
|
||||
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase, ImmediateOnCommitMixin
|
||||
from xmodule.modulestore.tests.django_utils import ImmediateOnCommitMixin, SharedModuleStoreTestCase
|
||||
from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory
|
||||
|
||||
from .. import downstreams as downstreams_views
|
||||
@@ -32,6 +32,7 @@ MOCK_UPSTREAM_ERROR = "your LibraryGPT subscription has expired"
|
||||
URL_PREFIX = '/api/libraries/v2/'
|
||||
URL_LIB_CREATE = URL_PREFIX
|
||||
URL_LIB_BLOCKS = URL_PREFIX + '{lib_key}/blocks/'
|
||||
URL_LIB_BLOCK = URL_PREFIX + 'blocks/{block_key}/'
|
||||
URL_LIB_BLOCK_PUBLISH = URL_PREFIX + 'blocks/{block_key}/publish/'
|
||||
URL_LIB_BLOCK_OLX = URL_PREFIX + 'blocks/{block_key}/olx/'
|
||||
URL_LIB_CONTAINER = URL_PREFIX + 'containers/{container_key}/' # Get a container in this library
|
||||
@@ -277,6 +278,10 @@ class _BaseDownstreamViewTestMixin:
|
||||
data["slug"] = slug
|
||||
return self._api('post', URL_LIB_CONTAINERS.format(lib_key=lib_key), data, expect_response)
|
||||
|
||||
def _delete_component(self, block_key, expect_response=200):
|
||||
""" Publish all changes in the specified container + children """
|
||||
return self._api('delete', URL_LIB_BLOCK.format(block_key=block_key), None, expect_response)
|
||||
|
||||
|
||||
class SharedErrorTestCases(_BaseDownstreamViewTestMixin):
|
||||
"""
|
||||
@@ -1503,3 +1508,109 @@ class GetDownstreamSummaryViewTest(
|
||||
'last_published_at': self.now.strftime('%Y-%m-%dT%H:%M:%S.%fZ'),
|
||||
}]
|
||||
self.assertListEqual(data, expected)
|
||||
|
||||
|
||||
class GetDownstreamDeletedUpstream(
|
||||
_BaseDownstreamViewTestMixin,
|
||||
ImmediateOnCommitMixin,
|
||||
SharedModuleStoreTestCase,
|
||||
):
|
||||
"""
|
||||
Test that parent container is marked ready_to_sync when even when the only change is a deleted component under it
|
||||
"""
|
||||
def call_api(
|
||||
self,
|
||||
course_id: str | None = None,
|
||||
ready_to_sync: bool | None = None,
|
||||
upstream_key: str | None = None,
|
||||
item_type: str | None = None,
|
||||
use_top_level_parents: bool | None = None,
|
||||
):
|
||||
data = {}
|
||||
if course_id is not None:
|
||||
data["course_id"] = str(course_id)
|
||||
if ready_to_sync is not None:
|
||||
data["ready_to_sync"] = str(ready_to_sync)
|
||||
if upstream_key is not None:
|
||||
data["upstream_key"] = str(upstream_key)
|
||||
if item_type is not None:
|
||||
data["item_type"] = str(item_type)
|
||||
if use_top_level_parents is not None:
|
||||
data["use_top_level_parents"] = str(use_top_level_parents)
|
||||
return self.client.get("/api/contentstore/v2/downstreams/", data=data)
|
||||
|
||||
def test_delete_component_should_be_ready_to_sync(self):
|
||||
"""
|
||||
Test deleting a component from library should mark the entire section container ready to sync
|
||||
"""
|
||||
# Create blocks
|
||||
section_id = self._create_container(self.library_id, "section", "section-12", "Section 12")["id"]
|
||||
subsection_id = self._create_container(self.library_id, "subsection", "subsection-12", "Subsection 12")["id"]
|
||||
unit_id = self._create_container(self.library_id, "unit", "unit-12", "Unit 12")["id"]
|
||||
video_id = self._add_block_to_library(self.library_id, "video", "video-bar-13")["id"]
|
||||
section_key = ContainerKey.from_string(section_id)
|
||||
subsection_key = ContainerKey.from_string(subsection_id)
|
||||
unit_key = ContainerKey.from_string(unit_id)
|
||||
video_key = LibraryUsageLocatorV2.from_string(video_id)
|
||||
|
||||
# Set children
|
||||
lib_api.update_container_children(section_key, [subsection_key], None)
|
||||
lib_api.update_container_children(subsection_key, [unit_key], None)
|
||||
lib_api.update_container_children(unit_key, [video_key], None)
|
||||
self._publish_container(unit_id)
|
||||
self._publish_container(subsection_id)
|
||||
self._publish_container(section_id)
|
||||
self._publish_library_block(video_id)
|
||||
course = CourseFactory.create(display_name="Course New")
|
||||
add_users(self.superuser, CourseStaffRole(course.id), self.course_user)
|
||||
chapter = BlockFactory.create(
|
||||
category='chapter', parent=course, upstream=section_id, upstream_version=2,
|
||||
)
|
||||
sequential = BlockFactory.create(
|
||||
category='sequential',
|
||||
parent=chapter,
|
||||
upstream=subsection_id,
|
||||
upstream_version=2,
|
||||
top_level_downstream_parent_key=get_block_key_string(chapter.usage_key),
|
||||
)
|
||||
vertical = BlockFactory.create(
|
||||
category='vertical',
|
||||
parent=sequential,
|
||||
upstream=unit_id,
|
||||
upstream_version=2,
|
||||
top_level_downstream_parent_key=get_block_key_string(chapter.usage_key),
|
||||
)
|
||||
BlockFactory.create(
|
||||
category='video',
|
||||
parent=vertical,
|
||||
upstream=video_id,
|
||||
upstream_version=1,
|
||||
top_level_downstream_parent_key=get_block_key_string(chapter.usage_key),
|
||||
)
|
||||
self._delete_component(video_id)
|
||||
self._publish_container(unit_id)
|
||||
response = self.call_api(course_id=course.id, ready_to_sync=True, use_top_level_parents=True)
|
||||
assert response.status_code == 200
|
||||
data = response.json()['results']
|
||||
assert len(data) == 1
|
||||
date_format = self.now.isoformat().split("+")[0] + 'Z'
|
||||
expected_results = {
|
||||
'created': date_format,
|
||||
'downstream_context_key': str(course.id),
|
||||
'downstream_usage_key': str(chapter.usage_key),
|
||||
'downstream_customized': [],
|
||||
'id': 8,
|
||||
'ready_to_sync': False,
|
||||
'ready_to_sync_from_children': True,
|
||||
'top_level_parent_usage_key': None,
|
||||
'updated': date_format,
|
||||
'upstream_context_key': self.library_id,
|
||||
'upstream_context_title': self.library_title,
|
||||
'upstream_key': section_id,
|
||||
'upstream_type': 'container',
|
||||
'upstream_version': 2,
|
||||
'version_declined': None,
|
||||
'version_synced': 2,
|
||||
}
|
||||
|
||||
self.assertDictEqual(data[0], expected_results)
|
||||
|
||||
@@ -87,6 +87,13 @@ class UpstreamLink:
|
||||
downstream_customized: list[str] | None # List of fields modified in downstream
|
||||
has_top_level_parent: bool # True if this Upstream link has a top-level parent
|
||||
|
||||
@property
|
||||
def is_upstream_deleted(self) -> bool:
|
||||
return bool(
|
||||
self.upstream_ref and
|
||||
self.version_available is None
|
||||
)
|
||||
|
||||
@property
|
||||
def is_ready_to_sync_individually(self) -> bool:
|
||||
return bool(
|
||||
@@ -94,7 +101,7 @@ class UpstreamLink:
|
||||
self.version_available and
|
||||
self.version_available > (self.version_synced or 0) and
|
||||
self.version_available > (self.version_declined or 0)
|
||||
)
|
||||
) or self.is_upstream_deleted
|
||||
|
||||
def _check_children_ready_to_sync(self, xblock_downstream: XBlock, return_fast: bool) -> list[dict[str, str]]:
|
||||
"""
|
||||
|
||||
Reference in New Issue
Block a user