I changed the name to Destroy instead of DestroyAutomation as we already have the namespace indicating this is DiscourseAutomation, it seemed redundant.

Yes I'm always mixed on this, to me a namespace, is a namespace and is not conveying which resource is going to be impacted... but Im fine with the change

It’s not necessarily a resource, the idea is more of using a namespace describing a business concept. But then, most of the model names are business concepts.

I moved the policy to be the first step, as it only relies on guardian. So we can stop the execution flow even before validating any param or fetching any model.

All the steps are grouped together, I know you don’t like it for the ones having blocks, it’s not to annoy you, I swear 😅 I saw that originally you added blank lines around params, but not around transaction. One thing I’d like for us to do is defining a consistent rule on this. This way, we could create a rubocop rule that will apply this and possibly even autocorrect the format.

So it boils down to choosing between having something like:

  policy :can_destroy_automation
  params do
    attribute :automation_id, :integer
    validates :automation_id, presence: true
  end
  model :automation
  transaction do
    step :log_action
    step :destroy_automation
  end

versus

  policy :can_destroy_automation

  params do
    attribute :automation_id, :integer
    validates :automation_id, presence: true
  end

  model :automation

  transaction do
    step :log_action
    step :destroy_automation
  end

I prefer the first version, I think you prefer the second one, but my goal is to have consistency so, in the end, I won’t care that much.

Yes I very much prefer the second one, but feel free to make a quick poll in dev-xp channel, no need to ask everyone

Ok so the second one won 😅

As the parameter names are 1-to-1 with the model, we can simply use #slice here.

This is not necessary, as the default subject provided by RSpec is described_class.new.

We were missing a test to make sure the contract is properly called inside the service.

I added a context for the happy path (as we usually do this), but I moved it after the failing examples. That’s something I can move back, no worries. Here also, I was wondering if we should decide on which order we should enforce. (happy path first then failing examples, or possible failing examples, then at the end you have the happy path).

👍 No strong opinions on this

Ok, let’s roll with this then.