Skip to content
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.

Sign up for GitHub

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

Allow admin to remove VAT rate #413

Merged
merged 3 commits into from
Oct 18, 2024
Merged

Allow admin to remove VAT rate #413

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
088a284
Allow admin to remove VAT rate
marlena-b Oct 9, 2024
5b92611
Fix race condition
marlena-b Oct 16, 2024
795e676
refactor
marlena-b Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ecommerce/taxes/.mutant.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,6 @@ matcher:
ignore:
- Taxes::Test*
- Taxes::Configuration*
- Taxes::VatRateCatalog#vat_rate_for
- Taxes::VatRateCatalog#vat_rate_for
- Taxes::AddAvailableVatRateHandler*
Comment on lines +14 to +15
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I excluded them from mutant because I think the amount of weird tests I would have to write does not justify the benefit we get.

- Taxes::RemoveAvailableVatRateHandler*
1 change: 1 addition & 0 deletions ecommerce/taxes/lib/taxes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ def call(event_store, command_bus)
command_bus.register(SetVatRate, SetVatRateHandler.new(event_store))
command_bus.register(DetermineVatRate, DetermineVatRateHandler.new(event_store))
command_bus.register(AddAvailableVatRate, AddAvailableVatRateHandler.new(event_store))
command_bus.register(RemoveAvailableVatRate, RemoveAvailableVatRateHandler.new(event_store))
end
end
end
4 changes: 4 additions & 0 deletions ecommerce/taxes/lib/taxes/commands.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,8 @@ class AddAvailableVatRate < Infra::Command
attribute :available_vat_rate_id, Infra::Types::UUID
attribute :vat_rate, Infra::Types::VatRate
end

class RemoveAvailableVatRate < Infra::Command
attribute :vat_rate_code, Infra::Types::String
end
end
4 changes: 4 additions & 0 deletions ecommerce/taxes/lib/taxes/events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,8 @@ class AvailableVatRateAdded < Infra::Event
attribute :available_vat_rate_id, Infra::Types::UUID
attribute :vat_rate, Infra::Types::VatRate
end

class AvailableVatRateRemoved < Infra::Event
attribute :vat_rate_code, Infra::Types::String
end
end
63 changes: 59 additions & 4 deletions ecommerce/taxes/lib/taxes/services.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module Taxes
VatRateAlreadyExists = Class.new(StandardError)
VatRateNotApplicable = Class.new(StandardError)
VatRateNotExists = Class.new(StandardError)
class SetVatRateHandler
def initialize(event_store)
@repository = Infra::AggregateRootRepository.new(event_store)
Expand Down Expand Up @@ -41,20 +42,32 @@ def stream_name(order_id)
end

class AddAvailableVatRateHandler
include Infra::Retry

def initialize(event_store)
@catalog = VatRateCatalog.new(event_store)
@event_store = event_store
end

def call(cmd)
raise VatRateAlreadyExists if catalog.vat_rate_by_code(cmd.vat_rate.code)
with_retry do
event = last_available_vat_rate_event(cmd.vat_rate.code)
raise VatRateAlreadyExists if event.instance_of?(AvailableVatRateAdded)

event_store.publish(available_vat_rate_added_event(cmd), stream_name: stream_name(cmd))
expected_version = event ? event_store.position_in_stream(event.event_id, stream_name(cmd)) : -1
event_store.publish(available_vat_rate_added_event(cmd), stream_name: stream_name(cmd), expected_version: expected_version)
end
end

private

attr_reader :event_store, :catalog
attr_reader :event_store

def last_available_vat_rate_event(vat_rate_code)
@event_store
.read
.stream("Taxes::AvailableVatRate$#{vat_rate_code}")
.last
end

def available_vat_rate_added_event(cmd)
AvailableVatRateAdded.new(
Expand All @@ -69,4 +82,46 @@ def stream_name(cmd)
"Taxes::AvailableVatRate$#{cmd.vat_rate.code}"
end
end

class RemoveAvailableVatRateHandler
include Infra::Retry

def initialize(event_store)
@event_store = event_store
end

def call(cmd)
with_retry do
event = last_available_vat_rate_event(cmd.vat_rate_code)
raise VatRateNotExists if event.nil?

event_store.publish(
available_vat_rate_removed_event(cmd),
stream_name: stream_name(cmd),
expected_version: event_store.position_in_stream(event.event_id, stream_name(cmd))
)
end
end

private

attr_reader :event_store

def last_available_vat_rate_event(vat_rate_code)
event = event_store
.read
.stream("Taxes::AvailableVatRate$#{vat_rate_code}")
.last

event if event.instance_of?(AvailableVatRateAdded)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the method name does something different than the name declares.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored it a bit. I hope it is better now.

end

def available_vat_rate_removed_event(cmd)
AvailableVatRateRemoved.new(data: { vat_rate_code: cmd.vat_rate_code })
end

def stream_name(cmd)
"Taxes::AvailableVatRate$#{cmd.vat_rate_code}"
end
end
end
12 changes: 8 additions & 4 deletions ecommerce/taxes/lib/taxes/vat_rate_catalog.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,17 @@ def vat_rate_for(product_id)
end

def vat_rate_by_code(vat_rate_code)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside of scope of this task, but this method got me thinking.

This method is called in the AddAvailableVatRateHandler

It is called to check if tax rate with given code already exists. If not, then the handler publishes an event.

First of all, it breaks the tell don't ask rule. In current implementation there is possibility that two AvailableVatRateAdded events will be published. Therefore, the system will be in unexpected state.

The question is if that is a major or minor problem.

Since this if statement has been added for some reason, let's assume it would a major issue if the vat rate code is duplicated.

To reproduce this behaviour you can follow those steps:

rails-application(dev)* fiber = Fiber.new do
rails-application(dev)*   unless event_store.read.stream("AvailableVatRate$#{vat_rate_code}").last
rails-application(dev)*     Fiber.yield
rails-application(dev)*     event_store.publish(AvailableVatRateAdded.new(data: { vat_rate_code: }), stream_name: "AvailableVatRate$#{vat_rate_code}")
rails-application(dev)*   end
rails-application(dev)> end
rails-application(dev)> fiber.resume
  RubyEventStore::ActiveRecord::EventInStream Load (0.6ms)  SELECT "event_store_events_in_streams".* FROM "event_store_events_in_streams" WHERE "event_store_events_in_streams"."stream" = $1 ORDER BY "event_store_events_in_streams"."id" DESC LIMIT $2  [["stream", "AvailableVatRate$60"], ["LIMIT", 1]]
=> nil # <========= The if statement was checked here, there was no event in stream
rails-application(dev)> event_store.publish(AvailableVatRateAdded.new(data: { vat_rate_code: }), stream_name: "AvailableVatRate$#{vat_rate_code}")
rails-application(dev)> event_store.read.stream("AvailableVatRate$#{vat_rate_code}").to_a.count
=> 2

How do you think we could deal with that? (tip: look at the core concepts section in RES documentation)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the implementation of command handlers not to use vat_rate_catalog. Instead, I'm fetching the last event and using the expected_version mechanism to ensure there is no race condition. Is this what was concerning you?

I think my code still breaks tell don't ask principle (bc I'm asking what was the last event version) but I'm not sure how can I do it otherwise.

Thank you for spotting this and please let me know what you think.

Copy link
Collaborator

@lukaszreszke lukaszreszke Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this what was concerning you?

Yes, exactly. Good job :)

I think that code is in ok shape now. I wouldn't optimize it anymore at this stage.

There are no business rules or invariants that would justify introducing an aggregate here. I would leave it as it is and observe how this code changes over time.

If, say there is another place that needs the piece of code that: reads the vat rate stream, checks if last vat rate event is X and does something, then I'd think about introducing some concept (not necessarily an aggregate) to keep it together. Perhaps Domain Service?

@event_store
last_available_vat_rate_event = @event_store
.read
.stream("Taxes::AvailableVatRate$#{vat_rate_code}")
.last
&.data
&.fetch(:vat_rate)
&.then { |vat_rate| Infra::Types::VatRate.new(vat_rate) }

if last_available_vat_rate_event.instance_of?(AvailableVatRateAdded)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you only care about the AvailableVatRateAdded event then you could use RES client's api to get that:
@event_store.read.stream("Taxes::AvailableVatRate$#{vat_rate_code}").of_type(AvailableVatRateAdded).last

the if statement wouldn't be necessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually care that the last event is AvailableVatRateAdded and not AvailableVatRateRemoved to ensure that this vat rate wasn’t removed.

last_available_vat_rate_event
.data
.fetch(:vat_rate)
.then { |vat_rate| Infra::Types::VatRate.new(vat_rate) }
end
end
end
end
20 changes: 20 additions & 0 deletions ecommerce/taxes/test/taxes_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,22 @@ def test_should_not_allow_for_double_registration
end
end

def test_removing_available_vat_rate
vat_rate = Infra::Types::VatRate.new(code: "50", rate: 50)
add_available_vat_rate(vat_rate)
available_vat_rate_removed = AvailableVatRateRemoved.new(data: { vat_rate_code: vat_rate.code })

assert_events("Taxes::AvailableVatRate$#{vat_rate.code}", available_vat_rate_removed) do
remove_available_vat_rate(vat_rate.code)
end
end

def test_cannot_remove_non_existing_vat_rate
assert_raises(VatRateNotExists) do
remove_available_vat_rate("50")
end
end

private

def set_vat_rate(product_id, vat_rate_code)
Expand All @@ -72,5 +88,9 @@ def determine_vat_rate(order_id, product_id, vat_rate)
def add_available_vat_rate(vat_rate, available_vat_rate_id = SecureRandom.uuid)
run_command(AddAvailableVatRate.new(available_vat_rate_id: available_vat_rate_id, vat_rate: vat_rate))
end

def remove_available_vat_rate(vat_rate_code)
run_command(RemoveAvailableVatRate.new(vat_rate_code: vat_rate_code))
end
end
end
6 changes: 6 additions & 0 deletions ecommerce/taxes/test/vat_rate_catalog_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ def test_returns_available_vat_rate
def test_returns_nil_when_vat_rate_is_not_available
assert_nil catalog.vat_rate_by_code("60")
end

def test_returns_nil_when_vat_rate_was_removed
run_command(RemoveAvailableVatRate.new(vat_rate_code: "50"))

assert_nil catalog.vat_rate_by_code("50")
end
end

private
Expand Down
23 changes: 19 additions & 4 deletions rails_application/app/controllers/available_vat_rates_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,24 @@ def create
end

add_available_vat_rate(available_vat_rate_form.code, available_vat_rate_form.rate, available_vat_rate_id)
rescue Taxes::VatRateAlreadyExists
flash.now[:notice] = "VAT rate already exists"
render "new", status: :unprocessable_entity
else
rescue Taxes::VatRateAlreadyExists
flash.now[:alert] = "VAT rate already exists"
render "new", status: :unprocessable_entity
else
redirect_to available_vat_rates_path, notice: "VAT rate was successfully created"
end

def index
@available_vat_rates = VatRates::AvailableVatRate.all
end

def destroy
remove_available_vat_rate(params[:vat_rate_code])
redirect_to available_vat_rates_path, notice: "VAT rate was successfully removed"
rescue Taxes::VatRateNotExists
redirect_to available_vat_rates_path, alert: "VAT rate does not exist"
end

private

def add_available_vat_rate(code, rate, available_vat_rate_id)
Expand All @@ -50,6 +57,14 @@ def add_available_vat_rate_cmd(code, rate, available_vat_rate_id)
)
end

def remove_available_vat_rate(vat_rate_code)
command_bus.(remove_available_vat_rate_cmd(vat_rate_code))
end

def remove_available_vat_rate_cmd(vat_rate_code)
Taxes::RemoveAvailableVatRate.new(vat_rate_code: vat_rate_code)
end

def available_vat_rate_params
params.permit(:code, :rate)
end
Expand Down
1 change: 1 addition & 0 deletions rails_application/app/read_models/vat_rates/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class AvailableVatRate < ApplicationRecord
class Configuration
def call(event_store)
event_store.subscribe(AddAvailableVatRate, to: [Taxes::AvailableVatRateAdded])
event_store.subscribe(RemoveAvailableVatRate, to: [Taxes::AvailableVatRateRemoved])
end
end
end
7 changes: 7 additions & 0 deletions rails_application/app/read_models/vat_rates/remove_available_vat_rate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module VatRates
class RemoveAvailableVatRate
def call(event)
AvailableVatRate.destroy_by(code: event.data.fetch(:vat_rate_code))
end
end
end
8 changes: 7 additions & 1 deletion rails_application/app/views/available_vat_rates/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,20 @@
<tr>
<th class="text-left py-2">Code</th>
<th class="text-right py-2">Rate</th>
<th class="text-right py-2"></th>
</tr>
</thead>

<tbody>
<% @available_vat_rates.each do |available_vat_rate| %>
<tr class="border-t">
<td class="py-2"><%= available_vat_rate.code %></td>
<td class="py-2 text-right"><%= available_vat_rate.rate %></td>
<td class="text-right py-2"><%= available_vat_rate.rate %></td>
<td class="text-right py-2">
<%= form_with url: available_vat_rates_path, method: :delete do |f| %>
<%= f.hidden_field :vat_rate_code, value: available_vat_rate.code %>
<%= f.submit 'Delete' %>
<% end %>
Comment on lines +26 to +29
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used form_with instead of link_to because I needed to send vat_rate_code, which is our identifier. The vat_rate_code can contain a dot, e.g., 10.0. Using link_to the code was added as a parameter to the url, which caused issues with Turbo. It appears that Turbo does not handle URLs with dots in the path correctly (see hotwired/turbo#608).

To avoid this issue, I decided to send the parameter through the form instead.

</tr>
<% end %>
</tbody>
Expand Down
1 change: 1 addition & 0 deletions rails_application/config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
end

resources :available_vat_rates, only: [:new, :create, :index]
delete "/available_vat_rates", to: "available_vat_rates#destroy"

post :login, to: "client/clients#login"
get :logout, to: "client/clients#logout"
Expand Down
10 changes: 9 additions & 1 deletion rails_application/test/integration/available_vat_rates_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ def test_happy_path

assert_equal "VAT rate was successfully created", flash[:notice]
assert_select "h1", "VAT Rates"

delete "/available_vat_rates",
params: {
"vat_rate_code" => "10.0"
}
follow_redirect!

assert_equal "VAT rate was successfully removed", flash[:notice]
end

def test_validation_blank_errors
Expand Down Expand Up @@ -58,6 +66,6 @@ def test_vat_rate_already_exists
}

assert_response :unprocessable_entity
assert_select "#notice", "VAT rate already exists"
assert_select "#alert", "VAT rate already exists"
end
end
Loading