Enable custom shipping promotions via config.spree.promotions.shipping_actions #2135
Conversation
…g_actions This updates the shipping promotion handler so that it will look for promotion action types via `Rails.application.config.spree.promotions.shipping_actions`, instead of only looking for actions of type `Spree::Promotion::Actions::FreeShipping`. This should allow applications to create their own shipping promotion actions.
| @@ -105,6 +105,12 @@ class Engine < ::Rails::Engine | |||
| ] | |||
| end | |||
|
|
|||
| initializer 'spree.promo.register.promotions.shipping_actions', before: :load_config_initializers do |app| | |||
There was a problem hiding this comment.
do we need to keep Spree::Promotion::Actions::FreeShipping into spree.promo.register.promotions.actions as well?
There was a problem hiding this comment.
At the moment we do, unfortunately. We could try to avoid the duplication by combining the contents of these two lists when reading the values, but I wonder if you might want to remove a shipping promotion action from promotions.actions so that it doesn't show up in the admin UI, but you'd want to keep it in promotions.shipping_actions so that existing orders & promotions would continue to work?
Very open to ideas on this.
There was a problem hiding this comment.
Thanks for the clarification. I have no good ideas other than the one you proposed. Anyway at the moment I can't figure out a scenario when removing something from that actions list would break existing orders and promotions. I mean, isn't the same if we remove another action from that actions list, like Spree::Promotion::Actions::CreateAdjustment?
There was a problem hiding this comment.
@kennyadsl here's the case that I'm thinking of:
- Developer creates a
OurGreatShippingActionpromo action and adds it to the list - Admin creates a promo using that action
- The store decides not to do any more of those promo actions and a developer removes the
OurGreatShippingActionfrom the list so it doesn't show up in the admin for new promos anymore. - However, if this also removes the action from
Spree::PromotionHandler::Shipping#active_shipping_promotionsthen that means that the existing promo will cease to work as well.
So we need 2 lists -- one list that indicates what actions should be available in the admin, and one list that indicates what promotions to consider in Spree::PromotionHandler::Shipping#active_shipping_promotions.
The difference from other promo actions is that other promo actions don't need a special invocation to work correctly -- i.e. other actions don't need a before_transition from: :delivery, do: :apply_shipping_promotions in order to work correctly.
There was a problem hiding this comment.
A couple other possibilities:
A) Just decide we don't care about the scenario above
B) Figure out how to make shipping promotions work correctly without the before_transition from: :delivery, do: :apply_shipping_promotions transition (i.e. eliminate the need to even have a Spree::PromotionHandler::Shipping class at all).
C) Think of another way to identify Shipping promo actions in Spree::PromotionHandler::Shipping. Perhaps all Shipping promo actions must inherit from a base class? Or have a is_shipping field on the action?
It might be worth just going w/ what's in this PR and iterating on it?
There was a problem hiding this comment.
Got it, thanks for taking the time to explain this better. IMO it's reasonable to care about the above scenario. As you said, there could be promotions that would stop working if a developer removes that class from the list before all promos with that actions are expired or disabled.
I'm fine merging and iterating on this, maybe with time we'll be able to simplify this logic a lot since it seems like we just want to automatically try to apply some promotions after a specific checkout step (probably I'm too optimistic and it's much more complex than this 😄 ).
| @@ -1,7 +1,7 @@ | |||
| module Spree | |||
| module PromotionHandler | |||
| # Used for activating promotions with shipping rules | |||
| class FreeShipping | |||
| class Shipping | |||
There was a problem hiding this comment.
We probably want to keep Spree::PromotionHandler::FreeShipping as a deprecated constant.
This is probably a good case for ActiveSupport::Deprecation::DeprecatedConstantProxy. I believe we re-add a promotion_handler/free_shipping.rb with
module Spree
module PromotionHandler
FreeShipping =
ActiveSupport::Deprecation::DeprecatedConstantProxy.new('FreeShipping', 'Shipping', Spree::Deprecation)
end
endThere was a problem hiding this comment.
@jhawthorn nice, I hadn't seen that before. Added.
This updates the shipping promotion handler so that it will look for promotion
action types via
Rails.application.config.spree.promotions.shipping_actions,instead of only looking for actions of type
Spree::Promotion::Actions::FreeShipping.This should allow applications to create their own shipping promotion actions.