Fix remove code from promotions migration

[kennyadsl]

This PR fixes two issues with the migration introduced into #3028.

Ref #2979

Please note that both issues are quite rare and we've found them by upgrading Solidus on very long-lived stores (previously on Spree).

I think it's better to backport these fixes into v2.8 as well since the migration could fail and some custom change to it could be required.

Promotions with type column set

spree_promotions table used to be named spree_activators in legacy spree versions. This table was used by many models with STI via the type column. After that, it was renamed into spree_promotions so having this type field does not make sense anymore.

The issue here is that if you run this migration on stores that were created before the rename there will be that type field automatically set to Spree::Promotion. This would raise an error while the migration runs:

StandardError:
       An error has occurred, this and all later migrations canceled:

       Invalid single-table inheritance type: Spree::Promotion is not a subclass of RemoveCodeFromSpreePromotions::Promotion

Promotions with empty string code (not nil)

code field could be both nil and an empty string. If multiple empty string codes are present the migration would fail since it's not possible to have more than one code with an empty string in the database.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have added tests to cover this change (if needed)

[jacobherrington]

I am okay with this.

This might be something worth documenting on the Solidus blog?

[seand7565]

I was wondering why we had so many issues with this migration. Looks great, thanks for debugging this!

[kennyadsl]

Yes, it's my fault not considering all the possible scenarios behind removing a simple/unused field. Lesson learned. We need more caution while removing fields from the database and probably even some tests on real stores database before merging.

[spaghetticode]

@kennyadsl thank you for the fix, it's very much appreciated ❤️👍

[tvdeyen]

Thank you 👏

[brchristian]

@kennyadsl Thank you for looking into this! We were just running into this issue ourselves last week.

Does it make sense for legacy stores to drop the type column? And if so, should that be part of this migration?

[kennyadsl]

I think it makes sense to remove that column but I think it's better to do this into another PR/migration since it's not really related to the code column. Probably it would have been to create that migration before, but it's too late now since this migration is already part of a release (v2.8). 😢

[kennyadsl]

@jacobherrington

This might be something worth documenting on the Solidus blog?

👍