Remove code column from spree_promotions table

[kennyadsl]

Description

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

Ref. #2979

TODO:

• Remove columns method on Spree::Promotion. This should be done in sync with Fix spree_promotions.code not being ignored properly #3000

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

[aitbw]

LGTM! 💪

[kennyadsl]

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

[mdesantis]

What about create!?

[kennyadsl]

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.

[mdesantis]

I totally agree with that 👍

[spaghetticode]

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

[elia]

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

[kennyadsl]

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.

[tvdeyen]

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! 👏

[tvdeyen]

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

[spaghetticode]

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

[kennyadsl]

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.

[jacobherrington]

LGTM! Thanks Alberto 👍

[spaghetticode]

@kennyadsl great job!

[ericsaupe]

LGTM!