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

Back out deletion of submissions when a source is deleted #1673

Closed
wants to merge 1 commit into from

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented Apr 26, 2017

Status

Ready for review

Description of Changes

This was a addition in #1440, but similarly to #1671 until we have a migration path
for making changes like this to the database we cannot ship this yet
to instances (it adds an ON DELETE CASCADE constraint).

Pushed to 0.4.x release series.

Added to checklist in #1658.

Testing

  1. Provision dev VM.
  2. Make sure dev tests pass locally.

Checklist

If you made changes to the app code:

  • Unit and functional tests pass on the development VM

This was an addition in #1440, but until we have a migration path
for making changes like this to the database we cannot ship this
to instances.
@redshiftzero
Copy link
Contributor Author

redshiftzero commented Apr 26, 2017

Hmm, so I just read about how SQLAlchemy does this a little more, and it looks like this might actually be done in the ORM and so okay to leave in. From the docs:

The behavior of SQLAlchemy’s “delete” cascade has a lot of overlap with the ON DELETE CASCADE feature of a database foreign key, as well as with that of the ON DELETE SET NULL foreign key setting when “delete” cascade is not specified. Database level “ON DELETE” cascades are specific to the “FOREIGN KEY” construct of the relational database; SQLAlchemy allows configuration of these schema-level constructs at the DDL level using options on ForeignKeyConstraint which are described at ON UPDATE and ON DELETE.

But for safety, need to test the upgrade to check that there isn't an unforeseen issue here.

@redshiftzero
Copy link
Contributor Author

Yeah, in further digging around on how this is done in SQLAlchemy, this is indeed ORM only. In order to set ON DELETE CASCADE one has to explicitly set the ForeignKeyConstraint. So! We can leave this in and I'm closing this PR.

@conorsch
Copy link
Contributor

Manual testing of the upgrade strategy here went well, so agreed we can close. Even when this feature lands, it'll only help for future delete actions. We should consider cleaning up dangling database references in a future migration.

@redshiftzero
Copy link
Contributor Author

Yep, totally. We have the cleanup still to do in issue #1189.

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.

2 participants