-
Notifications
You must be signed in to change notification settings - Fork 4
Fix user id migrations #319
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
Conversation
…able values, FKs in sqlite)
bbf79c6 to
8aeb783
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add this to a different file, since it is not a assignment handler test e.g. test_db.py or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. And I've also fixed some integrity errors in other tests and in migrations.
Regarding the pragma enabling FKs: we can only register this event listener when the db backend is sqlite, because this pragma is sqlite-specific. With PostgreSQL, executing the pragma will cause an error.
I register the event listener based on the value of theDATABASE_TYPEenv. var. - it probably would be better to actually get the sqlalchemy engine, or the GraderService'sdb_url, and check the dialect there... But I haven't managed to get it to work nicely yet.UPDATE: I've figured out a better way. Seems to work as expected.
One more complication came up when I added a test that checks that the event listener works.
In the test environment,
DATABASE_TYPEis not set, even though an sqlite database is used. We cannot set it "globally" , because some tests use a PostgreSQL database - and again, they would fail if the event listener was enabled.Also: At the moment the migration tests (any many more!) would fail if we just enable the pragma in them all, because many of the tests would cause IntegrityErrors. And we do not have on delete behaviour configured for models related to other models via FKs; we'd have to add
CASCADEto them all, I guess.