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

Remove code column from spree_promotions table #3028

Merged
merged 6 commits into from Jan 29, 2019

Conversation

@kennyadsl
Copy link
Member

commented Jan 6, 2019

Description

This PR removes code column form Spree::Promotion since promotions are now tied to multiple codes via Spree::PromotionCode.

Ref. #2979

TODO:

Checklist:

  • Pull Request guidelines are respected
  • Documentation/Readme have been updated accordingly
  • Changes are covered by tests (if possible)
  • Each commit has a meaningful message attached that described WHAT changed, and WHY

@kennyadsl kennyadsl added the WIP label Jan 6, 2019

@kennyadsl kennyadsl self-assigned this Jan 6, 2019

@kennyadsl kennyadsl force-pushed the nebulab:kennyadsl/remove-code-promotions branch 2 times, most recently from f773bb7 to 1a1729f Jan 6, 2019

@kennyadsl kennyadsl removed the WIP label Jan 8, 2019

@aitbw

aitbw approved these changes Jan 9, 2019

Copy link
Contributor

left a comment

LGTM! 💪

@kennyadsl

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2019

I have to add a guard to let people know that they have to remove data from the column before running the migration. Also, I need to add a changelog entry

@kennyadsl kennyadsl force-pushed the nebulab:kennyadsl/remove-code-promotions branch from d5ab562 to 9c1741f Jan 21, 2019

Show resolved Hide resolved CHANGELOG.md Outdated
Show resolved Hide resolved CHANGELOG.md Outdated
migration_context.say "Creating Spree::PromotionCode with value '#{promotion.code}' "\
"for Spree::Promotion with id '#{promotion.id}'"

promotion.codes.create(value: promotion.code)

This comment has been minimized.

Copy link
@mdesantis

mdesantis Jan 21, 2019

Contributor

What about create!?

This comment has been minimized.

Copy link
@kennyadsl

kennyadsl Jan 21, 2019

Author Member

Thanks for pointing this out @mdesantis !

There's a uniqueness validation on value so if that value is already used (let's say the code has been already moved manually into the Spree::PromotionCode) it will raise an exception.

What about using Spree::PromotionCode.find_or_create_by!(value: value, promotion_id: promotion.id) instead?
I think this way it will only raise if the value is not unique and that value has been used on a Spree::PromotionCode associated with another Spree::Promotion. It will do nothing if the current Spree::Promotion already has a Spree::PromotionCode with the same value.

This comment has been minimized.

Copy link
@mdesantis

mdesantis Jan 21, 2019

Contributor

I totally agree with that 👍

# This is another possible approach, it will convert Spree::Promotion#code
# to a Spree::PromotionCode before removing the `code` field.
#
promotions.each do |promotion|

This comment has been minimized.

Copy link
@spaghetticode

spaghetticode Jan 21, 2019

Contributor

I suppose the number of promotions might be quite high in some databases. In that case find_each should work better than each

# You have some promotions with "code" field present! This is not good
# since we are going to remove that column.
#
self.class.promotions_with_code_handler.new(self, promotions_with_code).call

This comment has been minimized.

Copy link
@elia

elia Jan 21, 2019

Contributor

I think it would be better to use a simple callable instead of a class that responds to #call and pass options directly to call, also a constant would be more straightforward than a method:

Suggested change
self.class.promotions_with_code_handler.new(self, promotions_with_code).call
deprecated_code_handler = StopWithError
# deprecated_code_handler = DoNothing
# deprecated_code_handler = MoveToSpreePromotionCode
deprecated_code_handler.call(self, promotions_with_code)

and then below:

# Please note that this will block the current migration and rollback all
# the previous ones run with the same "rails db:migrate" command.
StopWithError = ->(migration, promotions) {
  raise StandardError, "You are trying to drop 'code' column from "\
    "spree_promotions table but you have at least one record with that "\
    "column filled. Please take care of that or you could lose data. See:" \
    "\n" \
    "https://github.com/solidusio/solidus/pull/3028"\
    "\n"
}

This is much simpler and has the advantage of having the configuration near the error location, making it easier to find

This comment has been minimized.

Copy link
@kennyadsl

kennyadsl Jan 21, 2019

Author Member

This is very similar to what I did first but to test all the possible proposed approaches I ended up creating a method. I know it's not ideal but I can't think to other solutions for this. Feel free to suggest how can I keep this code tested reducing its complexity.

This comment has been minimized.

Copy link
@tvdeyen

tvdeyen Jan 22, 2019

Member

I like Elias idea of using simple callables, but Albertos approach is working well and is tested (not that easy with lambdas), so I think it's fine. The outcome is the same.

What I do care about is the very thoughtful work that is done here and want to give an extra appreciation! Very nicely done, Alberto! 👏

@kennyadsl kennyadsl force-pushed the nebulab:kennyadsl/remove-code-promotions branch 2 times, most recently from c544736 to 2c2318f Jan 21, 2019

@tvdeyen
Copy link
Member

left a comment

Thanks for working on this. Especially the very thoughtful way of handling the data migration. Really great work. Also the commit messages are 👌

# This approach will delete all codes without taking any action. At
# least we could print a message to track what we are deleting.
#
promotions.each do |promotion|

This comment has been minimized.

Copy link
@spaghetticode

spaghetticode Jan 22, 2019

Contributor

There's also this instance of each that can be replaced with find_each. Sorry for not finding it earlier.

@kennyadsl kennyadsl force-pushed the nebulab:kennyadsl/remove-code-promotions branch 2 times, most recently from f0adae8 to 9d22219 Jan 22, 2019

@kennyadsl

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

There's some issue with the database in test environment when it tries to roll back to the previous state after the test ends, I need to investigate more on why it's happening.

@kennyadsl kennyadsl force-pushed the nebulab:kennyadsl/remove-code-promotions branch 4 times, most recently from 5d20dfc to 66d3c63 Jan 23, 2019

@kennyadsl kennyadsl removed the WIP label Jan 28, 2019

@jacobherrington
Copy link
Member

left a comment

LGTM! Thanks Alberto 👍

@spaghetticode
Copy link
Contributor

left a comment

@kennyadsl great job!

@ericsaupe
Copy link
Contributor

left a comment

LGTM!

kennyadsl added some commits Jan 6, 2019

Add a migration to remove code from spree_promotions
We can't use a simple `change` method here since we also want
to remove the index and we should respect the order of these actions.
Fix Spree::Promotion.coupons scope
Promotions are now tied to multiple code records so this scope does
does not make sense anymore.
Remove an api checkout spec comment
Since it includes a reference to promotion.code that
is no more valid
Do not exclude code column when using Spree::Promotion
This code needs to be removed for 2 reasons:

1. it didn't work, it should be on a class level (`def self.columns`)
   see issue #2979, which can be closed when this is merged.
2. is not used anymore since we are removing the `code` column at
   all from the database.
Handle when a promotion with code is present during migrations
By default we raise and error to give users a notice that we code
column will be removed from spree_promotions table and they will lose
their data.

We are also providing another 2 options that can be activated by
changing `promotions_with_code_handler` class method content making
it return another class. Users can also create their own class directly
into the migration and handle this how they prefer.

@kennyadsl kennyadsl force-pushed the nebulab:kennyadsl/remove-code-promotions branch from 66d3c63 to 3435a9b Jan 28, 2019

@kennyadsl kennyadsl merged commit fc84ef8 into solidusio:master Jan 29, 2019

7 checks passed

Hound No violations found. Woof!
ci/circleci: mysql Your tests passed on CircleCI!
Details
ci/circleci: mysql_rails51 Your tests passed on CircleCI!
Details
ci/circleci: postgres Your tests passed on CircleCI!
Details
ci/circleci: postgres_rails51 Your tests passed on CircleCI!
Details
netlify/solidus-docs/deploy-preview Deploy preview ready!
Details
netlify/solidus-guides/deploy-preview Deploy preview ready!
Details

@kennyadsl kennyadsl deleted the nebulab:kennyadsl/remove-code-promotions branch Jan 29, 2019

@kennyadsl kennyadsl referenced this pull request Feb 18, 2019

Merged

Fix remove code from promotions migration #3108

3 of 3 tasks complete

@kennyadsl kennyadsl referenced this pull request Feb 28, 2019

Merged

Remove code setter/getter from Spree::Promotion #3127

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.