Course cache

[Bjvanminnen]

This PR adds a course cache, styled after the cache currently used by scripts. It also update a script method (course_link) to actually take advantage of the cache instead of hitting the db.

My biggest concern with this PR is that it ends up duplicating a lot of code. I'm not sure if it makes sense to spend a lot of time cleaning that up or not.

I think the right way to do this (and I'm open to feedback) would be to have a single file that handles our model caches. This would extract various cacheing that exists in the Script model and the Course model, and instead have most of that logic live in our model_cache.rb. This would likely have a single should_cache? method used, and there might be other opportunities to share helpers. My main concern is that I'm still only semi-competent when it comes to Ruby/Rails, and I'm not sure I'm the best person to try to build this. Would be interested in feedback from others as to how much they dislike the current repetitiveness.

[ashercodeorg]

This seems like a reasonable approach to establish the cache and associated tests, with a follow-up PR to remove the code duplication. Happy to do that PR if you want, though I don't think I'm any more "qualified" than you are.

[ashercodeorg]

nit: Though this is simply duplication of the existing paradigm, the method API and the method implementation would be better documented if id were replaced with id_or_name or course_id_or_course_name. Also below.

[ashercodeorg]

nit: The last two lines could just be ENV['UNIT_TEST'] || ENV['CI'].

[joshlory]

I think the right way to do this (and I'm open to feedback) would be to have a single file that handles our model caches. This would extract various cacheing that exists in the Script model and the Course model, and instead have most of that logic live in our model_cache.rb. This would likely have a single should_cache? method used, and there might be other opportunities to share helpers.

+💯 for this option!

I'm not sure I'm the best person to try to build this.

I'm not sure anyone on the team has full knowledge of how we cache models (maybe @wjordan?). Cleaning up the various disperse caching methods into one understandable system would help!

[Bjvanminnen]

This is now ready for review, having got tests passing.

One other significant change I made was to make it so that we always add scripts/courses to the cache the same way. Previously, if we added them when we first got data from the db, we did it using Model.includes so that we ended up with all the info desired. However, if we then later had a cache miss, we would add content to the cache without the includes. In practice, I suspect we don't really have cache misses, but this made reasoning about some of this in tests a lot easier (and seems like general goodness).

My plan for the time being is to punt on trying to move this all into a single file. I think this current step puts us in a better place (we get caching for courses) and though there's some duplication of pretty similar code, I don't think it really makes things any harder to understand. Feel free to push back.

[aoby]

What does this do? The returned connection isn't used.

[Bjvanminnen]

I do not know. I copied it because it was there in the script version. My assumption was that it perhaps somehow locked the db connection for this course of this method?

[aoby]

Looks like it maybe ensures that a connection is established for this thread, but that should happen anyway in the below query. It doesn't appear to do anything special for the method. See http://api.rubyonrails.org/classes/ActiveRecord/Base.html

[aoby]

Why retrieve all the ids, then loop through and retrieve each Course via get_without_cache, instead of one query?

If this is for the included associations, how about defining them in a scope and using that in both locations?

[aoby]

Something like:

scope :with_associated_models, -> {includes([:plc_course, :course_scripts])}

def self.course_cache_from_db
  {}.tap do |cache|
    Course.with_associated_models.find_each do |course|
      cache[course.name] = course
      cache[course.id.to_s] = course
    end
  end
end

def self.get_without_cache(id_or_name)
  # ...
  find_by = (id_or_name.to_i.to_s == id_or_name.to_s) ? :id : :name
  Course.with_associated_models.find_by(find_by => id_or_name)
end

[aoby]

might be cleaner to use regex instead? id_or_name =~ /^\d+$/ ? :id : :name

[Bjvanminnen]

This was copied from what's used in Script, which seems to have been working for us. I'd be semi-worried about introducing a regression trying to derive an equivalent regex.

[aoby]

I'm ok leaving this alone now, and revisiting when (if) we clean this up and consolidate the caching logic. I can help with that effort.

FWIW, we use this same regex for a similar purpose in other places.

[Bjvanminnen]

The test that keeps failing for me (level_group.feature) passes pretty consistently on my local host. In addition, it's one that used to be marked @no_circle due to flakiness on circle. Josh just remarked it and a couple others as no_circle. I'm going to give this circle another attempt, but unless something new pops up would like to merge. Anyone willing to give me an approve? :)

[ashercodeorg]

SGTM.

[Bjvanminnen]

Only failure was in weblab_submittable. This was disabled until recently on circle because of known flakiness, and passes for me locally, so I'm going to merge this PR.