Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple associated audits #406

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fatkodima
Copy link
Contributor

@fatkodima fatkodima commented Jan 17, 2018

This PR adds the ability to associate a model with multiple parent models. Something like the following:

class Mother < ActiveRecord::Base
  audited
  has_associated_audits
end

class Father < ActiveRecord::Base
  audited
  has_associated_audits
end

class Child < ActiveRecord::Base
  audited associated_with: [:mother, :father]
end

This introduces a bit of backwards incompatibility (like renaming associated to associates, for example) so I'm wondering how we should handle this: a) extensively deprecating (how? - also remains as a question then) or b) do not worry too much and release in next major version or c) other variant?

Related #337.
Related #209.
Related PR #342 - discussion on why this should be better to implement using separate table.

@zachfeldman
Copy link

Hey @fatkodima I just started using this gem and decided to use your PR as a starting point. Generally things are working!

I noticed that if I ask for associates, I get the full list of associated models, even those that didn't change with that audit. For instance, I have a Reservation that I've changed the house_id of from one house to another. The associates field returns the reservation's associated guest record even though it didn't change in addition to the new house record.

I also noticed the associated_audits method stayed the same...should that be like, associates_audits now?

@zachfeldman
Copy link

Hey, I've been rolling with this PR for a month or two and will get it into production soon. Could we possibly merge it into master?

@misaka
Copy link

misaka commented Apr 10, 2019

Hi there. Also interested in seeing this merged, aside from the conflicts is it just waiting for 5.0 to be ready?

@sumanawal
Copy link

Hi, I am having an issue while setting up this gem with above branch. Application stack:

Rails: 5.2.0
Ruby: 2.5.1

I am getting following error while performing bundle install command after adding gem 'audited', git: 'https://github.com/fatkodima/audited.git', branch: 'multiple-assoc-audits' in Gemfile.

Bundler could not find compatible versions for gem "activerecord":
  In snapshot (Gemfile.lock):
    activerecord (= 5.2.3)

  In Gemfile:
    activerecord-postgis-adapter was resolved to 5.2.2, which depends on
      activerecord (~> 5.1)

    ancestry was resolved to 3.0.7, which depends on
      activerecord (>= 3.2.0)

    apartment-activejob was resolved to 0.0.1, which depends on
      apartment was resolved to 2.2.1, which depends on
        activerecord (< 6.0, >= 3.1.2)

    audited was resolved to 4.6.0, which depends on
      activerecord (< 5.2, >= 4.0)

    blamer was resolved to 4.1.0, which depends on
      activerecord

    paranoia was resolved to 2.4.2, which depends on
      activerecord (< 6.1, >= 4.0)

    rails (~> 5.2.0) was resolved to 5.2.3, which depends on
      activerecord (= 5.2.3)

    activerecord-postgis-adapter was resolved to 5.2.2, which depends on
      rgeo-activerecord (~> 6.0) was resolved to 6.2.0, which depends on
        activerecord (>= 5.0)

Running `bundle update` will rebuild your snapshot from scratch, using
only
the gems in your Gemfile, which may resolve the conflict.

I am trying to implement this gem but without this branch, it's no use for me. So, I like to ask in which branch this pull request will be merged and when are you guys planning to release a version including these changes.

Thank you

@markaschneider
Copy link

I'd be willing to bet that in the vast majority of cases where there's a need for more than one associated, the need is specifically for 2 (supporting HABTM). If so, would it be better just to add 2 more fields to the audits table and allow a model to be associated with up to 2 parents?

@mikeni
Copy link

mikeni commented Jul 26, 2023

any progress on this?

@fatkodima
Copy link
Contributor Author

I am here. This was created quite a while ago 😅 If people will accept the PR, I will update it.

@silent-e
Copy link

silent-e commented Dec 1, 2023

+1 for getting some motion on this since I could really use it. I'm getting ready to start testing this solution. It's not exactly like what is being suggested here but could work for a through model having two belongs_to associations.

However I believe @fatkodima that the conflicts need to be resolved first before it can be merged.

@gang-qiu
Copy link

gang-qiu commented Dec 21, 2023

+1 this PR would be incredibly useful if accepted

@silent-e curious how you were able to adapt the HABTM solution for the simpler use case here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants