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

Delete any annotated channelmetadata many to many fields to avoid integrity errors #9141

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Feb 25, 2022

Summary

  • Currently included_languages is a many to many field from ChannelMetadata to Languages
  • Django implements this via a through table
  • When we delete the ChannelMetadata object via SQLAlchemy, this does not get properly deleted, because the schema we are using does not know about the included languages table
  • This is because using a schema that did know about it would cause other errors, so we let sleeping dogs lie
  • To workaround this, we explicitly delete any many to many through tables that are pointing at ChannelMetadata in order to prevent Integrity errors caused by trying to delete a channel metadata row when a many to many through row still points at it
  • Adds a regression test that does not replicate the integrity errors, but does at least assert that the deletion of the included languages has occurred.

Reviewer guidance

Does the test make sense?
Can you think of another way to try to actually get the reported integrity error to trigger in the test?


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Feb 25, 2022
@rtibbles rtibbles added this to the 0.15.2 milestone Feb 25, 2022
Copy link
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.

Code changes make sense, but I haven't tested

@rtibbles rtibbles merged commit 41c2e7b into learningequality:release-v0.15.x Mar 16, 2022
@rtibbles rtibbles deleted the foreign_languages_are_the_key branch March 16, 2022 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants