Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Issue when delete all tags on import [FC-0036] #135

Merged
61 changes: 17 additions & 44 deletions openedx_tagging/core/tagging/import_export/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,11 @@ def _get_tag(self) -> Tag:
Returns the respective tag of this actions
"""
if self.tag.id:
return self.taxonomy.tag_set.get(external_id=self.tag.id)
return self.taxonomy.tag_set.get(value=self.tag.value)
try:
return self.taxonomy.tag_set.get(external_id=self.tag.id)
except Tag.DoesNotExist:
pass
return self.taxonomy.tag_set.get(value=self.tag.value, external_id=None)

def _search_action(
self,
Expand Down Expand Up @@ -259,35 +262,14 @@ class UpdateParentTag(ImportAction):
def __str__(self) -> str:
taxonomy_tag = self._get_tag()

if taxonomy_tag.external_id:
prefix_str = _("Update the parent of tag (external_id={external_id})").format(
external_id=taxonomy_tag.external_id
)
else:
prefix_str = ""
prefix_str = _("Update the parent of tag (value={value})").format(
value=taxonomy_tag.value
)

if not taxonomy_tag.parent:
from_str = _("from empty parent")
else:
if taxonomy_tag.parent.external_id:
from_str = _("from parent (external_id={external_id})").format(
external_id=taxonomy_tag.parent.external_id
)
else:
from_str = _("from parent (value={value})").format(
value=taxonomy_tag.parent.value
)

return str(
_(
"{prefix_str} "
"{from_str} to parent (external_id={parent_id})."
).format(prefix_str=prefix_str, from_str=from_str, parent_id=self.tag.parent_id)
description_str = _("Update the parent of {tag} from parent {old_parent} to {new_parent}").format(
tag=taxonomy_tag,
old_parent=taxonomy_tag.parent,
new_parent=self.tag.parent_id,
)

return str(description_str)

@classmethod
def applies_for(cls, taxonomy: Taxonomy, tag) -> bool:
"""
Expand Down Expand Up @@ -343,20 +325,13 @@ class RenameTag(ImportAction):

def __str__(self) -> str:
taxonomy_tag = self._get_tag()
if taxonomy_tag.external_id:
prefix_str = _("Rename tag value of tag (external_id={external_id})").format(
external_id=taxonomy_tag.external_id
)
else:
prefix_str = _("Rename tag value of tag (id={id})").format(id=taxonomy_tag.id)

return str(
_(
"{prefix_str} "
"from '{from_value}' to '{to_value}'"
).format(prefix_str=prefix_str, from_value=taxonomy_tag.value, to_value=self.tag.value)
description_str = _("Rename tag value of {tag} to '{new_value}'").format(
tag=taxonomy_tag,
new_value=self.tag.value,
)

return str(description_str)

@classmethod
def applies_for(cls, taxonomy: Taxonomy, tag) -> bool:
"""
Expand Down Expand Up @@ -442,9 +417,7 @@ class WithoutChanges(ImportAction):
name = "without_changes"

def __str__(self) -> str:
if self.tag.id:
return str(_("No changes needed for tag (external_id={external_id})").format(external_id=self.tag.id))
return str(_("No changes needed for tag (value={value})").format(value=self.tag.value))
return str(_("No changes needed for {tag}").format(tag=self.tag))

@classmethod
def applies_for(cls, taxonomy: Taxonomy, tag) -> bool:
Expand Down
8 changes: 8 additions & 0 deletions openedx_tagging/core/tagging/import_export/import_plan.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ class TagItem:
index: int | None = 0
parent_id: str | None = None

def __str__(self):
"""
User-facing string representation of a Tag.
"""
if self.id:
return f"<{self.__class__.__name__}> ({self.id} / {self.value})"
return f"<{self.__class__.__name__}> ({self.value})"


class TagImportPlan:
"""
Expand Down
4 changes: 3 additions & 1 deletion openedx_tagging/core/tagging/models/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@ def __str__(self):
"""
User-facing string representation of a Tag.
"""
return f"<{self.__class__.__name__}> ({self.id}) {self.value}"
if self.external_id:
return f"<{self.__class__.__name__}> ({self.id} / {self.external_id} / {self.value})"
return f"<{self.__class__.__name__}> ({self.id} / {self.value})"
bradenmacdonald marked this conversation as resolved.
Show resolved Hide resolved

def get_lineage(self) -> Lineage:
"""
Expand Down
12 changes: 6 additions & 6 deletions tests/openedx_tagging/core/tagging/import_export/test_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,16 +283,16 @@ class TestUpdateParentTag(TestImportActionMixin, TestCase):
"tag_4",
"tag_3",
(
"Update the parent of tag (external_id=tag_4) from parent "
"(external_id=tag_3) to parent (external_id=tag_3)."
"Update the parent of <Tag> (29 / tag_4 / Tag 4) "
"from parent <Tag> (28 / tag_3 / Tag 3) to tag_3"
)
),
(
"tag_3",
"tag_2",
(
"Update the parent of tag (external_id=tag_3) from empty parent "
"to parent (external_id=tag_2)."
"Update the parent of <Tag> (28 / tag_3 / Tag 3) "
"from parent None to tag_2"
)
),
)
Expand Down Expand Up @@ -333,8 +333,8 @@ def test_str_no_external_id(self):
index=100,
)
expected = (
"Update the parent of tag (value=Tag 6) from parent (value=Tag 5)"
" to parent (external_id=tag_3)."
"Update the parent of <Tag> (31 / Tag 6) "
"from parent <Tag> (30 / Tag 5) to tag_3"
)
assert str(action) == expected

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,13 +251,13 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions
"--------------------------------\n"
"#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n"
"#2: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n"
"#3: Rename tag value of tag (external_id=tag_1) from 'Tag 1' to 'Tag 2'\n"
"#4: Update the parent of tag (external_id=tag_4) from parent (external_id=tag_3) "
"to parent (external_id=tag_100).\n"
"#3: Rename tag value of <Tag> (26 / tag_1 / Tag 1) to 'Tag 2'\n"
"#4: Update the parent of <Tag> (29 / tag_4 / Tag 4) from parent "
"<Tag> (28 / tag_3 / Tag 3) to tag_100\n"
"#5: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n"
"#6: Update the parent of tag (external_id=tag_2) from parent (external_id=tag_1) "
"to parent (external_id=None).\n"
"#7: Rename tag value of tag (external_id=tag_2) from 'Tag 2' to 'Tag 31'\n"
"#6: Update the parent of <Tag> (27 / tag_2 / Tag 2) from parent "
"<Tag> (26 / tag_1 / Tag 1) to None\n"
"#7: Rename tag value of <Tag> (27 / tag_2 / Tag 2) to 'Tag 31'\n"
"\nOutput errors\n"
"--------------------------------\n"
"Conflict with 'create' (#2) and action #1: Duplicated external_id tag.\n"
Expand Down Expand Up @@ -299,11 +299,11 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions
"--------------------------------\n"
"#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n"
"#2: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n"
"#3: Rename tag value of tag (external_id=tag_2) from 'Tag 2' to 'Tag 2 v2'\n"
"#4: Update the parent of tag (external_id=tag_4) from parent (external_id=tag_3) "
"to parent (external_id=tag_1).\n"
"#5: Rename tag value of tag (external_id=tag_4) from 'Tag 4' to 'Tag 4 v2'\n"
"#6: No changes needed for tag (external_id=tag_1)\n"
"#3: Rename tag value of <Tag> (27 / tag_2 / Tag 2) to 'Tag 2 v2'\n"
"#4: Update the parent of <Tag> (29 / tag_4 / Tag 4) from parent "
"<Tag> (28 / tag_3 / Tag 3) to tag_1\n"
"#5: Rename tag value of <Tag> (29 / tag_4 / Tag 4) to 'Tag 4 v2'\n"
"#6: No changes needed for <TagItem> (tag_1 / Tag 1)\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a user perspective, it's confusing that this sometimes says <TagItem> and other times says <Tag>. We may need to tweak this in the future, especially if we're trying to display this nicely in the frontend. (Though maybe we also need a JSON version of the plan for that.) I think this is OK for now though.

),
# Testing deletes (replace=True)
(
Expand All @@ -317,11 +317,11 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions
True,
"Import plan for Import Taxonomy Test\n"
"--------------------------------\n"
"#1: No changes needed for tag (external_id=tag_4)\n"
"#1: No changes needed for <TagItem> (tag_4 / Tag 4)\n"
"#2: Delete tag (external_id=tag_1)\n"
"#3: Delete tag (external_id=tag_2)\n"
"#4: Update the parent of tag (external_id=tag_4) from parent (external_id=tag_3) "
"to parent (external_id=None).\n"
"#4: Update the parent of <Tag> (29 / tag_4 / Tag 4) from parent "
"<Tag> (28 / tag_3 / Tag 3) to None\n"
"#5: Delete tag (external_id=tag_3)\n"
),
)
Expand All @@ -336,7 +336,6 @@ def test_plan(self, tags, replace, expected):
tags = [TagItem(**tag) for tag in tags]
self.import_plan.generate_actions(tags=tags, replace=replace)
plan = self.import_plan.plan()
print(plan)
assert plan == expected

@ddt.data(
Expand Down
2 changes: 1 addition & 1 deletion tests/openedx_tagging/core/tagging/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def test_representations(self):
== repr(self.language_taxonomy)
== "<LanguageTaxonomy> (-1) Languages"
)
assert str(self.bacteria) == repr(self.bacteria) == "<Tag> (1) Bacteria"
assert str(self.bacteria) == repr(self.bacteria) == "<Tag> (1 / Bacteria)"

def test_taxonomy_cast(self):
for subclass in (
Expand Down
Loading