Skip to content

Commit 5abaffc

Browse files
authored
Add a project_identifier parameter to LessonController#index (#649)
Experience CS was calling `LessonsController#index` on almost every Scratch project load, pulling the entire set of Lesson data between the two systems, and then filtering that array on the Experience CS side to find a lesson that matched a project identifier. This commit allows passing a `project_identifier` query parameter to `LessonsController#show` that will allow Experience CS to query directly for the lesson it needs. ## Status - Related to RaspberryPiFoundation/experience-cs#1002 (see [this comment](RaspberryPiFoundation/experience-cs#1002 (comment))) ## Points for consideration: - Performance: this change should allow us to make a change in Experience CS that will massively cut the amount of data being shifted between the two systems. ## What's changed? I refactored `LessonsController#index` to further narrow the scope to projects where a `project_identifier` query parameter was present in the request. I refactored it to keep Rubocop happy but, essentially, I just added the line: `scope = scope.joins(:project).where(projects: { identifier: params[:project_identifier] }) if params[:project_identifier].present?` ...to the calculation of the scope of Lessons to return. ## Steps to perform after deploying to production No additional steps required.
1 parent 439975d commit 5abaffc

2 files changed

Lines changed: 54 additions & 5 deletions

File tree

app/controllers/api/lessons_controller.rb

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,11 @@ class LessonsController < ApiController
77
load_and_authorize_resource :lesson
88

99
def index
10-
archive_scope = params[:include_archived] == 'true' ? Lesson : Lesson.unarchived
11-
scope = params[:school_class_id] ? archive_scope.where(school_class_id: params[:school_class_id]) : archive_scope
12-
ordered_scope = scope.order(created_at: :asc)
13-
14-
accessible_lessons = ordered_scope.accessible_by(current_ability).includes(project: :remixes)
10+
accessible_lessons = filtered_lessons_scope
11+
.accessible_by(current_ability)
12+
.includes(project: :remixes)
1513
@lessons_with_users = accessible_lessons.with_users
14+
1615
if current_user&.school_teacher?(school) || current_user&.school_owner?(school)
1716
render :teacher_index, formats: [:json], status: :ok
1817
else
@@ -75,6 +74,13 @@ def destroy
7574

7675
private
7776

77+
def filtered_lessons_scope
78+
archive_scope = params[:include_archived] == 'true' ? Lesson : Lesson.unarchived
79+
scope = params[:school_class_id] ? archive_scope.where(school_class_id: params[:school_class_id]) : archive_scope
80+
scope = scope.joins(:project).where(projects: { identifier: params[:project_identifier] }) if params[:project_identifier].present?
81+
scope.order(created_at: :asc)
82+
end
83+
7884
def verify_school_class_belongs_to_school
7985
return if base_params[:school_class_id].blank?
8086
return if school&.classes&.pluck(:id)&.include?(base_params[:school_class_id])

spec/features/lesson/listing_lessons_spec.rb

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,49 @@
132132
expect(data.first[:submitted_count]).to eq(1)
133133
end
134134

135+
context 'when filtering by project_identifier' do
136+
let!(:other_lesson) { create(:lesson, name: 'Another Lesson', visibility: 'public', user_id: teacher.id) }
137+
138+
it 'returns only lessons with the matching project identifier' do
139+
# Ensure other_lesson exists with a different project
140+
expect(other_lesson.project.identifier).not_to eq(lesson.project.identifier)
141+
142+
get("/api/lessons?project_identifier=#{lesson.project.identifier}", headers:)
143+
data = JSON.parse(response.body, symbolize_names: true)
144+
145+
expect(data.size).to eq(1)
146+
expect(data.first[:name]).to eq('Test Lesson')
147+
expect(data.first[:project][:identifier]).to eq(lesson.project.identifier)
148+
end
149+
150+
it 'returns empty array when no lesson matches the project identifier' do
151+
get('/api/lessons?project_identifier=non-existent-identifier', headers:)
152+
data = JSON.parse(response.body, symbolize_names: true)
153+
154+
expect(data.size).to eq(0)
155+
end
156+
157+
it 'works without authentication' do
158+
get("/api/lessons?project_identifier=#{lesson.project.identifier}")
159+
data = JSON.parse(response.body, symbolize_names: true)
160+
161+
expect(response).to have_http_status(:ok)
162+
expect(data.size).to eq(1)
163+
expect(data.first[:name]).to eq('Test Lesson')
164+
end
165+
166+
it 'can be combined with school_class_id filter' do
167+
lesson.update!(school_class_id: school_class.id)
168+
other_lesson.update!(school_class_id: school_class.id)
169+
170+
get("/api/lessons?school_class_id=#{school_class.id}&project_identifier=#{lesson.project.identifier}", headers:)
171+
data = JSON.parse(response.body, symbolize_names: true)
172+
173+
expect(data.size).to eq(1)
174+
expect(data.first[:name]).to eq('Test Lesson')
175+
end
176+
end
177+
135178
context "when the lesson's visibility is 'private'" do
136179
let!(:lesson) { create(:lesson, name: 'Test Lesson', visibility: 'private') }
137180
let(:owner) { create(:owner, school:) }

0 commit comments

Comments
 (0)