From f29df41ab648cbd8862ebc3c1557fc4b8f894483 Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Fri, 25 Oct 2024 16:54:01 +0100 Subject: [PATCH 1/8] Reduce number of queries in the courses api --- app/lib/required_qualifications_summary.rb | 7 ++++++- app/models/course.rb | 2 ++ app/serializers/api/public/v1/serializable_course.rb | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/lib/required_qualifications_summary.rb b/app/lib/required_qualifications_summary.rb index ed68511a53..1031d88829 100644 --- a/app/lib/required_qualifications_summary.rb +++ b/app/lib/required_qualifications_summary.rb @@ -8,7 +8,12 @@ def initialize(course) end def extract - legacy_qualifications_attribute = course.latest_published_enrichment&.required_qualifications + # This performance improvement saves ~100 database queries in the courses api endpoint + legacy_qualifications_attribute = if course.enrichments.loaded? + course.enrichments.max_by(&:created_at)&.required_qualifications + else + course.latest_published_enrichment&.required_qualifications + end return legacy_qualifications_attribute if legacy_qualifications_attribute.present? generate_summary_text diff --git a/app/models/course.rb b/app/models/course.rb index e790b98c55..8e342c0912 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -594,6 +594,8 @@ def gcse_grade_required end def last_published_at + return enrichments.max_by(&:last_published_timestamp_utc)&.last_published_timestamp_utc if enrichments.loaded? + enrichments.maximum(:last_published_timestamp_utc) end diff --git a/app/serializers/api/public/v1/serializable_course.rb b/app/serializers/api/public/v1/serializable_course.rb index 7fa93fab98..b844ac4b56 100644 --- a/app/serializers/api/public/v1/serializable_course.rb +++ b/app/serializers/api/public/v1/serializable_course.rb @@ -138,7 +138,7 @@ def enrichment_attribute(name, enrichment_name = name) end attribute :subject_codes do - @object.subjects.pluck(:subject_code).compact + @object.subjects.map(&:subject_code).compact end attribute :required_qualifications do From a3b1200cc260573efd00ea8b366f1d767b1c0b41 Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Fri, 25 Oct 2024 17:27:59 +0100 Subject: [PATCH 2/8] Cache the course count in the api --- app/controllers/api/public/v1/courses_controller.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/public/v1/courses_controller.rb b/app/controllers/api/public/v1/courses_controller.rb index a9da837d0b..42e2cab42d 100644 --- a/app/controllers/api/public/v1/courses_controller.rb +++ b/app/controllers/api/public/v1/courses_controller.rb @@ -7,7 +7,7 @@ class CoursesController < API::Public::V1::ApplicationController def index render jsonapi: paginate(courses), include: include_param, - meta: { count: courses.count('course.id') }, + meta: { count: cached_course_count }, class: API::Public::V1::SerializerService.call rescue ActiveRecord::StatementInvalid render json: { status: 400, message: 'Invalid changed_since value, the format should be an ISO8601 UTC timestamp, for example: `2019-01-01T12:01:00Z`' }.to_json, status: :bad_request @@ -15,6 +15,16 @@ def index private + def cached_course_count + if params[:no_cache] + courses.count('course.id') + else + Rails.cache.fetch('api_course_count', expires_in: 5.minutes) do + courses.count('course.id') + end + end + end + def courses @courses ||= APICourseSearchService.call( filter: params[:filter], From 0907c46076d5200c994b24320219f9afb99af7cf Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Fri, 25 Oct 2024 18:01:39 +0100 Subject: [PATCH 3/8] Cache year Need to cache against all params --- .../api/public/v1/courses_controller.rb | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/app/controllers/api/public/v1/courses_controller.rb b/app/controllers/api/public/v1/courses_controller.rb index 42e2cab42d..024f3f99ba 100644 --- a/app/controllers/api/public/v1/courses_controller.rb +++ b/app/controllers/api/public/v1/courses_controller.rb @@ -16,10 +16,12 @@ def index private def cached_course_count - if params[:no_cache] + year = permitted_params[:recruitment_cycle_year] || RecruitmentCycle.current.year + + if permitted_params[:no_cache] courses.count('course.id') else - Rails.cache.fetch('api_course_count', expires_in: 5.minutes) do + Rails.cache.fetch("api_course_count_#{year}", expires_in: 5.minutes) do courses.count('course.id') end end @@ -27,14 +29,18 @@ def cached_course_count def courses @courses ||= APICourseSearchService.call( - filter: params[:filter], - sort: params[:sort], + filter: permitted_params[:filter], + sort: permitted_params[:sort], course_scope: recruitment_cycle.courses ) end def include_param - params.fetch(:include, '') + permitted_params.fetch(:include, '') + end + + def permitted_params + params.permit('page', 'no_cache', 'sort', 'per_page', 'courses', 'recruitment_cycle_year', 'include', 'filter' => %w[updated_since funding_type]) end end end From 02f294e7b20dd505f81a3d5025687d8bc0ff2e90 Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Wed, 30 Oct 2024 13:58:32 +0000 Subject: [PATCH 4/8] Filter out enrichments where the last_published_timestamp_utc is nil --- app/models/course.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/course.rb b/app/models/course.rb index 8e342c0912..f32ba6f68f 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -594,7 +594,7 @@ def gcse_grade_required end def last_published_at - return enrichments.max_by(&:last_published_timestamp_utc)&.last_published_timestamp_utc if enrichments.loaded? + return enrichments.select(&:last_published_timestamp_utc).max_by(&:last_published_timestamp_utc)&.last_published_timestamp_utc if enrichments.loaded? enrichments.maximum(:last_published_timestamp_utc) end From 673655c319f016c353fa1b11c70ce9ecd4094931 Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Wed, 30 Oct 2024 16:13:52 +0000 Subject: [PATCH 5/8] Add test for when course.enrichments are not loaded --- app/lib/required_qualifications_summary.rb | 1 - spec/lib/required_qualifications_summary_spec.rb | 12 ++++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/lib/required_qualifications_summary.rb b/app/lib/required_qualifications_summary.rb index 1031d88829..af00f0d612 100644 --- a/app/lib/required_qualifications_summary.rb +++ b/app/lib/required_qualifications_summary.rb @@ -8,7 +8,6 @@ def initialize(course) end def extract - # This performance improvement saves ~100 database queries in the courses api endpoint legacy_qualifications_attribute = if course.enrichments.loaded? course.enrichments.max_by(&:created_at)&.required_qualifications else diff --git a/spec/lib/required_qualifications_summary_spec.rb b/spec/lib/required_qualifications_summary_spec.rb index 4f28832933..6f3cc88e33 100644 --- a/spec/lib/required_qualifications_summary_spec.rb +++ b/spec/lib/required_qualifications_summary_spec.rb @@ -60,6 +60,18 @@ end end + context 'when the course enrichments are not loaded' do + let(:enrichment) { build(:course_enrichment, :published, required_qualifications: 'GCSE Computer Science') } + let(:course) { create(:course, enrichments: [enrichment]) } + + it 'returns the expected value' do + course.reload + expect(course.enrichments.loaded?).to be_falsey + summary = described_class.new(course).extract + expect(summary).to eq 'GCSE Computer Science' + end + end + context 'when required_qualifications enrichment attribute is blank' do it 'assembles a whole summary based on course attributes' do course = create(:course, :primary) From fe478be4640d60835647e0a97617d06bc9c208cd Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Wed, 30 Oct 2024 16:40:51 +0000 Subject: [PATCH 6/8] Add spec for Course#last_published_at --- spec/models/course_spec.rb | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index 0a4c9c4654..aab741f9ba 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -3157,6 +3157,38 @@ end end + describe '#last_published_at' do + context 'with an unpublished course' do + let(:course) { create(:course, study_mode: :part_time, enrichments: [create(:course_enrichment)]) } + + it 'returns nil' do + expect(course.last_published_at).to be_nil + end + end + + context 'with a twice published course' do + let(:course) do + create(:course, study_mode: :part_time, enrichments: + [ + create(:course_enrichment, :published, last_published_timestamp_utc: '1/1/2024'), + create(:course_enrichment, :published, last_published_timestamp_utc: '1/1/2023') + ]) + end + + it 'selects the enrichment with the latest date' do + expect(course.last_published_at).to eq('1/1/2024') + end + + context 'when the enrichments are not loaded' do + it 'selects the enrichment with the latest date' do + course.reload + expect(course.enrichments.loaded?).to be_falsey + expect(course.last_published_at).to eq('1/1/2024') + end + end + end + end + describe 'funding_type and program_type' do context 'setting the funding to apprenticeship' do it 'sets the funding to apprenticeship and program_type to pg_teaching_apprenticeship' do From 56988d8c9a8ac4da2bcbe42ca0259930bc58f11c Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Thu, 31 Oct 2024 08:56:05 +0000 Subject: [PATCH 7/8] Specify that api/courses params are permitted now --- spec/controllers/api/public/v1/courses_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/api/public/v1/courses_controller_spec.rb b/spec/controllers/api/public/v1/courses_controller_spec.rb index 720522c180..29cd5e6dc9 100644 --- a/spec/controllers/api/public/v1/courses_controller_spec.rb +++ b/spec/controllers/api/public/v1/courses_controller_spec.rb @@ -306,7 +306,7 @@ it 'delegates to the CourseSearchService' do expect(CourseSearchService).to have_received(:call).with( - hash_including(filter: ActionController::Parameters.new(funding_type: 'salary')) + hash_including(filter: ActionController::Parameters.new(funding_type: 'salary').permit!) ) end end From 6a711b806aa599e18385cb58bbb911580fb09256 Mon Sep 17 00:00:00 2001 From: Iain McNulty Date: Fri, 1 Nov 2024 09:32:08 +0000 Subject: [PATCH 8/8] Remove no_cache api param --- app/controllers/api/public/v1/courses_controller.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/public/v1/courses_controller.rb b/app/controllers/api/public/v1/courses_controller.rb index 024f3f99ba..6bbf6040c7 100644 --- a/app/controllers/api/public/v1/courses_controller.rb +++ b/app/controllers/api/public/v1/courses_controller.rb @@ -18,12 +18,8 @@ def index def cached_course_count year = permitted_params[:recruitment_cycle_year] || RecruitmentCycle.current.year - if permitted_params[:no_cache] + Rails.cache.fetch("api_course_count_#{year}", expires_in: 5.minutes) do courses.count('course.id') - else - Rails.cache.fetch("api_course_count_#{year}", expires_in: 5.minutes) do - courses.count('course.id') - end end end @@ -40,7 +36,7 @@ def include_param end def permitted_params - params.permit('page', 'no_cache', 'sort', 'per_page', 'courses', 'recruitment_cycle_year', 'include', 'filter' => %w[updated_since funding_type]) + params.permit('page', 'sort', 'per_page', 'courses', 'recruitment_cycle_year', 'include', 'filter' => %w[updated_since funding_type]) end end end