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

Jump to bottom

Allow admin to remove VAT rate #413

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow admin to remove VAT rate #413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
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
26 changes: 26 additions & 0 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 @@ -69,4 +70,29 @@ def stream_name(cmd)
"Taxes::AvailableVatRate$#{cmd.vat_rate.code}"
end
end

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

def call(cmd)
raise VatRateNotExists unless catalog.vat_rate_by_code(cmd.vat_rate_code)

event_store.publish(available_vat_rate_removed_event(cmd), stream_name: stream_name(cmd))
end

private

attr_reader :event_store, :catalog

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)

@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

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