Skip to content

Commit

Permalink
In importcontent, use a set for node_ids
Browse files Browse the repository at this point in the history 
This makes the code easier to test, and better reflects that the order
of node_ids is not significant. Related code in content_manifest already
deals with node_ids as a set.
  • Loading branch information
@dylanmccall
dylanmccall committed Sep 7, 2022
1 parent f47a71b commit 1ca7065
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 45 deletions.
10 changes: 8 additions & 2 deletions kolibri/core/content/management/commands/importcontent.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,12 @@ def _transfer( # noqa: max-complexity=16
timeout=transfer.Transfer.DEFAULT_TIMEOUT,
content_dir=None,
):
if node_ids is not None:
node_ids = set(node_ids)

if exclude_node_ids is not None:
exclude_node_ids = set(exclude_node_ids)

if manifest_file and not path:
# If manifest_file is stdin, its name will be "<stdin>" and path
# will become "". This feels clumsy, but the resulting behaviour
Expand Down Expand Up @@ -688,7 +694,7 @@ def handle_async(self, *args, **options):


def _node_ids_from_content_manifest(content_manifest, channel_id):
node_ids = []
node_ids = set()

channel_metadata = ChannelMetadata.objects.get(id=channel_id)

Expand All @@ -701,7 +707,7 @@ def _node_ids_from_content_manifest(content_manifest, channel_id):
local_version=channel_metadata.version,
)
)
node_ids.extend(
node_ids.update(
content_manifest.get_include_node_ids(channel_id, channel_version)
)

Expand Down
83 changes: 40 additions & 43 deletions kolibri/core/content/test/test_import_export.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,10 @@ def test_with_node_ids(self):
matched_nodes_queries_list = get_import_export_nodes(
self.the_channel_id,
renderable_only=False,
node_ids=[
node_ids={
self.c2_node_id,
self.c1_node_id,
],
},
)

self.assertCountEqual(
Expand All @@ -167,11 +167,11 @@ def test_with_node_ids_and_exclude_node_ids(self):
matched_nodes_queries_list = get_import_export_nodes(
self.the_channel_id,
renderable_only=False,
node_ids=[
node_ids={
self.c2_node_id,
self.c1_node_id,
],
exclude_node_ids=[self.c2c3_node_id],
},
exclude_node_ids={self.c2c3_node_id},
)

self.assertCountEqual(
Expand All @@ -185,8 +185,8 @@ def test_with_node_ids_equals_exclude_node_ids(self):
matched_nodes_queries_list = get_import_export_nodes(
self.the_channel_id,
renderable_only=False,
node_ids=[self.c1_node_id],
exclude_node_ids=[self.c1_node_id],
node_ids={self.c1_node_id},
exclude_node_ids={self.c1_node_id},
)

self.assertCountEqual(
Expand Down Expand Up @@ -220,7 +220,7 @@ def test_with_node_ids_empty(self):
matched_nodes_queries_list = get_import_export_nodes(
self.the_channel_id,
renderable_only=False,
node_ids=[],
node_ids=set(),
exclude_node_ids=None,
)

Expand Down Expand Up @@ -533,6 +533,7 @@ class ImportContentTestCase(TestCase):
the_channel_id = "6199dde695db4ee4ab392222d5af1e5c"
the_channel_version = 0

c1_node_id = "32a941fb77c2576e8f6b294cde4c3b0c"
c2c1_node_id = "2b6926ed22025518a8b9da91745b51d3"
c2c2_node_id = "4d0c890de9b65d6880ccfa527800e0f4"

Expand Down Expand Up @@ -790,7 +791,7 @@ def test_remote_cancel_during_connect_error(
"importcontent",
"network",
self.the_channel_id,
node_ids=["32a941fb77c2576e8f6b294cde4c3b0c"],
node_ids=[self.c1_node_id],
)
cancel_mock.assert_called_with()
annotation_mock.set_content_visibility.assert_called()
Expand Down Expand Up @@ -832,20 +833,19 @@ def test_remote_import_httperror_404(
10,
)

node_id = [self.c2c1_node_id]
call_command(
"importcontent",
"network",
self.the_channel_id,
node_ids=node_id,
node_ids=[self.c2c1_node_id],
renderable_only=False,
)
logger_mock.assert_called_once()
self.assertTrue("3 files are skipped" in logger_mock.call_args_list[0][0][0])
annotation_mock.set_content_visibility.assert_called_with(
self.the_channel_id,
[],
node_ids=node_id,
node_ids={self.c2c1_node_id},
exclude_node_ids=None,
public=False,
)
Expand Down Expand Up @@ -1060,7 +1060,7 @@ def test_remote_import_chunkedencodingerror(
"importcontent",
"network",
self.the_channel_id,
node_ids=["32a941fb77c2576e8f6b294cde4c3b0c"],
node_ids=[self.c1_node_id],
)
cancel_mock.assert_called_with()
annotation_mock.set_content_visibility.assert_called()
Expand Down Expand Up @@ -1148,16 +1148,12 @@ def test_local_import_source_corrupted(
fd2, local_src_path = tempfile.mkstemp()
os.close(fd1)
os.close(fd2)
LocalFile.objects.filter(
files__contentnode="32a941fb77c2576e8f6b294cde4c3b0c"
).update(file_size=1)
LocalFile.objects.filter(files__contentnode=self.c1_node_id).update(file_size=1)
path_mock.side_effect = [local_dest_path, local_src_path]
get_import_export_mock.return_value = (
1,
[
LocalFile.objects.filter(
files__contentnode="32a941fb77c2576e8f6b294cde4c3b0c"
)
LocalFile.objects.filter(files__contentnode=self.c1_node_id)
.values("id", "file_size", "extension")
.first()
],
Expand All @@ -1168,7 +1164,7 @@ def test_local_import_source_corrupted(
"disk",
self.the_channel_id,
"destination",
node_ids=["32a941fb77c2576e8f6b294cde4c3b0c"],
node_ids=[self.c1_node_id],
)
remove_mock.assert_any_call(local_dest_path)

Expand Down Expand Up @@ -1210,20 +1206,18 @@ def test_local_import_source_corrupted_full_progress(
os.close(fd)
os.remove(local_dest_path)
# Delete all but one file associated with ContentNode to reduce need for mocking
files = ContentNode.objects.get(
id="32a941fb77c2576e8f6b294cde4c3b0c"
).files.all()
files = ContentNode.objects.get(id=self.c1_node_id).files.all()
first_file = files.first()
files.exclude(id=first_file.id).delete()
LocalFile.objects.filter(
files__contentnode="32a941fb77c2576e8f6b294cde4c3b0c"
).update(file_size=expected_file_size)
LocalFile.objects.filter(files__contentnode=self.c1_node_id).update(
file_size=expected_file_size
)
get_import_export_mock.return_value = (
1,
list(
LocalFile.objects.filter(
files__contentnode="32a941fb77c2576e8f6b294cde4c3b0c"
).values("id", "file_size", "extension")
LocalFile.objects.filter(files__contentnode=self.c1_node_id).values(
"id", "file_size", "extension"
)
),
10,
)
Expand All @@ -1238,7 +1232,7 @@ def test_local_import_source_corrupted_full_progress(
"disk",
self.the_channel_id,
"destination",
node_ids=["32a941fb77c2576e8f6b294cde4c3b0c"],
node_ids=[self.c1_node_id],
)

mock_overall_progress.assert_any_call(expected_file_size)
Expand Down Expand Up @@ -1289,13 +1283,13 @@ def test_remote_import_source_corrupted(
"importcontent",
"network",
self.the_channel_id,
node_ids=["32a941fb77c2576e8f6b294cde4c3b0c"],
node_ids=[self.c1_node_id],
)
annotation_mock.set_content_visibility.assert_called_with(
self.the_channel_id,
[],
exclude_node_ids=None,
node_ids=["32a941fb77c2576e8f6b294cde4c3b0c"],
node_ids={self.c1_node_id},
public=False,
)

Expand Down Expand Up @@ -1385,7 +1379,7 @@ def test_local_import_with_detected_manifest_file(
# according to channel_id in the detected manifest file.
get_import_export_mock.assert_called_once_with(
self.the_channel_id,
[six.text_type(self.c2c1_node_id)],
{six.text_type(self.c2c1_node_id)},
None,
False,
renderable_only=True,
Expand Down Expand Up @@ -1422,7 +1416,7 @@ def test_local_import_with_detected_manifest_file_and_unlisted_channel(
# of node_ids.
get_import_export_mock.assert_called_once_with(
self.the_channel_id,
[],
set(),
None,
False,
renderable_only=True,
Expand Down Expand Up @@ -1543,7 +1537,7 @@ def test_local_import_with_local_manifest_file_with_multiple_versions(
# list of node_ids built from all versions of the channel_id channel.
get_import_export_mock.assert_called_once_with(
self.the_channel_id,
[six.text_type(self.c2c1_node_id), six.text_type(self.c2c2_node_id)],
{six.text_type(self.c2c1_node_id), six.text_type(self.c2c2_node_id)},
None,
False,
renderable_only=True,
Expand Down Expand Up @@ -1591,7 +1585,7 @@ def test_local_import_with_detected_manifest_file_and_node_ids(
# of node_ids, ignoring the detected manifest file.
get_import_export_mock.assert_called_once_with(
self.the_channel_id,
[six.text_type(self.c2c2_node_id)],
{six.text_type(self.c2c2_node_id)},
None,
False,
renderable_only=True,
Expand All @@ -1602,15 +1596,19 @@ def test_local_import_with_detected_manifest_file_and_node_ids(
get_import_export_mock.reset_mock()

call_command(
"importcontent", "disk", self.the_channel_id, import_source_dir, node_ids=[]
"importcontent",
"disk",
self.the_channel_id,
import_source_dir,
node_ids=[],
)

# If a manifest file is present in the source directory but node_ids is set to
# an empty (falsey) list, importcontent should call get_import_export with that
# empty list of node_ids, ignoring the detected manifest file.
get_import_export_mock.assert_called_once_with(
self.the_channel_id,
[],
set(),
None,
False,
renderable_only=True,
Expand Down Expand Up @@ -1671,7 +1669,7 @@ def test_local_import_with_detected_manifest_file_and_manifest_file(
# node_ids according to channel_id in the provided manifest file.
get_import_export_mock.assert_called_once_with(
self.the_channel_id,
[six.text_type(self.c2c2_node_id)],
{six.text_type(self.c2c2_node_id)},
None,
False,
renderable_only=True,
Expand Down Expand Up @@ -1774,7 +1772,7 @@ def test_remote_import_with_local_manifest_file(
# channel_id in the provided manifest file.
get_import_export_mock.assert_called_once_with(
self.the_channel_id,
[six.text_type(self.c2c1_node_id)],
{six.text_type(self.c2c1_node_id)},
None,
False,
renderable_only=True,
Expand Down Expand Up @@ -1877,20 +1875,19 @@ def test_remote_import_fail_on_error(
10,
)

node_id = [self.c2c1_node_id]
with self.assertRaises(HTTPError):
call_command(
"importcontent",
"network",
self.the_channel_id,
node_ids=node_id,
node_ids=[self.c2c1_node_id],
renderable_only=False,
fail_on_error=True,
)
annotation_mock.set_content_visibility.assert_called_with(
self.the_channel_id,
[],
node_ids=node_id,
node_ids={self.c2c1_node_id},
exclude_node_ids=None,
public=False,
)
Expand Down

0 comments on commit 1ca7065

Please sign in to comment.