-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Do a solidus 1.4 roll-up migration #2229
Conversation
5752283
to
b84f120
Compare
t.integer "ship_address_id" | ||
t.integer "bill_address_id" | ||
t.datetime "created_at", :null => false | ||
t.datetime "updated_at", :null => false |
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.
Use the new Ruby 1.9 hash syntax.
t.string "login" | ||
t.integer "ship_address_id" | ||
t.integer "bill_address_id" | ||
t.datetime "created_at", :null => false |
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.
Use the new Ruby 1.9 hash syntax.
t.string "single_access_token" | ||
t.string "perishable_token" | ||
t.integer "login_count", :default => 0, :null => false | ||
t.integer "failed_login_count", :default => 0, :null => false |
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.
Use the new Ruby 1.9 hash syntax.
t.string "persistence_token" | ||
t.string "single_access_token" | ||
t.string "perishable_token" | ||
t.integer "login_count", :default => 0, :null => false |
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.
Use the new Ruby 1.9 hash syntax.
# and creates a table equivolent to it | ||
create_table "spree_users", :force => true do |t| | ||
t.string "crypted_password", :limit => 128 | ||
t.string "salt", :limit => 128 |
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.
Use the new Ruby 1.9 hash syntax.
# but spree_api depends on it existing. This defininition comes from solidus_auth_devise's first migration, | ||
# and creates a table equivolent to it | ||
create_table "spree_users", :force => true do |t| | ||
t.string "crypted_password", :limit => 128 |
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.
Use the new Ruby 1.9 hash syntax.
# this table should not technically exist in the database (as its provided by auth_devise), | ||
# but spree_api depends on it existing. This defininition comes from solidus_auth_devise's first migration, | ||
# and creates a table equivolent to it | ||
create_table "spree_users", :force => true do |t| |
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.
Use the new Ruby 1.9 hash syntax.
b84f120
to
9d13257
Compare
The logical break between solidus 1.4 and 2.0 means we can take some liberties with some clean up in a pretty safe way. This rolls up the migrations (something we haven't done for the last 5 years), cleaning up our migration directory and importing fewer migrations into new stores. It also has the added benefit of not needing to run through the entire migration history for a test_app, or having to maintain any of the old migrations. Like the spree_one_two rollup, I had to include a definition of spree_users even though it shouldn't be part of core's schema.
9d13257
to
59bf348
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.
I'm pro this. But can we fix solidus_api
first?
# this table should not technically exist in the database (as its provided by auth_devise), | ||
# but spree_api depends on it existing. This defininition comes from solidus_auth_devise's first migration, | ||
# and creates a table equivolent to it | ||
create_table "spree_users", force: true do |t| |
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.
Can't we fix solidus_api
instead? It should not rely on this table.
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.
Discussed IRL, not reasonable to do with this PR (which doesn't change behavior). We'll need to change behavior to fix it elsewhere.
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.
👌🏻
Also, we are not creating a store in migrations since solidusio#2229, but populating database with default data and sample data (for example creating the sandbox) is creating two different stores.
The logical break between solidus 1.4 and 2.0 means we can take some
liberties with some clean up in a pretty safe way.
This rolls up the migrations (something we haven't done for the last 5
years), cleaning up our migration directory and importing fewer
migrations into new stores.
It also has the added benefit of not needing to run through the entire
migration history for a test_app, or having to maintain any of the old
migrations.