Allow creating custom vat rates #377

marlena-b · 2024-08-13T15:20:37Z

Issue: #174

This PR lets users create custom VAT rates. Previously, VAT rates were hardcoded in the configuration files, but now users can add their own. It is not possible to add two VAT rates with the same code (it would be very confusing if we had duplicates).

In my changes VAT rate codes can be strings, like "20.0 % S" or "5.0 % R" (source). However, when adding a product, the system still checks if the VAT rate code is a number, which breaks the flow for vat rates with string names. I’ll fix that in a separate PR, along with some confusing naming, to keep this one focused.

Nagranie.z.ekranu.2024-08-13.o.16.52.42.mov

netlify · 2024-08-13T15:20:55Z

❌ Deploy Preview for ecommerce-events failed.

Name	Link
🔨 Latest commit	`265b7c2`
🔍 Latest deploy log	https://app.netlify.com/sites/ecommerce-events/deploys/66c31a99f3cb3a0008ecc40f

marlena-b · 2024-08-13T15:25:05Z

rails_application/test/test_helper.rb

@@ -81,11 +81,16 @@ def register_customer(name)
  end

  def register_product(name, price, vat_rate)
+    add_available_vat_rate(vat_rate)


Do you think this is fine?
If two products will be created with the same vat rate then this will attempt to create two vat rates with the same code. The second attempt will fail and return 422 response. This does not interfere with test logic but I'd like to know if making this call every time when we register product is fine.

The other option is to change every test using register_product so they also explicitly create vat rates.

I am afraid that having such solution would lead to hacking. Also, it would make the method name invalid.

I'd prefer if the add_available_vat_rate is called just once. In fact, all we need is to have available rates defined before tests are run.

Thanks 👍 I didn't know how to do it in a way that it is called only once but I added it to each test needing it (inside a setup block).

marlena-b · 2024-08-13T15:27:01Z

rails_application/app/read_models/vat_rates/configuration.rb

+    def to_value
+      Infra::Types::VatRate.new(code: code, rate: rate)
+    end


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

marlena-b · 2024-08-13T15:29:20Z

ecommerce/taxes/lib/taxes/services.rb

+
+    def stream_name(cmd)
+      "Taxes::AvailableVatRate$#{cmd.vat_rate.code}"


I'm using code instead of uuid in the stream name.
This helps ensure uniqueness of vat rates and simplifies some logic.

Please let me know what you think.

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.

marlena-b · 2024-08-13T16:03:11Z

ecommerce/taxes/test/vat_rate_catalog_test.rb

+        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


I don't quite like this test but mutant complained without it:

@event_store .read .stream("Taxes::AvailableVatRate$#{vat_rate_code}") .to_a .any?

Mutant indicated that this code passes all the tests without the stream filter.

I think the test if fine, but the name of it could be improved.

I had to change the functionality of VatRateCatalog so I removed these tests.

lukaszreszke · 2024-08-14T12:26:26Z

ecommerce/taxes/lib/taxes/product.rb

-    def set_vat_rate(vat_rate)
-      raise VatRateNotApplicable unless vat_rate_applicable?(vat_rate)
+    def set_vat_rate(vat_rate, catalog)
+      raise VatRateNotApplicable unless vat_rate_applicable?(vat_rate.code, catalog)


The validation checking if the vat rate is available should happen outside of the domain model

Thanks @lukaszreszke! Can you help me figure out where to put it? Is command handler a better place for it?

Yes - command handler is better place for checking such a logic.

marlena-b · 2024-08-14T12:37:05Z

rails_application/app/controllers/available_vat_rates_controller.rb

+    attr_reader :code, :rate
+
+    def initialize(params)
+      @code = params[:code]
+      @rate = params[:rate]
+    end


same as: https://github.com/RailsEventStore/ecommerce/pull/378/files#r1716804177

lukaszreszke · 2024-08-14T12:59:32Z

rails_application/app/controllers/products_controller.rb

-  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


I don't think that the controller should validate if the vat rate exists

Especially as it happens in the domain model (which is also not correct).

Would you move it to the SetVatRateHandler?

marlena-b · 2024-08-16T15:07:00Z

rails_application/app/controllers/products_controller.rb

+  rescue ProductCatalog::AlreadyRegistered
+    flash[:notice] = "Product was already registered"
+    render "new"
+  rescue Taxes::VatRateNotApplicable
+    flash[:notice] = "Selected VAT rate is not applicable"
+    render "new"


I extracted this because it was inside a transaction before.

lukaszreszke · 2024-08-18T18:04:06Z

ecommerce/taxes/test/taxes_test.rb

      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 })


small tip, you could do

vat_rate_set = VatRateSet.new(data: { product_id:, vat_rate: })

instead :)

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 :)

lukaszreszke · 2024-08-18T18:05:20Z

rails_application/app/read_models/vat_rates/configuration.rb

+  class AvailableVatRate < ApplicationRecord
+    self.table_name = "available_vat_rates"
+
+    def to_value


It is not used anymore, you can remove this method in next PR :)

I already removed it :)

Allow creating custom vat rates

9c1a986

marlena-b commented Aug 13, 2024

View reviewed changes

marlena-b marked this pull request as ready for review August 13, 2024 16:05

lukaszreszke reviewed Aug 14, 2024

View reviewed changes

Fix form object

3c091de

marlena-b commented Aug 14, 2024

View reviewed changes

lukaszreszke reviewed Aug 14, 2024

View reviewed changes

Implement CR suggestions

ec3cc83

marlena-b commented Aug 16, 2024

View reviewed changes

lukaszreszke reviewed Aug 18, 2024

View reviewed changes

lukaszreszke approved these changes Aug 18, 2024

View reviewed changes

Remove unused to_value method

265b7c2

marlena-b merged commit 76954aa into RailsEventStore:master Aug 19, 2024
0 of 4 checks passed

marlena-b mentioned this pull request Aug 19, 2024

Revert "Allow creating custom vat rates" #382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow creating custom vat rates #377

Allow creating custom vat rates #377

marlena-b commented Aug 13, 2024

netlify bot commented Aug 13, 2024 •

edited

Loading

marlena-b Aug 13, 2024

lukaszreszke Aug 14, 2024

marlena-b Aug 16, 2024

marlena-b Aug 13, 2024

marlena-b Aug 13, 2024

lukaszreszke Aug 14, 2024 •

edited

Loading

marlena-b Aug 13, 2024

lukaszreszke Aug 14, 2024

marlena-b Aug 16, 2024

lukaszreszke Aug 14, 2024

marlena-b Aug 14, 2024

lukaszreszke Aug 14, 2024

marlena-b Aug 16, 2024

marlena-b Aug 14, 2024

lukaszreszke Aug 14, 2024

lukaszreszke Aug 14, 2024

marlena-b Aug 16, 2024

marlena-b Aug 16, 2024

lukaszreszke Aug 18, 2024

marlena-b Aug 19, 2024

lukaszreszke Aug 18, 2024

marlena-b Aug 19, 2024


		def stream_name(cmd)
		"Taxes::AvailableVatRate$#{cmd.vat_rate.code}"

Allow creating custom vat rates #377

Allow creating custom vat rates #377

Conversation

marlena-b commented Aug 13, 2024

netlify bot commented Aug 13, 2024 • edited Loading

❌ Deploy Preview for ecommerce-events failed.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszreszke Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Aug 13, 2024 •

edited

Loading

lukaszreszke Aug 14, 2024 •

edited

Loading