"Shelters Needing Updates" view fixes #28
Conversation
|
|
||
| def outdated | ||
| @outdated, @filters = apply_params( | ||
| Shelter.where("updated_at < ?", 4.hours.ago).order('updated_at DESC'), |
There was a problem hiding this comment.
Just some food for thought (not a review). This query is a good candidate for a scope Shelter.outdated. The definition would be placed in the Shelter model after the after_commit callback:
scope :outdated, ->(timing = 4.hours.ago) { where("updated_at < ?", timing) }There was a problem hiding this comment.
Nice, hadn't used those before. I'll take a look at that.
| @@ -0,0 +1,13 @@ | |||
| json.cache! @outdated do | |||
There was a problem hiding this comment.
I don't see any configuration for a cache expiration time. Unless I have missed it (very possible) json.cache! will cache indefinitely. Adding an expires_in parameter is probably smart. Also, how do we invalidate the cache if an update is made before expiration?
There was a problem hiding this comment.
Also curious about why you didn't keep @shelters as the variable. Would have allowed you to keep the same variable for apply_params function instead of replaces the instance variables with local variables.
There was a problem hiding this comment.
Started out trying to be explicit, and things got deeper and deeper. I may switch this back and clean that up. I do like the refactoring of apply_params, so I'll likely keep that anyway.
Regarding cache, I'm just mimicking what's there already. I agree we should probably look at that, but I think that's a new issue unrelated to this, and should have it's own PR.
There was a problem hiding this comment.
Just did a little digging with fresh eyes and found that caching isn't even enabled in either development or production.
|
|
||
| private | ||
|
|
||
| def apply_params(shelters, params) |
There was a problem hiding this comment.
You don't need to pass params here, the params object is accessible anywhere in the controller.
There was a problem hiding this comment.
Sorry, all my work with functional languages coming through. Will fix.
|
OK, I think things here are ready to go. Appreciate a review, and if there's nothing else to do, an approval. Thanks! |
This does the following:
statecolumn to the Shelters Needing Updates view (and to all others) - closes Makestatefield visible in "Shelters Needing Updates" view #24/api/v1/shelters/outdated- closes Enable "Shelters Needing Updates" as a JSON endpoint #26Review appreciated. These are pretty simple fixes, so I don't expect anything to break.