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

Remove hardcoded references to id and created_at fields #129

Merged
merged 3 commits into from
Mar 12, 2012

Conversation

jrmehle
Copy link
Contributor

@jrmehle jrmehle commented Feb 1, 2012

The gem assumes the generated migration is run without modification and refers to id and created_at fields throughout.

The first two commits change the version model to use the class' primary key instead of referencing id directly.

The last commit adds a configuration option for "timestamp_field" which allows the user to specify the name of the field which is used to track when the version was created.

@airblade
Copy link
Collaborator

airblade commented Feb 7, 2012

Thanks very much for this, much appreciated.

Please could you add a test for the custom timestamp field?

@jrmehle
Copy link
Contributor Author

jrmehle commented Mar 1, 2012

I'm a bit lost as to how to go about testing this feature. I've tried setting the value in the test app and then checking for it, but that breaks other tests.

@airblade airblade merged commit f8becc6 into paper-trail-gem:master Mar 12, 2012
@airblade
Copy link
Collaborator

Sorry about the delay getting back to you. I hope it hasn't inconvenienced you.

I added a test for the timestamp field here.

Your changes are in v2.6.1. Thanks again!

@jaredbeck
Copy link
Member

The last commit adds a configuration option for "timestamp_field" which allows the user to specify the name of the field which is used to track when the version was created.

@jrmehle Why was this necessary? Was there some reason your app had to change the name of the column (versions.created_at)? The standard column name in rails is created_at. Furthermore, the versions table is an implementation detail of PT. Yes, parts like object_changes are configurable, and we want people to be able to add metadata columns, but I see no reason to make the name of the created_at column configurable. It adds unnecessary complexity to PT. Unless there is a good reason why it has to be configurable, I would like to simplify PT by deprecating this configuration option. Please advise.

jaredbeck added a commit that referenced this pull request Sep 4, 2016
A little over four years ago, a feature was added to allow users
to change the name of the `versions.created_at` column. No reason
was given at the time.

#129

There are three reasons why this should not be configurable:

1. The standard column name in rails is `created_at`.
2. The majority of the `versions` table is, and should be, an
   implementation detail of PT.
3. This configuration option added moderate complexity to the
   library, and severe complexity to the test suite.
@jrmehle
Copy link
Contributor Author

jrmehle commented Sep 4, 2016

Yeah, it was necessary. The reason it was added was because at the time I was working for a company with a legacy, non-standard Rails database that did have timestamp columns, but happened to have named them differently than Rails would expect. Anyone working in this sort of environment is going to benefit from this change. While I do not work for this company any longer, I can't give my blessing to this change.

@jaredbeck
Copy link
Member

Thanks for the quick response, Jared.

.. a legacy, non-standard Rails database that did have timestamp columns, but happened to have named them differently than Rails would expect.

Why were they named differently? If they were forced to use the standard name in the versions table only would they be able to?

@jrmehle
Copy link
Contributor Author

jrmehle commented Sep 4, 2016

Consistency. Every other table in the database conformed to the same naming scheme for timestamp fields. Sorry, I can't really answer what the company I left almost 2 years ago would do. At the time, the deal was, we conformed to existing company database standards. I can imagine that other such scenarios exist within organizations. Again, I therefore cannot recommend you go forward with your change.

@jaredbeck
Copy link
Member

Thanks Jared, that answers my question. If I understand correctly, there was no technical reason why that company couldn't use the standard rails timestamp name. I'm going to recommend that we simplify PT by requiring people to follow the rails convention. I'll discuss it with Andy and Ben in #861 and I'll make sure to mention your recommendation there as well. Thanks again for your quick response.

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.

3 participants