Better error when submitting order with out of stock product #390
Conversation
❌ Deploy Preview for ecommerce-events failed.
|
|
|
||
| def initialize(id) | ||
| @id = id | ||
| end | ||
|
|
||
| def set_customer(customer_id) | ||
| raise CustomerAlreadyAssigned if @customer_id | ||
| return if @customer_id == customer_id |
There was a problem hiding this comment.
I think we need to allow users to reassign customers. If order submission fails the first time the user is still being assigned. So when user tries again this raises an exception and brakes the process.
I changed it so when customer_id is not changed it does nothing, but if it is changed then we emit the event.
marlena-b
force-pushed
the
order-with-out-of-stock-product-better-error
branch
from
5e0c00e to
933292f
Compare
| state.order_lines.each do |product_id, quantity| | ||
| command_bus.(Inventory::Reserve.new(product_id: product_id, quantity: quantity)) | ||
| state.product_reserved(product_id) | ||
| rescue Inventory::InventoryEntry::InventoryNotAvailable | ||
| unavailable_products << product_id | ||
| end |
There was a problem hiding this comment.
ReservationProcess no longer stops at the first unavailable product. Instead, it tries to reserve all of them and records which were unavailable. This way we can later display the full list of unavailable products.
| end | ||
| event_store.publish(event, stream_name: stream_name(state.order_id)) |
There was a problem hiding this comment.
ReservationProcess now emits events. We then subscribe to these events in the controller and display proper errors to the user.
We could raise an error instead but then the transaction would be rolled back and most of the code here would make no sense.
| def order_side_effects(state) | ||
| event_store | ||
| .within { yield } | ||
| .subscribe(to: ReservationProcessFailed) { reject_order(state) } | ||
| .subscribe(to: ReservationProcessSuceeded) { accept_order(state) } | ||
| .call |
There was a problem hiding this comment.
I think these side effects could be extracted to fully-fledged event handlers. Then the ReservationProcess could have only a single responsibility. This PR is already big so I didn't want to extract it here. Let me know what you think and I will add an issue for this refactor.
| ActiveRecord::Base.transaction do | ||
| command_bus.(Ordering::SubmitOrder.new(order_id: params[:order_id])) | ||
| command_bus.(Crm::AssignCustomerToOrder.new(customer_id: cookies[:client_id], order_id: params[:order_id])) | ||
| Orders::SubmitService.new(order_id: params[:order_id], customer_id: cookies[:client_id]).call.then do |result| |
There was a problem hiding this comment.
The code in this controller got more and more complex so I extracted it to a service.
I used a very simple Service object implementation combined with a simple but flexible Result object. This results in an expressive DSL like the one below :)
(the DSL was partly inspired by respond_to method)
| redirect_to edit_order_path(params[:order_id]), alert: "You can't submit an empty order" | ||
| rescue Crm::Customer::NotExists | ||
| redirect_to order_path(params[:order_id]), alert: "Order can not be submitted! Customer does not exist." | ||
| Orders::SubmitService.new(order_id: params[:order_id], customer_id: params[:customer_id]).call.then do |result| |
There was a problem hiding this comment.
I initially wanted to have two separate services for the admin and client side but then they looked almost entirely the same. So I decided to merge them so we don't duplicate too much code and tests.
| @@ -0,0 +1,17 @@ | |||
| class ApplicationService | |||
There was a problem hiding this comment.
This is the first time service objects are used in this repo so I wanted to keep them as simple as possible. In my implementation service objects should only implement initialize and call methods and then we can call them like MyService.call(..). This makes it easier to mock them (bc we only need to mock the class method call).
Please let me know if you prefer to use some other implementation instead, this is just my proposal.
| if success | ||
| Result.new(:success) | ||
| else | ||
| Result.new(:products_out_of_stock, unavailable_products) | ||
| end | ||
| rescue Ordering::Order::IsEmpty | ||
| Result.new(:order_is_empty) | ||
| rescue Crm::Customer::NotExists | ||
| Result.new(:customer_not_exists) |
There was a problem hiding this comment.
call method returns a Result object which represents a logical path of execution combined with optional arguments (e.g. unavailable_products).
| def path(name, &block) | ||
| return unless @status == name.to_sym | ||
|
|
||
| block.call(*@args) | ||
| end |
There was a problem hiding this comment.
There is also an alternative implementation which uses more metaprogramming but results in a nicer DSL:
def method_missing(name, *_, &block)
return unless @status == name.to_sym
block.call(*@args)
end
and then the DSL looks like this:
MyService.call(params).then do |on|
on.success { puts "Awesome!" }
on.validation_failed { |errors| puts "Oh no, there are some errors: #{errors}" }
on.something_else { |arg1, arg2| puts arg1 * arg2 }
end
Let me know which one you prefer :)
marlena-b
force-pushed
the
order-with-out-of-stock-product-better-error
branch
from
933292f to
844d110
Compare
|
There are many changes in this pull request which I would like to debate separately, but as a whole I think it's ready to be merged.
|
Issue: #375
Before when a product was added to an order and subsequently went out of stock before submission, the system incorrectly showed a success message "Your order is being submitted" while leaving the order in draft status. Now it shows "Order can not be submitted! Product not available in requested quantity!".
There are a lot of changes. I will try to explain all of them in the comments below.
The implementation for the Result object was taken from this presentation: https://www.icloud.com/keynote/0x2bfAdGek3Eg-I_2kIomD-6g#Controller_refactoring (it lacks notes but you can check slides 7 and 17 for "before" and "after")