Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure promotions via a configuration instance #5738

Merged
merged 1 commit into from
May 3, 2024
Merged

Configure promotions via a configuration instance #5738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions core/lib/spree/app_configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,12 +422,6 @@ def payment_canceller
)
end

# Allow switching out the promotion configuration class
#
# @!attribute [rw] promotion_configuration_class
# @return [Class] a class instance that fulfils the interface of a promo configuration
class_name_attribute :promotion_configuration_class, default: 'Spree::Core::PromotionConfiguration'

# Allows providing your own class for adding payment sources to a user's
# "wallet" after an order moves to the complete state.
#
Expand Down Expand Up @@ -594,8 +588,13 @@ def stock
@stock_configuration ||= Spree::Core::StockConfiguration.new
end

# Allows providing your own promotion configuration instance
# @!attribute [rw] promotions
# @return [Spree::Core::PromotionConfiguration] an object that conforms to the API of
# the standard promotion configuration class Spree::Core::PromotionConfiguration.
attr_writer :promotions
def promotions
@promotion_configuration ||= promotion_configuration_class.new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explained like this, it makes total sense. I was just missing some context. Going to approve this.

@promotions ||= Spree::Core::PromotionConfiguration.new
end

class << self
Expand Down
2 changes: 1 addition & 1 deletion core/spec/lib/spree/app_configuration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
end

it "uses core's promotion configuration class by default" do
expect(prefs.promotion_configuration_class).to eq Spree::Core::PromotionConfiguration
expect(prefs.promotions).to be_a Spree::Core::PromotionConfiguration
end

context "deprecated preferences" do
Expand Down
Loading