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

Make several columns in the audits table NOT NULL #622

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jdufresne
Copy link
Contributor

Improve data integrity by adding NOT NULL constraints to columns that
should always have a value. This helps catch application and library
bugs earlier by catching mishandling of data.

Fixes #572

@danielmorrison
Copy link
Member

I like it.

A couple things:

We test migrations in test/upgrade_generator_test.rb and should do that here too.

I'm confused why you made the test changes. I see that it fails without them but I'm not sure why on first glance. Seems like a smell, but maybe just a test fluke.

@jdufresne
Copy link
Contributor Author

We test migrations in test/upgrade_generator_test.rb and should do that here too.

Added this in the latest revision.

Improve data integrity by adding NOT NULL constraints to columns that
should _always_ have a value. This helps catch application and library
bugs earlier by catching mishandling of data.

Fixes collectiveidea#572
@jdufresne
Copy link
Contributor Author

I'm confused why you made the test changes. I see that it fails without them but I'm not sure why on first glance. Seems like a smell, but maybe just a test fluke.

Good thinking. I tracked this down to auditable_id being NOT NULL, so I've reverted that portion of the change for now.

@jdufresne
Copy link
Contributor Author

@danielmorrison Any thoughts on moving ahead on this?

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.

Add null constraints to migration file?
2 participants