Display time promotion on order page #409
Conversation
❌ Deploy Preview for ecommerce-events failed.
|
| @@ -145,6 +165,14 @@ def use_coupon(coupon_id, discount) | |||
| @discount = Discounts::PercentageDiscount.new(event.data.fetch(:amount)) | |||
| end | |||
|
|
|||
| on TimePromotionDiscountSet do |event| | |||
There was a problem hiding this comment.
Thesis: TimePromotionDiscountSet is just a feature. It could be modeled at upper layer.
There was a problem hiding this comment.
I can see one of typical patterns applied here when it comes to designing aggregates. Often aggregates in projects have tendency to implement features instead of just being able to deal with a piece of business logic. Time promotion discount is great example of that.
What the Offer aggregate needs to deal with discounts is just to know how much is the discount. It is also captured with Time promotion of course, but the time factor here is, in my opinion, superfluous.
Consider that we have another kind of discount. Let it be birthday discount, first time buyer discount, coupon discount etc. This class will just keep growing.
Let's start with a question. What is this class supposed to do?
I assume that it is supposed to calculate the total price including any discounts.
What kind of discounts can we have in ecommerce?
Probably more that I can think of
What kind of discounts should we implement in this aggregate?
In my opinion one method to apply discount is enough
How can it be done differently?
Think about Offer aggregate as something that is used to build an offer. Of course we want to be able to add some discounts for different reasons. I suggest we only have discount method that produces PercentDiscountSet. Then, from the layer above (application layer) there would be a command handler that could apply discount based on command type, by calling Offer#discount method. What kind of discounts? Any that POs might think about:
- Time promotion
- Coupon
- First buy
- ... ?
What changes would I suggest introducing (not necessarily in this PR):
- Command and command handler stays
- Remove time promotions related methods from aggregate
- Create additional table or stream responsible for time discounts for given time period
- Command handlers gets that information, if there are any active time discounts
- and if there are, it calls
Offer#discount
Look at other benefits of this approach:
- CalculateOrderTotal value doesn't have to listen to any new events each time you add new discount
- It implies that it is less error prone. Now if you introduce new discount method and you forget to subscribe to new event produced by that method then the customer experience will be poor. This problem disappears with aforementioned approach
- Offer class has only one small responsibility
As I mentioned, this can be done in different PR. If anything is unclear please ask a question, I'll try to clarify :)
There was a problem hiding this comment.
I decided to this in the same PR. Let me know if the solution is what you were thinking of :)
There was a problem hiding this comment.
Good job @marlena-b! I've added few additional comments to adjust the design, but again, if you agree with them, we can do that in separate PR.
marlena-b
force-pushed
the
display-time-promotion-on-order-page
branch
5 times, most recently
from
6ffee12 to
adf2e58
Compare
marlena-b
force-pushed
the
display-time-promotion-on-order-page
branch
from
adf2e58 to
51340bf
Compare
| @@ -45,7 +45,31 @@ def initialize(event_store) | |||
|
|
|||
| def call(cmd) | |||
| @repository.with_aggregate(Offer, cmd.aggregate_id) do |order| | |||
| order.change_discount(Discounts::PercentageDiscount.new(cmd.amount)) | |||
| order.change_discount(Discounts::GENERAL_DISCOUNT, Discounts::PercentageDiscount.new(cmd.amount)) | |||
There was a problem hiding this comment.
Just to verbalise my thoughts: I wonder if the information about the type of the discount is needed here. I don't know for now, but I think I would transfer this information through the PercentageDiscount class. That way we don't have to change method's interface. And it would be simple to add some default (such as GENERAL_DISCOUNT). What do you think @marlena-b?
There was a problem hiding this comment.
That was my first thought as well. The main problem is that once we add it to PercentageDiscount class it no longer is a value object. There can be only one discount of a specific type assigned to the order so type is actually an identifier of a discount.
This also leads to problems like what type should we use if we want to add two discounts? Or what type should be used for the initial accumulator in the inject here: discounts.values.inject(Discounts::NoPercentageDiscount.new, :add).apply(catalog_price_for_single)? These problems made me feel it is not a right design to add it to this class.
There was a problem hiding this comment.
The main problem is that once we add it to
PercentageDiscountclass it no longer is a value object. There can be only one discount of a specific type assigned to the order so type is actually an identifier of a discount.
The type would still be a type. If only one discount of specifiic type can be assigned to an order this is just a rule that can be exectued within the order class. But I wouldn't say that extending PercentageDiscount by type would make it an entity.
I assume that would still be true:
PercentageDiscount.new(type: GENERAL, amount: 100).eql? PercentageDiscount.new(type: GENERAL, amount: 100)I'll refer to the second part of your reply later :)
There was a problem hiding this comment.
Ok, I can give it another shot 👍 I will merge this PR and fix it in another one.
Issue: #407
Time Promotions are now displayed on order show page. It currently works only on admin side. The client order show page will be adjusted in another PR.
Nagranie.z.ekranu.2024-10-6.o.14.40.31.mov
Combined discounts are displayed correctly as well:
