Configure promotions via a configuration instance #5738
Conversation
github-actions
bot
added
the
changelog:solidus_core
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5738 +/- ##
=======================================
Coverage 88.84% 88.84%
=======================================
Files 704 704
Lines 16761 16761
=======================================
Hits 14891 14891
Misses 1870 1870 ☔ View full report in Codecov by Sentry. |
Prior to this, we would only allow changing the configuration of the promotion system by switching the `promotions_configuration_class`. The `AppConfiguration` would then instantiate that class, and one would be able to do all the necessary customizations via the instance. This allows customizing a promotion configuration and then setting it as the promotion configuration class. It's a lot easier to understand.
mamhoff
force-pushed
the
promotions-attribute-accessor
branch
from
0b09bc1
to
6712bd3
Compare
| def promotions | ||
| @promotion_configuration ||= promotion_configuration_class.new |
There was a problem hiding this comment.
Maybe it goes beyond this PR, but are we sure we want to use promotions as the interface for a PromotionConfiguration object? I have the feeling something is off here, and this could be a good opportunity to refactor, what do you think?
There was a problem hiding this comment.
We're already in the context of a configuration object, and I feel that Spree::Config.promotion_configuration is a mouthful and doesn't provide any more information than Spree::Config.promotions. Especially because some of the methods in the configuration object have more promotion: Spree::Config.promotion_configuration.promotions_adjuster_class.new(order), for example, has more duplication than I would wish for (I'm even thinking of shortening that before the release to just adjuster_class.
So ideally, we'd have something like Spree::Config.promotions.adjuster_class.new(order). IMO, shorter is better.
However, I'm not willing to make this a hill I die on. If y'all think more verbosity is better, I'm all ears.
There was a problem hiding this comment.
Explained like this, it makes total sense. I was just missing some context. Going to approve this.
Summary
Prior to this, we would only allow changing the configuration of the promotion system by switching the
promotions_configuration_class. TheAppConfigurationwould then instantiate that class, and one would be able to do all the necessary customizations via the instance.This allows customizing a promotion configuration and then setting it as the promotion configuration class. It's a lot easier to understand.
Checklist
The following are mandatory for all PRs: