-
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
Allow creating custom vat rates #377
Changes from 2 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,12 +1,14 @@ | ||
module Taxes | ||
VatRateAlreadyExists = Class.new(StandardError) | ||
class SetVatRateHandler | ||
def initialize(event_store) | ||
@repository = Infra::AggregateRootRepository.new(event_store) | ||
@catalog = VatRateCatalog.new(event_store) | ||
end | ||
|
||
def call(cmd) | ||
@repository.with_aggregate(Product, cmd.product_id) do |product| | ||
product.set_vat_rate(cmd.vat_rate) | ||
product.set_vat_rate(cmd.vat_rate, @catalog) | ||
end | ||
end | ||
end | ||
|
@@ -34,4 +36,34 @@ def stream_name(order_id) | |
"Taxes::Order$#{order_id}" | ||
end | ||
end | ||
end | ||
|
||
class AddAvailableVatRateHandler | ||
def initialize(event_store) | ||
@catalog = VatRateCatalog.new(event_store) | ||
@event_store = event_store | ||
end | ||
|
||
def call(cmd) | ||
raise VatRateAlreadyExists if catalog.vat_rate_available?(cmd.vat_rate.code) | ||
|
||
event_store.publish(available_vat_rate_added_event(cmd), stream_name: stream_name(cmd)) | ||
end | ||
|
||
private | ||
|
||
attr_reader :event_store, :catalog | ||
|
||
def available_vat_rate_added_event(cmd) | ||
AvailableVatRateAdded.new( | ||
data: { | ||
available_vat_rate_id: cmd.available_vat_rate_id, | ||
vat_rate: cmd.vat_rate | ||
} | ||
) | ||
end | ||
|
||
def stream_name(cmd) | ||
"Taxes::AvailableVatRate$#{cmd.vat_rate.code}" | ||
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'm using Please let me know what you think. 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. Technically it is a good choice. However I am wondering if the model is correct. But I don't have an answer yet. It is good for first iteration I guess. |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,32 +3,59 @@ | |
module Taxes | ||
class TaxesTest < Test | ||
def test_setting_available_vat_rate | ||
vat_rate = Infra::Types::VatRate.new(code: "50", rate: 50) | ||
add_available_vat_rate(vat_rate) | ||
|
||
product_id = SecureRandom.uuid | ||
vat_rate_set = VatRateSet.new(data: { product_id: product_id, vat_rate: available_vat_rate }) | ||
vat_rate_set = VatRateSet.new(data: { product_id: product_id, vat_rate: vat_rate }) | ||
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. small tip, you could do
instead :) 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 would prefer to leave this as is for now, as it is consistent with the rest of the code in the project. In the future we can add a rubocop rule to enforce style guides :) |
||
assert_events("Taxes::Product$#{product_id}", vat_rate_set) do | ||
set_vat_rate(product_id, available_vat_rate) | ||
set_vat_rate(product_id, vat_rate) | ||
end | ||
end | ||
|
||
def test_setting_unavailable_vat_rate_should_raise_error | ||
product_id = SecureRandom.uuid | ||
unavailable_vat_rate = Infra::Types::VatRate.new(code: "20", rate: 20) | ||
|
||
assert_raises(Product::VatRateNotApplicable) do | ||
set_vat_rate(product_id, unavailable_vat_rate) | ||
end | ||
end | ||
|
||
def test_determining_vat_rate | ||
vat_rate = Infra::Types::VatRate.new(code: "50", rate: 50) | ||
add_available_vat_rate(vat_rate) | ||
|
||
order_id = SecureRandom.uuid | ||
product_id = SecureRandom.uuid | ||
another_product_id = SecureRandom.uuid | ||
|
||
set_vat_rate(product_id, available_vat_rate) | ||
vat_rate_determined = VatRateDetermined.new(data: { order_id: order_id, product_id: product_id, vat_rate: available_vat_rate }) | ||
set_vat_rate(product_id, vat_rate) | ||
vat_rate_determined = VatRateDetermined.new(data: { order_id: order_id, product_id: product_id, vat_rate: vat_rate }) | ||
assert_events("Taxes::Order$#{order_id}", vat_rate_determined) do | ||
determine_vat_rate(order_id, product_id, available_vat_rate) | ||
determine_vat_rate(order_id, product_id, vat_rate) | ||
end | ||
assert_events("Taxes::Order$#{order_id}") do | ||
determine_vat_rate(order_id, another_product_id, available_vat_rate) | ||
determine_vat_rate(order_id, another_product_id, vat_rate) | ||
end | ||
end | ||
|
||
def test_adding_available_vat_rate | ||
available_vat_rate_id = SecureRandom.uuid | ||
vat_rate = Infra::Types::VatRate.new(code: "50", rate: 50) | ||
available_vat_rate_added = AvailableVatRateAdded.new(data: { available_vat_rate_id: available_vat_rate_id, vat_rate: vat_rate }) | ||
|
||
assert_events("Taxes::AvailableVatRate$#{vat_rate.code}", available_vat_rate_added) do | ||
add_available_vat_rate(vat_rate, available_vat_rate_id) | ||
end | ||
end | ||
|
||
def test_should_not_allow_for_double_registration | ||
vat_rate = Infra::Types::VatRate.new(code: "50", rate: 50) | ||
add_available_vat_rate(vat_rate) | ||
|
||
assert_raises(VatRateAlreadyExists) do | ||
add_available_vat_rate(vat_rate) | ||
end | ||
end | ||
|
||
|
@@ -42,12 +69,8 @@ def determine_vat_rate(order_id, product_id, vat_rate) | |
run_command(DetermineVatRate.new(order_id: order_id, product_id: product_id, vat_rate: vat_rate)) | ||
end | ||
|
||
def available_vat_rate | ||
Configuration.available_vat_rates.first | ||
end | ||
|
||
def unavailable_vat_rate | ||
Infra::Types::VatRate.new(code: "50", rate: 50) | ||
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 | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
require_relative "test_helper" | ||
|
||
module Taxes | ||
class VatRateCatalogTest < Test | ||
class VatRateAvailableTest < VatRateCatalogTest | ||
def test_returns_true_when_vat_rate_is_available | ||
vat_rate = Infra::Types::VatRate.new(code: "50", rate: 50) | ||
add_available_vat_rate(vat_rate) | ||
|
||
assert catalog.vat_rate_available?(vat_rate.code) | ||
end | ||
|
||
def test_returns_false_when_vat_rate_is_not_available | ||
vat_rate = Infra::Types::VatRate.new(code: "50", rate: 50) | ||
|
||
refute catalog.vat_rate_available?(vat_rate.code) | ||
end | ||
|
||
def test_returns_false_when_vat_rate_is_not_available_even_when_other_vat_rates_are_available | ||
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 don't quite like this test but mutant complained without it:
Mutant indicated that this code passes all the tests without the 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 the test if fine, but the name of it could be improved. 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 had to change the functionality of VatRateCatalog so I removed these tests. |
||
vat_rate = Infra::Types::VatRate.new(code: "50", rate: 50) | ||
add_available_vat_rate(vat_rate) | ||
|
||
refute catalog.vat_rate_available?("60") | ||
end | ||
end | ||
|
||
|
||
private | ||
|
||
def catalog | ||
VatRateCatalog.new(@event_store) | ||
end | ||
|
||
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 | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
class AvailableVatRatesController < ApplicationController | ||
class AvailableVatRateForm | ||
include ActiveModel::Model | ||
include ActiveModel::Validations | ||
|
||
attr_reader :code, :rate | ||
|
||
def initialize(params) | ||
@code = params[:code] | ||
@rate = params[:rate] | ||
end | ||
Comment on lines
+6
to
+11
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. |
||
|
||
validates :code, presence: true | ||
validates :rate, presence: true, numericality: { only_numeric: true, greater_than: 0 } | ||
end | ||
|
||
def new | ||
end | ||
|
||
def create | ||
available_vat_rate_id = SecureRandom.uuid | ||
available_vat_rate_form = AvailableVatRateForm.new(available_vat_rate_params) | ||
|
||
unless available_vat_rate_form.valid? | ||
return render "new", locals: { errors: available_vat_rate_form.errors }, status: :unprocessable_entity | ||
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 | ||
redirect_to available_vat_rates_path, notice: "VAT rate was successfully created" | ||
end | ||
|
||
def index | ||
@available_vat_rates = VatRates::AvailableVatRate.all | ||
end | ||
|
||
private | ||
|
||
def add_available_vat_rate(code, rate, available_vat_rate_id) | ||
command_bus.(add_available_vat_rate_cmd(code, rate, available_vat_rate_id)) | ||
end | ||
|
||
def add_available_vat_rate_cmd(code, rate, available_vat_rate_id) | ||
Taxes::AddAvailableVatRate.new( | ||
available_vat_rate_id: available_vat_rate_id, | ||
vat_rate: Infra::Types::VatRate.new(code: code, rate: rate) | ||
) | ||
end | ||
|
||
def available_vat_rate_params | ||
params.permit(:code, :rate) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,8 +91,8 @@ def set_future_product_price(product_id, price, valid_since) | |
command_bus.(set_product_future_price_cmd(product_id, price, valid_since)) | ||
end | ||
|
||
def set_product_vat_rate(product_id, vat_rate_code) | ||
vat_rate = Taxes::Configuration.available_vat_rates.find{|rate| rate.code == vat_rate_code} | ||
def set_product_vat_rate(product_id, vat_rate) | ||
vat_rate = VatRates::AvailableVatRate.find_by!(code: vat_rate).to_value | ||
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 don't think that the controller should validate if the vat rate exists 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. Especially as it happens in the domain model (which is also not correct). Would you move it to the 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. Done 👍 |
||
command_bus.(set_product_vat_rate_cmd(product_id, vat_rate)) | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
module VatRates | ||
class AddAvailableVatRate | ||
def call(event) | ||
AvailableVatRate.create!( | ||
uid: event.data.fetch(:available_vat_rate_id), | ||
code: event.data.fetch(:vat_rate).fetch(:code), | ||
rate: event.data.fetch(:vat_rate).fetch(:rate) | ||
) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
module VatRates | ||
class AvailableVatRate < ApplicationRecord | ||
self.table_name = "available_vat_rates" | ||
|
||
def to_value | ||
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. It is not used anymore, you can remove this method in next PR :) 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 already removed it :) |
||
Infra::Types::VatRate.new(code: code, rate: rate) | ||
end | ||
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. Other models don't have any methods but this was very useful. I can remove and refactor if we don't want to have any methods in models |
||
end | ||
|
||
class Configuration | ||
def call(event_store) | ||
event_store.subscribe(AddAvailableVatRate, to: [Taxes::AvailableVatRateAdded]) | ||
end | ||
end | ||
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.
The validation checking if the vat rate is available should happen outside of the domain model
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.
Thanks @lukaszreszke! Can you help me figure out where to put it? Is command handler a better place for it?
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.
Yes - command handler is better place for checking such a logic.
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.
Done!