westonganger / paper_trail-association_tracking

Plugin for the PaperTrail gem to track and reify associations
MIT License
128 stars 38 forks source link

Add support for custom version association class with separate database connection #46

Closed sobrinho closed 3 months ago

sobrinho commented 1 year ago

Fixes https://github.com/westonganger/paper_trail-association_tracking/issues/45

sobrinho commented 1 year ago

This is a draft, still needs to be tested.

sobrinho commented 5 months ago

@westonganger ping :)

westonganger commented 5 months ago

@sobrinho I don't see any documentation for this. Can you write some in the readme. Then it should be easier to review.

sobrinho commented 5 months ago

@westonganger added a small example to README.md.

Is that what you are looking for?

This is the same as papertrail has for the version class, we have a use case like this :

class Product < ApplicationRecord
  has_paper_trail(
    versions: { class_name: "ProductVersion" }, # papertrail has this native
    version_associations: { class_name: "ProductVersionAssociation" } # this PR will add this one
  )
end
westonganger commented 5 months ago

Ok looks alright. Can you also add a changelog entry for this

sobrinho commented 5 months ago

@westonganger added

westonganger commented 5 months ago

Sorry to keep asking for more. But can you actually write one or two tests for this feature?

westonganger commented 4 months ago

@sobrinho ping

sobrinho commented 4 months ago

@westonganger will do today or tomorrow

sobrinho commented 3 months ago

@westonganger can you review this again?

I had to change the implementation a little bit and I'm not 100% sure this is in the right direction.

sobrinho commented 3 months ago

Even if I remove the PaperTrailAssociationTracking::VersionConcern entirely, specs still passes.

So, looks like after this we are decoupling Version and VersionAssociation entirely.

westonganger commented 3 months ago

Even if I remove the PaperTrailAssociationTracking::VersionConcern entirely, specs still passes.

So, looks like after this we are decoupling Version and VersionAssociation entirely.

So that should mean we would also have the option to modify VersionAssociation to connect to a new database and not have to specify it per model? (which is good/desirable)

sobrinho commented 3 months ago

Yes, the idea here is that version association class is a completely separate entity from the version class itself.

And if we remove the PaperTrailAssociationTracking::VersionConcern, we are still good.

westonganger commented 3 months ago

Merged. Thanks for your contribution.