Replace Equipment Item Count Queries with Counter Cache

[esoterik]

This is my bad; I forgot that there was a counter cache for equipment items (even though I originally added it) when I was doing #1501!

Should be super simple and eliminate an extra DB call on all availability methods.

[orenyk]

Wait, what? How did we both miss this?

EDIT Ohhh, I see, it was while ago. Good catch.

[esoterik]

okay, it turns out that we use this counter cache all of nowhere. Currently it just counts the number of items on the model, regardless of whether or not they are active. I'm going to re-purpose it to count only active equipment items because it will be much more useful that way

edit
misunderstood how items are deactivated; the counter cache is fine as is

[esoterik]

okay, this is actually creating tons of issues in the test suite because the equipment model doesn't get reloaded before the availability calculations so most of our validations break. It can be fixed by forcing the model to reload in the actual model definition, but I don't think that's a good idea. I'm trying to figure out another solution but it isn't easy

[orenyk]

What were we originally using the counter cache for and when did we stop using it? If we're no longer using it and it creates a ton of testing issues, maybe counter caches simply aren't a great pattern to use in general (the object reloading requirement during testing is particularly suspect to me). Do you want to see if it's something that's recommended against in which case we can just remove the unused cache and go from there? I think for the overdue count it made way more sense, and it's currently working (although future availability modifications / testing may run into trouble), but if adding another counter cache is only going to complicate things I think we can afford to take the extra query.

One alternative would be to define a (private?) instance method for at least avoiding multiple queries during a given request, e.g. something like

def equipment_items_count
  @equipment_items_count ||= equipment_items.count
end

This allows us to potentially save a few DB hits without messing up our tests too much. What do you think?

[esoterik]

I think that the original counter cache was for the equipment models table view (or at least that's when it was added), and I don't think it was ever really used. I can't find much commentary on whether or not counter caches should be used, but I don't think that it will be easy to get them to work properly in rspec for our case.

The instance method seems like a better solution to me.