stefankroes / ancestry

Organise ActiveRecord model into a tree structure
MIT License
3.71k stars 458 forks source link

Extensible orphan_strategy #658

Closed kbrock closed 1 year ago

kbrock commented 1 year ago

This introduces 2 ways for developers to extend the built in orphan strategies.

Fixes #142

/cc @brendon Here is custom orphan strategies.

After implementing this, I'm thinking of keeping :none and dropping the custom string.

As a developer, what is your take?


Turning orphan strategy handling off

class ModelBefore
  def apply_orphan_strategy # no longer necessary with :none
    # skip
  end
  has_ancestry orphan_strategy: :none
end

class ModelAfter
  has_ancestry orphan_strategy: :none
end

Implementing a custom orphan strategy

module CustomStrategy
  def apply_orphan_strategy_abc
    # custom goodness
  end
end

class ModelAfter1
  includes CustomStrategy
  has_ancestry orphan_strategy: :abc
end

class ModelAfter2
  includes CustomStrategy
  before_destroy :apply_orphan_strategy_abc
  has_ancestry orphan_strategy: :none
end
kbrock commented 1 year ago

Ok, so this stalled.

I think that introducing none makes sense while introducing a naming strategy is too much. The issues with making sure the method is defined first and the difficulty in documentation makes me feel that this is not ready yet.

I will keep this PR intact, and create another PR to introduce none The naming strategy code is smaller and simpler. Not ready to support the strategy yet, but leaving the code in there.

kbrock commented 1 year ago

update:

kbrock commented 1 year ago

update:

brendon commented 1 year ago

Looks good :) When I'm upgrading next, I'll give it a go. It might be that the custom orphan strategy would need to be defined immediately after has_ancestry to ensure callbacks are still in the correct order in some apps :)

kbrock commented 1 year ago

Actually, the custom method needs to be defined before the has_ancestry. So just adding an include or subclassing a model would work.

But in looking through github, the overrides for this method were almost all to disable it. There was one that moved this to a background task. (was thinking of changing these methods to pass an object, but the reliance on changed makes it tricky)

brendon commented 1 year ago

I think in my case it was the order of deletion but I think that was also fixed recently so I might not need the custom method anyway. I've not started moving to Rails 7 yet but when I do, this gem will get upgraded :D

kbrock commented 1 year ago

@brendon Thanks. That info helps me.

Someone asked about when this will ship. I'm probably going to backport this, as I am making (slow) progress towards converting many functions to sql only.