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

Fix paper trail compatibility #298

Closed

Conversation

etagwerker
Copy link

Hello!

This is a proposed solution to issue #206, for the case where a model has both has_paper_trail and translates :versioning => true

I've added the case to the set of tests.

The code will only delegate #version and #versions to the translation class if the model doesn't already have paper_trail enabled.

I believe there will be a problem if you define your model like this:

class Paper < ActiveRecord::Base
  translates :description, :versioning => true
  has_paper_trail :only => [:name]

The problem will be that #version and #versions will call the translation methods.

If you define it like this:

class Paper < ActiveRecord::Base
  has_paper_trail :only => [:name]
  translates :description, :versioning => true

It should work alright.

I understand it's not ideal to have to order the statements, but I wanted to keep the contribution backwards compatible.

With this contribution, if you want to have has_paper_trail and translates :versioning => true in the same model, you will have to access versions like this:

# for object's paper trail
obj.versions
# for object's translations paper trail
obj.translation.versions

Any comments will be appreciated.

I hope this helps.

Thanks!

@shioyama
Copy link
Contributor

shioyama commented Nov 7, 2013

Thanks very much for this! We'll have a look asap and get back to you.

@shioyama
Copy link
Contributor

We're currently refactoring internals (see #294) to extract the versioning functionality into a separate gem, globalize-versioning. It would be great if we could merge these efforts.

@parndt how's that going?

@parndt
Copy link
Member

parndt commented Nov 13, 2013

I haven't made any further progress unfortunately as I've been super busy with rubyconf!

@etagwerker
Copy link
Author

@parndt @shioyama I could take a stab at it.

As the versioning code only uses paper_trail, I suggest that the new gem should be called globalize-paper_trail.

What do you think?

@shioyama
Copy link
Contributor

No it should also support vestal_versions, see #269. We have debated creating two gems called globalize-paper_trail and globalize-vestal_versions.

@etagwerker
Copy link
Author

@shioyama I didn't know about vestal_versions

I added my comment to that thread.

@mastermike14
Copy link

well paper_trail is already supported by globalize so updates should be made to improve compatibility. Vestal versions is totally unrelated to this

@etagwerker
Copy link
Author

@shioyama @parndt Can we get this PR merged into 3-0-stable?

That branch is failing for this particular scenario.

I could help with the refactor of globalize-paper_trail after the merge.

Thanks!

@shioyama
Copy link
Contributor

@etagwerker Really sorry for the delay. As you can see I've started a discussion on the new globalize-versioning gem, so I'd like to move our focus there if you don't mind.

@shioyama
Copy link
Contributor

shioyama commented Jan 9, 2014

I've just released Globalize 3.0.4 with versioning extracted and earlier released 4.0.0 also with no versioning.

To use versioning, you now have to add the globalize-versioning gem. versions are no longer delegated to translation. Also options for the versioning gem can now be passed to translates.

I'm going to close this but please have a look at the new gem and report any issues to the issue tracker there. Thanks!

@shioyama shioyama closed this Jan 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants