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).
Report is 77 commits behind head on main.

Files with missing lines	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?

[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.

[tvdeyen]

@kennyadsl still have concerns or can we merge?

[kennyadsl]

I was sure I answered here at some point, maybe I forgot to press the "Comment" button, as usual. 😄

I'm totally fine moving forward with this, feel free to merge.

[tvdeyen]

I was sure I answered here at some point, maybe I forgot to press the "Comment" button, as usual. 😄

I'm totally fine moving forward with this, feel free to merge.

No worries. Thank you <3

[MadelineCollier]

Hey I think we are seeing some consistent spec failures relating to the new deprecation warnings:

65) Promotions should display the reimbursements table
      Failure/Error: let!(:reimbursement) { create(:reimbursement) }
      
      ActiveSupport::DeprecationException:
        DEPRECATION WARNING: eligible is deprecated and will be removed from Solidus 5.0 (called from weighted_order_adjustment_amount at /home/circleci/solidus/core/app/models/spree/calculator/returns/default_refund_amount.rb:17)
        
59) Spree::Admin::ReimbursementsController#perform a Spree::Core::GatewayError is raised sets an error message with the correct text
      Failure/Error: let(:reimbursement) { create(:reimbursement) }
      
      ActiveSupport::DeprecationException:
        DEPRECATION WARNING: eligible is deprecated and will be removed from Solidus 5.0 (called from weighted_order_adjustment_amount at /home/circleci/solidus/core/app/models/spree/calculator/returns/default_refund_amount.rb:17)
        
...etc

@mamhoff do you know why we'd be seeing that on main but these were never surfaced on the CI runs against branch?

[mamhoff]

Yes, because #5813 was merged between the opening the PR and merging it. I'll go and fix the warnings.