Skip to content

Minimal fix for duplicate tags for null channel errors#3701

Merged
bjester merged 5 commits intolearningequality:hotfixesfrom
rtibbles:that_syncing_feeling_again
Sep 30, 2022
Merged

Minimal fix for duplicate tags for null channel errors#3701
bjester merged 5 commits intolearningequality:hotfixesfrom
rtibbles:that_syncing_feeling_again

Conversation

@rtibbles
Copy link
Copy Markdown
Member

Summary

Description of the change(s) you made

  • Avoids using get_or_create as will error out when there are multiple ContentTag objects for the same tag_name and null channel combination
  • Removes unused code in models.py that I discovered in the course of this
  • Adds a test for syncing tags and a regression test for this fix
  • Fixes a bug that would happen after copying a node due to not setting original_channel_id on a ContentNode during the copy operation on the frontend
  • Fixes a bug that prevented any refresh of data from the backend if any changes were present in the changes table, even if they had been synced

Manual verification steps performed

  1. Import a resource from one channel into another channel
  2. In the source channel apply a tag "testtag"
  3. In Django shell (./contentcuration/manage.py shell) create a tag with the same tag_name property
from contentcuration.models import ContentTag
ContentTag.objects.create(tag_name="testtag", channel_id=None)
  1. In the channel that imported this resource, now initiate a sync, and be sure to select Tags to sync
  2. See the sync is successful

For the copy bug, this replicates by doing the following:

  1. Import a resource from another channel
  2. Wait for the copy to complete
  3. Edit the resource - in the failure case the edit modal will only partially render - with this fix it now renders properly.

Fixes #3690
Fixes other undocumented bugs.

@rtibbles rtibbles requested a review from bjester September 29, 2022 19:17
@rtibbles rtibbles force-pushed the that_syncing_feeling_again branch from 0bf5231 to 10f4e34 Compare September 29, 2022 19:25
Copy link
Copy Markdown
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

LGTM! Will this fix #3526?

@bjester bjester merged commit c76a65e into learningequality:hotfixes Sep 30, 2022
@rtibbles rtibbles deleted the that_syncing_feeling_again branch September 30, 2022 14:11
@pcenov
Copy link
Copy Markdown
Member

pcenov commented Oct 3, 2022

@rtibbles none of the newly added fields such as Learning Activity, Level, Requirements and Category are being synced - is this intentional or an oversight?

@rtibbles
Copy link
Copy Markdown
Member Author

rtibbles commented Oct 3, 2022

This is an oversight - I'll need to make some updates to ensure these get synced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants