-
Notifications
You must be signed in to change notification settings - Fork 72
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.
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
Better error when submitting order with out of stock product #390
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
.ruby-version | ||
.tool-versions |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
module Processes | ||
class ReservationProcessFailed < Infra::Event | ||
attribute :order_id, Infra::Types::UUID | ||
attribute :unavailable_products, Infra::Types::Array.of(Infra::Types::UUID) | ||
end | ||
|
||
class ReservationProcessSuceeded < Infra::Event | ||
attribute :order_id, Infra::Types::UUID | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,14 +10,7 @@ def call(event) | |
state = build_state(event) | ||
case event.event_type | ||
when 'Ordering::OrderSubmitted' | ||
begin | ||
reserve_stock(state) | ||
rescue Inventory::InventoryEntry::InventoryNotAvailable | ||
release_stock(state) | ||
reject_order(state) | ||
else | ||
accept_order(state) | ||
end | ||
order_side_effects(state) { reserve_stock(state) } | ||
when 'Fulfillment::OrderCancelled' | ||
release_stock(state) | ||
when 'Fulfillment::OrderConfirmed' | ||
|
@@ -28,10 +21,29 @@ def call(event) | |
private | ||
|
||
def reserve_stock(state) | ||
unavailable_products = [] | ||
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 | ||
Comment on lines
25
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
if unavailable_products.empty? | ||
event = ReservationProcessSuceeded.new(data: { order_id: state.order_id }) | ||
else | ||
release_stock(state) | ||
event = ReservationProcessFailed.new(data: { order_id: state.order_id, unavailable_products: unavailable_products }) | ||
end | ||
event_store.publish(event, stream_name: stream_name(state.order_id)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ReservationProcess now emits events. We then subscribe to these events in the controller and display proper errors to the user. |
||
end | ||
|
||
def order_side_effects(state) | ||
event_store | ||
.within { yield } | ||
.subscribe(to: ReservationProcessFailed) { reject_order(state) } | ||
.subscribe(to: ReservationProcessSuceeded) { accept_order(state) } | ||
.call | ||
Comment on lines
+41
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
end | ||
|
||
def release_stock(state) | ||
|
@@ -54,8 +66,12 @@ def reject_order(state) | |
command_bus.(Ordering::RejectOrder.new(order_id: state.order_id)) | ||
end | ||
|
||
def stream_name(order_id) | ||
"ReservationProcess$#{order_id}" | ||
end | ||
|
||
def build_state(event) | ||
stream_name = "ReservationProcess$#{event.data.fetch(:order_id)}" | ||
stream_name = stream_name(event.data.fetch(:order_id)) | ||
begin | ||
past_events = event_store.read.stream(stream_name).to_a | ||
last_stored = past_events.size - 1 | ||
|
@@ -93,4 +109,4 @@ def product_reserved(product_id) | |
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,13 +12,12 @@ def new | |
end | ||
|
||
def create | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code in this controller got more and more complex so I extracted it to a service. (the DSL was partly inspired by respond_to method) |
||
result.path(:success) { redirect_to client_order_path(params[:order_id]), notice: "Your order is being submitted" } | ||
result.path(:products_out_of_stock) { |unavailable_products| redirect_to edit_client_order_path(params[:order_id]), | ||
alert: "Order can not be submitted! #{unavailable_products.join(", ")} not available in requested quantity!"} | ||
result.path(:order_is_empty) { redirect_to edit_client_order_path(params[:order_id]), alert: "You can't submit an empty order" } | ||
end | ||
redirect_to client_order_path(params[:order_id]), notice: "Your order is being submitted" | ||
rescue Ordering::Order::IsEmpty | ||
redirect_to edit_client_order_path(params[:order_id]), alert: "You can't submit an empty order" | ||
end | ||
|
||
def show | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,12 +74,13 @@ def remove_item | |
end | ||
|
||
def create | ||
ApplicationRecord.transaction { submit_order(params[:order_id], params[:customer_id]) } | ||
redirect_to order_path(params[:order_id]), notice: "Your order is being submitted" | ||
rescue Ordering::Order::IsEmpty | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
result.path(:success) { redirect_to order_path(params[:order_id]), notice: "Your order is being submitted" } | ||
result.path(:products_out_of_stock) { |unavailable_products| redirect_to edit_order_path(params[:order_id]), | ||
alert: "Order can not be submitted! #{unavailable_products.join(", ")} not available in requested quantity!"} | ||
result.path(:order_is_empty) { redirect_to edit_order_path(params[:order_id]), alert: "You can't submit an empty order" } | ||
result.path(:customer_not_exists) { redirect_to order_path(params[:order_id]), alert: "Order can not be submitted! Customer does not exist." } | ||
end | ||
end | ||
|
||
def expire | ||
|
@@ -113,11 +114,6 @@ def cancel | |
|
||
private | ||
|
||
def submit_order(order_id, customer_id) | ||
command_bus.(Ordering::SubmitOrder.new(order_id: order_id)) | ||
command_bus.(Crm::AssignCustomerToOrder.new(order_id: order_id, customer_id: customer_id)) | ||
end | ||
|
||
def authorize_payment(order_id) | ||
command_bus.call(authorize_payment_cmd(order_id)) | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
class ApplicationService | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Please let me know if you prefer to use some other implementation instead, this is just my proposal. |
||
def self.call(...) | ||
new(...).call | ||
end | ||
|
||
def call | ||
raise NotImplementedError | ||
end | ||
|
||
def event_store | ||
Rails.configuration.event_store | ||
end | ||
|
||
def command_bus | ||
Rails.configuration.command_bus | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
module Orders | ||
class SubmitService < ApplicationService | ||
def initialize(order_id:, customer_id:) | ||
@order_id = order_id | ||
@customer_id = customer_id | ||
end | ||
|
||
def call | ||
success = true | ||
unavailable_products = nil | ||
|
||
event_store | ||
.within { submit_order } | ||
.subscribe(to: Processes::ReservationProcessFailed) do |event| | ||
success = false | ||
unavailable_products = Products::Product.where(id: event.data.fetch(:unavailable_products)).pluck(:name) | ||
end | ||
.call | ||
|
||
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) | ||
Comment on lines
+20
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
end | ||
|
||
private | ||
|
||
attr_reader :order_id, :customer_id | ||
|
||
def submit_order | ||
ActiveRecord::Base.transaction do | ||
command_bus.(Ordering::SubmitOrder.new(order_id: order_id)) | ||
command_bus.(Crm::AssignCustomerToOrder.new(order_id: order_id, customer_id: customer_id)) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
class Result | ||
attr_reader :status, :args | ||
|
||
def initialize(status, *args) | ||
@status = status | ||
@args = args | ||
end | ||
|
||
def path(name, &block) | ||
return unless @status == name.to_sym | ||
|
||
block.call(*@args) | ||
end | ||
Comment on lines
+9
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is also an alternative implementation which uses more metaprogramming but results in a nicer DSL:
and then the DSL looks like this:
Let me know which one you prefer :) |
||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.