Skip to content

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

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.

Sign up for GitHub

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

Merged
bradenmacdonald merged 10 commits into from
Jan 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fb8d161
fix: Issue when delete all tags on import
ChrisChV Dec 20, 2023
b1c4cc9
fix: issuo on delete all taxonomy on import
ChrisChV Dec 21, 2023
e23f542
chore: bump version
ChrisChV Dec 21, 2023
e91e735
Merge branch 'main' into chris/fix-delete-all-on-import
ChrisChV Jan 4, 2024
6181f7c
refactor: Updates on representation of tags and actions
ChrisChV Jan 8, 2024
0b09872
fix: Bug on test
ChrisChV Jan 8, 2024
f2df68b
fix: Bug on test
ChrisChV Jan 8, 2024
f2a341d
chore: Merge conflicts
ChrisChV Jan 11, 2024
9030b1e
chore: Update actions logs
ChrisChV Jan 11, 2024
fa42366
style: Delete prints
ChrisChV Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 40 additions & 8 deletions openedx_tagging/core/tagging/import_export/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ def _get_tag(self) -> Tag:
"""
Returns the respective tag of this actions
"""
return self.taxonomy.tag_set.get(external_id=self.tag.id)
if self.tag.id:
return self.taxonomy.tag_set.get(external_id=self.tag.id)
Comment thread
bradenmacdonald marked this conversation as resolved.
Outdated
return self.taxonomy.tag_set.get(value=self.tag.value)

def _search_action(
self,
Expand Down Expand Up @@ -256,16 +258,35 @@ class UpdateParentTag(ImportAction):

def __str__(self) -> str:
taxonomy_tag = self._get_tag()

if not taxonomy_tag.parent:
from_str = _("from empty parent")
else:
from_str = _("from parent (external_id={external_id})").format(external_id=taxonomy_tag.parent.external_id)
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
)
from_str = ""

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
)

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

@classmethod
Expand Down Expand Up @@ -323,11 +344,18 @@ 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(
_(
"Rename tag value of tag (external_id={external_id}) "
"{prefix_str} "
"from '{from_value}' to '{to_value}'"
).format(external_id=taxonomy_tag.external_id, from_value=taxonomy_tag.value, to_value=self.tag.value)
).format(prefix_str=prefix_str, from_value=taxonomy_tag.value, to_value=self.tag.value)
)

@classmethod
Expand Down Expand Up @@ -373,7 +401,9 @@ class DeleteTag(ImportAction):
"""

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

name = "delete"

Expand Down Expand Up @@ -413,7 +443,9 @@ class WithoutChanges(ImportAction):
name = "without_changes"

def __str__(self) -> str:
return str(_("No changes needed for tag (external_id={external_id})").format(external_id=self.tag.id))
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))

@classmethod
def applies_for(cls, taxonomy: Taxonomy, tag) -> bool:
Expand Down
2 changes: 1 addition & 1 deletion openedx_tagging/core/tagging/import_export/import_plan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def generate_actions(

if replace:
tags_for_delete = {
tag.external_id: tag for tag in self.taxonomy.tag_set.all()
tag.external_id or tag.id: tag for tag in self.taxonomy.tag_set.all()

@rpenido rpenido Dec 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because of this change, I think we also need to change the code below:
https://github.com/openedx/openedx-learning/blob/fb8d16112fa4705fda8f8ac44cd3401cfb08619e/openedx_tagging/core/tagging/import_export/import_plan.py#L96

Context: this is to prevent updating a tag that could already be deleted

The following test (to be put in import_export/test_api.py) reproduce the issue:

    def test_import_removing_with_childs_no_external_id(self) -> None:
        """
        Test import need to remove childs with parents that will also be removed,
        using tags without external_id
        """
        new_taxonomy = Taxonomy(name="New taxonomy")
        new_taxonomy.save()
        level2 = Tag.objects.create(
            id=1000,
            value="Tag 2",
            taxonomy=new_taxonomy,
        )
        level1 = Tag.objects.create(
            id=1001,
            value="Tag 1",
            taxonomy=new_taxonomy,
        )
        level3 = Tag.objects.create(
            id=1002,
            value="Tag 3",
            taxonomy=new_taxonomy,
        )
        level2.parent = level1
        level2.save()

        level3.parent = level3
        level3.save()

        # Import with empty tags, to remove all tags
        importFile = BytesIO(json.dumps({"tags": []}).encode())
        result, _tasks, _plan =  import_export_api.import_tags(
            new_taxonomy,
            importFile,
            ParserFormat.JSON,
            replace=True,
        )
        assert result

@ChrisChV ChrisChV Dec 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Thanks!

@ChrisChV ChrisChV Dec 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rpenido Updated here: b1c4cc9

}

for tag in tags:
Expand Down