Move eligible column to legacy promotions

[mamhoff]

Summary

The eligible column on spree_adjustments is only ever used in the legacy promotion system. It should live there.

This simplifies the API of Solidus core quite a bit, as views and serializers do not need to think about whether an adjustment is eligible (tax adjustments are always eligible, for example). SolidusFriendlyPromotions never creates ineligible adjustments.

The idea is: If an adjustment is there, it's real and can be used and reckoned with.

Because it works, and this is code that is slated for deprecation, I've not added Deface, but instead overridden all partials that this affects. Particularly mailer partials can't even be targeted with Deface (no HTML, no Deface).

This is probably the one moment we can move this column out of core.

[codecov]

Codecov Report

Attention: Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.82%. Comparing base (a9d73fb) to head (03b3890).

Files	Patch %	Lines
..._legacy_promotions/models/spree_order_decorator.rb	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5802      +/-   ##
==========================================
+ Coverage   88.78%   88.82%   +0.03%     
==========================================
  Files         731      735       +4     
  Lines       17057    17103      +46     
==========================================
+ Hits        15144    15191      +47     
+ Misses       1913     1912       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kennyadsl]

I'm not sure about this one. I'd agree this is a good approach if we don't have to override so many things.

I'd love to have the simplicity of the API without the eligible thing, but probably I'd evaluate keeping it (and noopifying it for the new promotion system). We could deprecate it later, after we extract the legacy promotion system out of the core, so we could have a clean interface in the next major.

I'm a bit battled on this one to be honest, let me know your thoughts on this approach.

[tvdeyen]

I do not see an issue with this at all. As long as we release legacy promotions with the next major of solidus everything will behave like before. The .eligible scope is still present and does the same as before. Even the template changes wouldn't be necessary imo, but I think it is the right thing to do.

@kennyadsl can you explain what potential issues this change might have?

[tvdeyen]

Now with Rails 7.0 as schema version you can make use of if_not_exists: true here

[mamhoff]

Or I can just leave it like this, I don't think there's a big advantage to it.

[kennyadsl]

can you explain what potential issues this change might have?

The only issue I had in mind is that there is a lot of duplication. In the initial phase, we "kinda" need to maintain both versions of the promotion system, and we need to recall to update all the pieces that we are overriding with this PRs on the legacy promotion system.

[tvdeyen]

can you explain what potential issues this change might have?

The only issue I had in mind is that there is a lot of duplication. In the initial phase, we "kinda" need to maintain both versions of the promotion system, and we need to recall to update all the pieces that we are overriding with this PRs on the legacy promotion system.

Do we plan to evolve the legacy promotions system or just "keep the lights on"? I hope we will move forward with the new promotion system and keep the legacy one "as is", only fixing security issues and such. Not even fixing bugs (at least not existing ones), because this is the whole part of the deal, right? The legacy system is so flawed (and eligible Adjustments is part of that) we do not even want to see it ever again. At least for me :)

What kind of maintenance do you have in mind?

[mamhoff]

So I like to do things fully and not leave unfinished business behind, and the eligible column is - to me - unfinished business. I agree it looks like a lot to maintain, but then I don't think it will be:
For people still using the legacy promotion system, nothing changes (they won't even get the deprecation warnings, because the deprecation is associated to the method object, not the method name!). For people using no promotions or the upcoming solidus_promotions, we reward them with a simpler API and less baggage.

If we're concerned about backwards compatibility, I could make this less dangerous by keeping the implementation and the column, but deprecating it, and removing the column and the implementation with the next major version. I would still keep the partial changes though, so we don't have the baggage of lots of .eligible calls in our templates.

[jarednorman]

I think I agree with @mamhoff on this one.