-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Fix RedundantPresenceValidationOnBelongs on some files #12407
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
class RequireOrderCycleAndSenderAndReceiverOnExchange < ActiveRecord::Migration[7.0] | ||
def change | ||
change_column_null :exchanges, :order_cycle_id, false | ||
change_column_null :exchanges, :sender_id, false | ||
change_column_null :exchanges, :receiver_id, false | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
class RequireNameAndCoordinatorOnOrderCycle < ActiveRecord::Migration[7.0] | ||
def change | ||
change_column_null :order_cycles, :name, false | ||
change_column_null :order_cycles, :coordinator_id, false | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
class RequireAddress1AndCityAndPhoneAndCountryAndOnAddress < ActiveRecord::Migration[7.0] | ||
def change | ||
change_column_null :spree_addresses, :address1, false | ||
change_column_null :spree_addresses, :city, false | ||
change_column_null :spree_addresses, :phone, false | ||
change_column_null :spree_addresses, :country_id, false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, we got a problem here, too:
This one should be quite easy to fix by setting the default country on this instance, manually in the database. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would also be good to check if these invalid addresses are actually used. Unfortunately, we have 13 references from other tables to addresses. We should probably script that. |
||
end | ||
end |
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.
We don't actually have to enforce these three. If they are null, the records are invalid in Rails but we have some database violations. I ran this:
Most databases are okay, but some are not:
Same numbers for
city
.And for
phone
we have the same in AU but two other results: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.
Mmh, Can/Should I remove this migration file now that this is merged ?
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.
I am looking at this, we are going to try fix the problematic data, but if it's too much of an issue we might revert some of the migrations