Remove unused version_year from script cache key

[wjordan]

Fixes a bug introduced in #22887 where scripts fetched for a non-nil version_year (which included all fetches from ScriptLevelsController#show) were not pre-loaded during application initialization. This caused the first request (within each process) to ScriptLevelsController#show for each script to be slower than expected on startup.

Since #28683, version_year is no longer used by the script-cache fetch anyway, so all of the logic extending the cache key by this variable can be removed for simplicity.

[davidsbailey]

Thank you, @wjordan ! Looks great.

While you are in here, you could also delete the unused get_script_family_redirect if you want -- nothing references it, so it should be pretty low risk.

The only other thing I can think of would be to add an integration test which counts the number of database requests. However, it looks like we already have some which do that, so I'm a bit puzzled as to why those haven't been failing:

code-dot-org/dashboard/test/integration/caching_test.rb

Lines 22 to 27 in 06d2450

	test "should get show of frozen level 1" do
	assert_cached_queries(0) do
	get '/s/frozen/stage/1/puzzle/1'
	end
	assert_response :success
	end

[davidsbailey]

looks like cache_key needs to be defined and must be a string

[davidsbailey]

this should just be script id or script name.

[davidsbailey]

The only other thing I can think of would be to add an integration test which counts the number of database requests. However, it looks like we already have some which do that, so I'm a bit puzzled as to why those haven't been failing

Oops, bad example, because frozen is a HOC script. but course1 is not, so I would have expected this test to have been failing:

code-dot-org/dashboard/test/integration/caching_test.rb

Lines 80 to 85 in 06d2450

	test "should get show of course1 level 1 twice" do
	assert_cached_queries(0) do
	get '/s/course1/stage/3/puzzle/1'
	end
	assert_response :success
	end

[davidsbailey]

ok... looks like those tests take a cache miss the first time, and are only measuring the db query count on the second try. so a different kind of test would be needed in order to test that this PR eliminates the slow startup problem that we're trying to address.

[maddiedierker]

🥇 thank you for this, will!

[wjordan]

so a different kind of test would be needed in order to test that this PR eliminates the slow startup problem that we're trying to address.

Yeah, for covering this particular issue we need to test that the caching-initialization behavior has already cached the script on the very first request, not the second. I've added a test to cover this in 9a0326f (and confirmed the test failed before this PR and passes with the fix).