expectedbehavior / acts_as_archival

An ActiveRecord plugin for atomic archiving and unarchiving of object trees. Inspired by ActsAsParanoid and PermanentRecord
MIT License
128 stars 23 forks source link

Option to not archive dependent records #35

Open aried3r opened 7 years ago

aried3r commented 7 years ago

Hey!

Would you be open to adding an option to not archive dependent records? We might have a specific use case in our app, but we also want to reduce complexity, for example, if A has_many Bs and A is archived, alls B's get archived. If I separately unarchive alls Bs, A does not get unarchived automatically. Of course we can do that ourselves, but we'd rather save us all the trouble and just archive A and display that visually.

Maybe as an option to acts_as_archival, acts_as_archival archive_dependent: false.

Of course, this would be an all-or-nothing option, as in you wouldn't be able to configure that not all Bs should be archived, but A also has_many Cs, which you would want to archive.

If you agree to this option, I'd do the all-or-nothing option which is optional of course. I found act_on_archivals but I don't think there's a way to override it?

janxious commented 7 years ago

I'm not sure I understand the use case you're describing exactly. Mind writing out a CLI example as in the README or a mintest test case (or two) that demonstrate the intent? Not opposed to the idea of more options for controlling (un)archiving.

Another option might be to add options to the #archive!/#unarchive! methods. Like Thing.archive!(archive_dependent: false). Or perhaps an option that takes a block like Thing.archive!(except: -> { whatever })

I found act_on_archivals but I don't think there's a way to override it?

For the purposes of client (rather than AAA library) code, I think you would override one or both of these methods and implement your own subclass of AssociationOperation::Base. I think that's probably the way to handle this "(un)arvchive except these" case, but I might be trying to overuse an abstraction.

aried3r commented 6 years ago

I'm not sure I understand the use case you're describing exactly. Mind writing out a CLI example as in the README or a mintest test case (or two) that demonstrate the intent?

Sure!

class Hole < ActiveRecord::Base
  acts_as_archival
  has_many :rats, dependent: :destroy
end

class Rat < ActiveRecord::Base
  acts_as_archival
end

h = Hole.create
10.times { h.rats.create }
h.archive! # Archives `h` and its 10 rats
h.unarchive! # Unarchives `h` and its 10 rats

# Proposal
h = Hole.create
10.times { h.rats.create }
h.archive!(archive_dependent: false) # Archives just `h`
h.unarchive!(unarchive_dependent: false) # Unarchives just `h`

I think you would override one or both of these methods and implement your own subclass of AssociationOperation::Base

I'll look into that, thanks! But that would lead to an all-or-nothing solution, right? I cannot decide on a per-record basis what should happen.

janxious commented 6 years ago

Ultimately having either or both options could be implemented all-or-nothing or per record. I am relatively sure making more options would require some refactoring to support multiple AssociationOperation::Base-based strategies when doing archiving and unarchiving no matter what the API ends up being.

Your latest proposal code looks fairly reasonable and I get now what you are attempting to do. Your initial proposal had: acts_as_archival archive_dependent: false. I think a more rails-ish way of doing this would be to add options to the acts_as_archival method to skip associations you don't want to be treated as archival. It should be noted that if they don't define archived_at and archive_number they will be skipped anyway.

aried3r commented 6 years ago

I think a more rails-ish way of doing this would be to add options to the acts_as_archival method to skip associations you don't want to be treated as archival.

Could you elaborate on that or give an example?

It should be noted that if they don't define archived_at and archive_number they will be skipped anyway.

Yeah, I think it's a special case the way we use "archiving" in our product right now. Both parent and children can be archived, but archiving the parent doesn't necessarily imply archiving the children.

janxious commented 6 years ago

elaborate on that or give an example

I was thinking something like:

acts_as_archival archive_dependents: false # ignore all dependents
# or
acts_as_archival archive_dependents: [:class1s, :class3s, :etcs] # allowlist
# and/or
acts_as_archival ignore_dependents: [:class2s, :class4s, :etcs] # denylist
# and/or
acts_as_archival ignore_dependents: false # default behavior, pay attention to all dependents

I think it's a special case the way we use "archiving"

I know of at least one other company who did the same thing. I believe they just defined their own #archive methods on classes that needed special functionality. That said, I think this is a reasonable thing to make a first class citizen of the gem if we can find a good version of it.