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.

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

fix: bug importing tags with the same value as an existing one #141

Merged
merged 1 commit into from
Jan 10, 2024
Merged

fix: bug importing tags with the same value as an existing one #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 34 additions & 24 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 @@ -117,38 +117,48 @@ def _validate_value(self, indexed_actions) -> ImportActionError | None:
actions
"""
try:
# Validates if exists a tag with the same value on the Taxonomy
taxonomy_tag = self.taxonomy.tag_set.get(value=self.tag.value)
return ImportActionError(
action=self,
message=_(
"Duplicated tag value with tag in database (external_id={external_id})."
).format(external_id=taxonomy_tag.external_id)
)
is_deleted_tag_value = any(
self.tag.value == action.tag.value
for action in indexed_actions["delete"]
) if "delete" in indexed_actions else False

# If the tag will be deleted, skip the Database validation
if not is_deleted_tag_value:
# Validates if exists a tag with the same value on the Taxonomy
taxonomy_tag = self.taxonomy.tag_set.get(value=self.tag.value)
return ImportActionError(
action=self,
message=_(
"Duplicated tag value with tag in database (external_id={external_id})."
).format(external_id=taxonomy_tag.external_id)
)
except Tag.DoesNotExist:
# Validates value duplication on create actions
pass

# Validates value duplication on create actions
action = self._search_action(
indexed_actions,
CreateTag.name,
"value",
self.tag.value,
)

if not action:
# Validates value duplication on rename actions
action = self._search_action(
indexed_actions,
CreateTag.name,
RenameTag.name,
"value",
self.tag.value,
)

if not action:
# Validates value duplication on rename actions
action = self._search_action(
indexed_actions,
RenameTag.name,
"value",
self.tag.value,
)
if action:
return ImportActionConflict(
action=self,
conflict_action_index=action.index,
message=_("Duplicated tag value."),
)

if action:
return ImportActionConflict(
action=self,
conflict_action_index=action.index,
message=_("Duplicated tag value."),
)
return None


Expand Down
16 changes: 8 additions & 8 deletions 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 @@ -49,7 +49,7 @@ def _init_indexed_actions(self):
for action in available_actions:
self.indexed_actions[action.name] = []

def _build_action(self, action_cls, tag: TagItem):
def _build_action(self, action_cls: type[ImportAction], tag: TagItem):
"""
Build an action with `tag`.

Expand Down Expand Up @@ -139,6 +139,13 @@ def generate_actions(
tag.external_id: tag for tag in self.taxonomy.tag_set.all()
}

for tag in tags:
Copy link
Contributor

Choose a reason for hiding this comment

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

@rpenido Maybe to avoid this loop, you can add the delete actions first using the _build_action() func, adding a new param add_first. What do you think?

Copy link
Contributor Author

@rpenido rpenido Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My first attempt was just to change the self.actions.append(action) to self.actions.insert(0, action) for the DeleteTag here (this was your suggestion?):

openedx-learning/openedx_tagging/core/tagging/import_export/import_plan.py

Lines 64 to 65 in 7072032

# Add action
self.actions.append(action)

This didn't solve the problem because we ran the validation below (inside _build_action() for the CreateTag action) in the plan stage (before actually running the delete action).

openedx-learning/openedx_tagging/core/tagging/import_export/import_plan.py

Lines 61 to 62 in 7072032

# We validate if there are no inconsistencies when executing this action
self.errors.extend(action.validate(self.indexed_actions))

openedx-learning/openedx_tagging/core/tagging/import_export/actions.py

Lines 120 to 127 in 7072032

# Validates if exists a tag with the same value on the Taxonomy
taxonomy_tag = self.taxonomy.tag_set.get(value=self.tag.value)
return ImportActionError(
action=self,
message=_(
"Duplicated tag value with tag in database (external_id={external_id})."
).format(external_id=taxonomy_tag.external_id)
)

I need to skip this validation if the tag value has a DeleteTag action, so I need to build all the delete actions (calling _build_delete_actions) before validating the new tags.

I also tried to avoid this loop filtering while we constructed the dict here:

openedx-learning/openedx_tagging/core/tagging/import_export/import_plan.py

Lines 138 to 140 in 7072032

tags_for_delete = {
tag.external_id: tag for tag in self.taxonomy.tag_set.all()
}

But because we have tags (from the DB) and tags (from the importing param), the filtering code became nearly intelligible, so I opted for the loop outside (since it is not a nested loop).

Copy link
Contributor

Choose a reason for hiding this comment

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

My first attempt was just to change the self.actions.append(action) to self.actions.insert(0, action) for the DeleteTag here (this was your suggestion?):

Yes, it is my suggestion.

But because we have tags (from the DB) and tags (from the importing param), the filtering code became nearly intelligible, so I opted for the loop outside (since it is not a nested loop).

I see, in that case the loop is fine. Thanks for the explanation!

if tag.id in tags_for_delete:
tags_for_delete.pop(tag.id)

# Delete all not readed tags
self._build_delete_actions(tags_for_delete)

for tag in tags:
has_action = False

Expand All @@ -152,13 +159,6 @@ def generate_actions(
# If it doesn't find an action, a "without changes" is added
self._build_action(WithoutChanges, tag)

if replace and tag.id in tags_for_delete:
tags_for_delete.pop(tag.id)

if replace:
# Delete all not readed tags
self._build_delete_actions(tags_for_delete)

def plan(self) -> str:
"""
Returns an string with the plan and errors
Expand Down
22 changes: 22 additions & 0 deletions tests/openedx_tagging/core/tagging/import_export/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,3 +253,25 @@ def test_import_removing_with_childs(self) -> None:
ParserFormat.JSON,
replace=True,
)

def test_import_same_value_without_external_id(self) -> None:
new_taxonomy = Taxonomy(name="New taxonomy")
new_taxonomy.save()

# Tag with no external_id
Tag.objects.create(
value="same_value",
taxonomy=new_taxonomy,
)

# Import with one tag with the same value
importFile = BytesIO(json.dumps({"tags": [{"id": "imported_tag", "value": "same_value"}]}).encode())

result, _tasks, _plan = import_export_api.import_tags(
new_taxonomy,
importFile,
ParserFormat.JSON,
replace=True,
)

assert result
23 changes: 13 additions & 10 deletions tests/openedx_tagging/core/tagging/import_export/test_import_plan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,6 @@ def test_build_delete_actions(self) -> None:
True,
0,
[
{
'name': 'without_changes',
'id': 'tag_4',
},
{
'name': 'delete',
'id': 'tag_1',
Expand All @@ -201,6 +197,10 @@ def test_build_delete_actions(self) -> None:
'name': 'delete',
'id': 'tag_3',
},
{
'name': 'without_changes',
'id': 'tag_4',
},
]
)
)
Expand Down Expand Up @@ -315,14 +315,18 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions
},
],
True,
# ToDo: Change the lines below when https://github.com/openedx/openedx-learning/pull/135 is merged
"Import plan for Import Taxonomy Test\n"
"--------------------------------\n"
"#1: No changes needed for tag (external_id=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) "
"#1: Delete tag (external_id=tag_1)\n"
"#2: Delete tag (external_id=tag_2)\n"
"#3: Update the parent of tag (external_id=tag_4) from parent (external_id=tag_3) "
"to parent (external_id=None).\n"
"#5: Delete tag (external_id=tag_3)\n"
# "#3: Update the parent of <Tag> (29 / tag_4 / Tag 4) from parent "
# "<Tag> (28 / tag_3 / Tag 3) to None\n"
"#4: Delete tag (external_id=tag_3)\n"
"#5: No changes needed for tag (external_id=tag_4)\n"
# "#5: No changes needed for <TagItem> (tag_4 / Tag 4)\n"
),
)
@ddt.unpack
Expand All @@ -336,7 +340,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
12 changes: 6 additions & 6 deletions tests/openedx_tagging/core/tagging/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2457,12 +2457,12 @@ def test_import_plan(self, file_format: str) -> None:
assert response.data["task"]["status"] == "success"
expected_plan = "Import plan for Test import taxonomy\n" \
+ "--------------------------------\n" \
+ "#1: Create a new tag with values (external_id=tag_1, value=Tag 1, parent_id=None).\n" \
+ "#2: Create a new tag with values (external_id=tag_2, value=Tag 2, parent_id=None).\n" \
+ "#3: Create a new tag with values (external_id=tag_3, value=Tag 3, parent_id=None).\n" \
+ "#4: Create a new tag with values (external_id=tag_4, value=Tag 4, parent_id=None).\n" \
+ "#5: Delete tag (external_id=old_tag_1)\n" \
+ "#6: Delete tag (external_id=old_tag_2)\n"
+ "#1: Delete tag (external_id=old_tag_1)\n" \
+ "#2: Delete tag (external_id=old_tag_2)\n" \
+ "#3: Create a new tag with values (external_id=tag_1, value=Tag 1, parent_id=None).\n" \
+ "#4: Create a new tag with values (external_id=tag_2, value=Tag 2, parent_id=None).\n" \
+ "#5: Create a new tag with values (external_id=tag_3, value=Tag 3, parent_id=None).\n" \
+ "#6: Create a new tag with values (external_id=tag_4, value=Tag 4, parent_id=None).\n"
assert response.data["plan"] == expected_plan

self._check_taxonomy_not_changed()
Expand Down
Loading