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

[rpenido]

openedx/modular-learning#170

Description

If we try to import tags over a taxonomy that shares the same Tag Value, an error is thrown about the conflict tag value.

It can be reproduced by importing the Template Taxonomy over the "Lightcast Open Skills Taxonomy".
Both taxonomies have a tag with the value "piano". In the plan, it tries to create the tag piano for the Template before deleting the one from Lightcast, raising an error.

This PR changes the order of the import actions, always running the DeleteTag actions before others to avoid this.

Testing Instructions

Please ensure that the tests cover the expected behavior.

---

Private-ref:

• FAL-3603

[openedx-webhooks]

Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[ChrisChV]

@rpenido Looks good 👍, only one suggestion

• I tested this: I tested that the bug is gone
• I read through the code and considered the security, stability and performance implications of the changes.
• Includes tests for bugfixes and/or features added.
• Includes documentation

[ChrisChV]

@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?

[rpenido]

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).

[ChrisChV]

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!

[bradenmacdonald]

I would prefer a few more comments to explain what the code is doing, as it's a little hard to follow in some places, but overall this seems fine to me. Thanks!

Should we merge #135 first?

[rpenido]

Should we merge #135 first?

They are independent. You could merge this first.

We will have a minor conflict anyway in the tests because:

• This PR changes the order of the actions
• fix: Issue when delete all tags on import [FC-0036] #135 changes the tag string representation

I added a comment in the code that will conflict.

[openedx-webhooks]

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.