Adding/Removing items to refunds #425
Conversation
| end | ||
|
|
||
| def remove_item(product_id) | ||
| raise ProductNotFoundError unless @refund_items.quantity(product_id).positive? |
There was a problem hiding this comment.
I wonder if this error could have more meaningful business name. Something like RefundHaveNotBeenRequestedForThisProduct
There was a problem hiding this comment.
Good idea, I changed the name 👍
| def call(event) | ||
| refund_id = event.data.fetch(:refund_id) | ||
| product_id = event.data.fetch(:product_id) | ||
| item = find(refund_id, product_id) |
There was a problem hiding this comment.
Is this event handler idempotent?
There was a problem hiding this comment.
Hmm, it isn't, but most, if not all, of our event handlers aren't idempotent 🤔 I'm not sure if I know how to make it idempotent, maybe you have some ideas?
| @@ -0,0 +1,11 @@ | |||
| module Ordering | |||
| class Projections | |||
There was a problem hiding this comment.
Projection is an implementation detail. I suggest calling this class ProductQuantityAvailableToRefund. Method could be named as call
There was a problem hiding this comment.
Thank you, changed!
| @@ -12,6 +12,14 @@ def create | |||
| redirect_to edit_order_refund_path(refund_id, order_id: params[:order_id]) | |||
| end | |||
|
|
|||
| def add_item | |||
| add_item_to_refund | |||
There was a problem hiding this comment.
I wonder if this abstraction brings any benefit. Do you think it is more readable this way?
There was a problem hiding this comment.
Tbh I followed the same pattern as is in e.g. products_controller but it is not followed everywhere so it might also be done without this abstraction.
| refund.add_item(command.product_id) | ||
| refund.add_item( | ||
| command.product_id, | ||
| available_quantity_to_refund(command.order_id, command.product_id) |
There was a problem hiding this comment.
I think that the available quantity to refund information should be passed to Refund once it is requested. We don't want this value to be changed.
I see it that way:
- Order has been paid
- Someone wants to refund part of it or whole
- Refund is created, it knows what products (or how many this case*) were there in the order
- It allows refunding those products or not
-
- Perhaps it should know which products were in the order initially, not only quantity of them?
- Are we talking about refunding items or product? Method names of refund imply items, but projection's method indicates product.
There was a problem hiding this comment.
Thanks for the suggestion! I'm not sure if I understand what you mean.
You propose that when creating a Refund, we should pass in all the quantities per product (e.g., { product_1_id: 13, product_2_id: 3 }), and store this information within the Refund aggregate. This would allow us to validate refund operations against the original available quantities without needing to query the projection each time.
Is that what you mean?
Issue: #154
This is the second step to allow order refunds. First step is here: #424.
Nagranie.z.ekranu.2025-01-7.o.10.07.34.mov