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

cache_key does not consult updated_on #9033

Closed
bemurphy opened this issue Jan 22, 2013 · 7 comments
Closed

cache_key does not consult updated_on #9033

bemurphy opened this issue Jan 22, 2013 · 7 comments

Comments

@bemurphy
Copy link
Contributor

The Timestamp module accommodates for both updated_at and updated_on timestamps, while .cache_key only checks updated_at.

I'm thinking a sane strategy would be to check if either or both exist, and use the max of the result for the cache key. Thoughts?

@senny
Copy link
Member

senny commented Jan 23, 2013

sounds reasonable. If we support updated_at, we should support it throughout the system.

@rafaelfranca what do you think?

@bemurphy
Copy link
Contributor Author

I started a stab at it here: bemurphy@da73700

If we think it's a good idea, I can open a separate PR. I'm not very happy with that code though. It's a) messy and b) seems to know a lot about updated_ column names.

@senny
Copy link
Member

senny commented Jan 23, 2013

can you make use of ActiveRecord::Timestamp#timestamp_attributes_for_update ? This would allow you to be independent of the exact names but still take all available names into account (it would even be dynamic).

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/timestamp.rb#L89-L91

@bemurphy
Copy link
Contributor Author

@senny yeah, I was looking at that earlier and thinking it looked useful. I'll take a stab in the morning, thanks.

@senny
Copy link
Member

senny commented Jan 23, 2013

ping me here if you need further assistance. I think this is a good change.

@bemurphy
Copy link
Contributor Author

@senny Maybe like bemurphy@e5e0b78. Still messy; I'm feeling hesitant to pollute the methods by extracting to #max_updated_timestamp or something, though.

@rafaelfranca
Copy link
Member

Closed by #9105

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

No branches or pull requests

3 participants