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

Beef up AR migrations with null:false #1705

Conversation

ConnorSheremeta
Copy link
Contributor

#1685 from #1272 (comment) Beefed up AR migrations by stating that certain attributes cannot be null

Also in PR #1704

murny
murny previously approved these changes Jun 10, 2020
Copy link
Contributor

@murny murny left a comment

Choose a reason for hiding this comment

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

So we might have to be careful here editing existing migrations like this (i assume we are probably okay here, as these haven't been ran in production yet). So if we all good, then this looks good 👍 (otherwise we will have to add seperate migrations for adding these as null: false)

We may want to add a ticket in the backlog to make sure we have the same validations on the model layer for this too? Or we could tackle it in this PR, but whatever we decide as long as its getting done. (Some have it already like communities, but collections for example doesn't have a validation for title being required on model level)

Also we should probably run these so the schema.rb file gets updated with these new changes

@ConnorSheremeta
Copy link
Contributor Author

I've added a constraint on the model, the only one that was missing was the collection title as far as I can tell. I have the schema changes in #1704 but I didn't add them here, I thought it would cause problems... I can definitely run through the migration process and commit the new schema db file here if there aren't any issues in doing so. My knowledge of migrations and impact on the schema file doesn't run too deep. I believe there are other changes to the schema that occur during the migrations and rake task that aren't in integration_postmigration that I could add as well, this might be better off in another PR.

@ConnorSheremeta
Copy link
Contributor Author

It has been decided to not commit the changes to schema.db here

@ConnorSheremeta ConnorSheremeta merged commit d0ff3ed into integration_postmigration Jul 14, 2020
@murny murny deleted the cds/add-not-null-migration-constraint branch September 2, 2020 20:30
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