-
Notifications
You must be signed in to change notification settings - Fork 899
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
Add original_version
method to more easily obtain the original version.
#1204
Comments
Hi Joshua, thanks for the contribution.
Sounds like a useful method.
In PT 6, to avoid AR model namespace pollution, we moved almost everything from
The name is in keeping with other, similar methods:
Thanks, tests are required for all new features.
Did you want it merged into a different branch in addition to master? We don't usually do backports, except for important stuff like security patches. |
Thanks for the quick reply, @jaredbeck!
Makes sense, thanks. We're on the old version so once we update we'll make this change to
Naming is always tough. :) I definitely see your point about the method not returning a
Thanks! |
I strongly recommend updating one major version at a time. There have been many breaking changes over the years, all documented in the changelog, and some with runtime deprecation warnings. If you upgrade one major at a time, you can benefit from said warnings. |
Couldn't agree more. As a policy, everything we do at CNTRAL is statically versioned. So no Thanks again, will keep you posted! 👍 |
Closing in anticipation of PR. Thanks, Joshua. |
Oh noe. Sorry for the tardiness. Was just revisiting this. |
No worries at all. Take your time. I just figured the discussion was complete, so I could mark it off the list. |
+1 I think this feature would be handy. |
Did something like this feature get added to PaperTrail? If not should the issue be re-opened? |
@DanielStevenLewis Not as far as I'm aware. We're still using it in our fork but haven't found the time to submit a PR back upstream. |
@joshuapinter @jaredbeck can this be reopened? It seems super odd one can't reify the first version of an object, this should be possible |
Returning the original version is a common use-case and is not as easy as it should be, particularly because calling `reify` on the first version returns `nil`, which is the `object` *before* it was created. This method simply tries to `reify` from the *second* version, if it exists. If it doesn’t exist (i.e. there has not been any changes to the object), it falls back to `self`, which is the original and *only* version of the object. Fixes paper-trail-gem#1204.
Just following up on this as I finally got around to making the minor mods and stubbing out a PR for it. I'll wait to ensure there is still interest in adding this before adding tests. FYI, we're still using this in production and it's been working well for us. Thanks. |
of time
Is your feature suggestion related to a problem? Please describe.
Getting the original version of an item is way too difficult. It stems from the fact that calling
reify
on the firstversion
returnsnil
and not the original object. That's fine. But it means that you need to do some fancy footwork to get the original version of an object and also handle when an object doesn't have any changes (i.e. only a singlecreate
version).You can see the issue in these two spots:
https://stackoverflow.com/questions/45353464/how-to-get-original-attribute-for-a-model-with-paper-trail/56258720#56258720
Calling
reify
on the first version returnsnil
#475Describe the solution you'd like to build
I want to add an
original_version
method to thehas_paper_trail
concern that will return the original version of an object, regardless of how many changes the object has.So instead of writing something like this
versions.second&.reify || self
you can just writeoriginal_version
.Describe alternatives you've considered
versions.second&.reify || self
Commit on Our Fork.
We're using
3.0-stable
branch so I forked that and committed our desired change there. No tests but we can certainly write them if you want this merged into master.The text was updated successfully, but these errors were encountered: