Allow admin to remove VAT rate

[marlena-b]

Issue: #408

This PR allows admin to delete VAT Rate.

Nagranie.z.ekranu.2024-10-9.o.14.09.11.mov

[netlify]

❌ Deploy Preview for ecommerce-events failed.

Name	Link
🔨 Latest commit	795e676
🔍 Latest deploy log	https://app.netlify.com/sites/ecommerce-events/deploys/6710be4bb7e2c80008ab5241

[marlena-b]

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.

[lukaszreszke]

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

[marlena-b]

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

[lukaszreszke]

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)

[marlena-b]

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.

[lukaszreszke]

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?

[marlena-b]

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.

[lukaszreszke]

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

[marlena-b]

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