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

Fix migration 20101026184950 down method #144

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

spaghetticode
Copy link
Member

@spaghetticode spaghetticode commented Feb 20, 2019

When the up method of migration 20101026184950 runs after the rollback,
the column unlock_token already exists as it's added by the down method.

There's no apparent reason to keep adding that column in the down method, so
this commit removes it.

@kennyadsl
Copy link
Member

Is it normal that both up and down create the same column? Maybe there's an error there?

@spaghetticode
Copy link
Member Author

I may well be... I'm not 100% sure about that. Since my main goal was to be able to complete Solidus migrations rollback without generating errors, I did the most conservative choice.

This migration is still present in the original form also in the spree version of the gem: https://github.com/spree/spree_auth_devise/blob/master/db/migrate/20101026184950_rename_columns_for_devise.rb

Now I tried to create a sample app without add_column :spree_users, :unlock_token, :string in the down method and both migrations and rollbacks are still fine. If you agree I can change the down instead of the up method.

@kennyadsl
Copy link
Member

kennyadsl commented Feb 20, 2019

Yes I think it's better. I think that line is just a mistake since that same column is removed at line 25 of that migration.

@spaghetticode spaghetticode changed the title Fix migration 20101026184950 when running after rollback Fix migration 20101026184950 down method Feb 20, 2019
@kennyadsl
Copy link
Member

@spaghetticode can you please rebase? I removed the CircleCI config

When the `up` method of migration `20101026184950` runs after the rollback,
the column `unlock_token` already exists as it's added by the `down` method.

There's no apparent reason to keep adding that column in the `down` method, so
this commit removes it.
@kennyadsl kennyadsl merged commit f04f5ca into solidusio:master Mar 13, 2019
@kennyadsl
Copy link
Member

Thanks @spaghetticode !

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