From 062d0e7ed671fc56f43123adcfa53f8fbf5db813 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 16:17:25 +0100 Subject: [PATCH 1/4] Move before below lets in updating project feature spec This is more idiomatic and thus easier to read. --- spec/features/project/updating_a_project_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/features/project/updating_a_project_spec.rb b/spec/features/project/updating_a_project_spec.rb index 871574a23..0a204b48d 100644 --- a/spec/features/project/updating_a_project_spec.rb +++ b/spec/features/project/updating_a_project_spec.rb @@ -3,12 +3,6 @@ require 'rails_helper' RSpec.describe 'Updating a project', type: :request do - before do - authenticated_in_hydra_as(owner) - - create(:component, project:, name: 'main', extension: 'py', content: 'print("hi")') - end - let(:headers) { { Authorization: UserProfileMock::TOKEN } } let(:project_type) { Project::Types::PYTHON } let(:user_id) { owner.id } @@ -27,6 +21,12 @@ } end + before do + authenticated_in_hydra_as(owner) + + create(:component, project:, name: 'main', extension: 'py', content: 'print("hi")') + end + it 'responds 200 OK' do put("/api/projects/#{project.id}", headers:, params:) expect(response).to have_http_status(:ok) From 5fda5a12da59c9436d405cef23cf17ca07b8be89 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 10 Jul 2025 13:14:20 +0100 Subject: [PATCH 2/4] Move let into private method in Updating a project spec This value doesn't change in any of the contexts, so using a `let` block is of limited value. Also it will avoid triggering a violation of `RSpec/MultipleMemoizedHelpers` in a later commit. --- spec/features/project/updating_a_project_spec.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/spec/features/project/updating_a_project_spec.rb b/spec/features/project/updating_a_project_spec.rb index 0a204b48d..2e8dca217 100644 --- a/spec/features/project/updating_a_project_spec.rb +++ b/spec/features/project/updating_a_project_spec.rb @@ -3,7 +3,6 @@ require 'rails_helper' RSpec.describe 'Updating a project', type: :request do - let(:headers) { { Authorization: UserProfileMock::TOKEN } } let(:project_type) { Project::Types::PYTHON } let(:user_id) { owner.id } let!(:project) { create(:project, name: 'Test Project', user_id:, locale: 'en', project_type:) } @@ -78,4 +77,10 @@ expect(data[:name]).to eq('Test Project') end end + + private + + def headers + { Authorization: UserProfileMock::TOKEN } + end end From 167925402f0e5902401ccbe6539ef5822833494a Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 16:54:35 +0100 Subject: [PATCH 3/4] Fix updating project feature spec Previously this spec was not representative of how the API was being used in practice and it was only really working by accident. The PUT requests in the examples were using the `Project#id` rather than the `Project#identifier` as they should have been. The spec was working even though the `Api::ProjectsController#load_project` before action was *not* finding the project, because the `load_and_authorize_resource` before action was then finding the project using `Project.find(params[:id])` and setting the `@project` instance variable used by the rest of the logic in the controller. However, clients of editor-api like editor-standalone use the project identifier as the resource ID in the URL path [1], so this spec was completely unrepresentative of how the endpoint was really being used. In this commit I've changed the examples to use the `Project#identifier` as the resource ID in the PUT requests. This means that the `Api::ProjectsController#load_project` before action now finds the project by identifier and since it sets the `@project` instance variable the `load_and_authorize_resource` before action no longer attempts to load the resource, it just does the authorize step using the already loaded project. In order to make this work reliably, I had to explicitly set the `Project#locale` to one of the fallback locales in `ProjectLoader` [2], i.e. 'en' or `nil`. Otherwise, the random locale selected in the project factory [3] meant that sometimes the `load_project` before action (which uses the `ProjectLoader` did not find the project and the `load_and_authorize_resource` before action raised an `ActiveRecord::RecordNotFound` resulting in a 404 Not Found. Now the spec is no longer making use of the randome locale from the factory, I've taken the opportunity to add some new examples demonstrating the behaviour when the project has different locales. This effectively demonstrates that `load_project` is wired up to `ProjectLoader` correctly. As an aside, I'm not convinced that having the factory select a locale at random is a good idea. I found it very confusing when it led to undeterministic specs. However, I'll leave that for another time. [1]: https://github.com/RaspberryPiFoundation/editor-standalone/blob/1d4375635cb6890794732072d608dbd4b05b3bb0/src/utils/apiCallHandler/projects.js#L16 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/b4bd337d09a88b1f41ecdc13136f7d11da0dcf89/lib/project_loader.rb#L8 [3]: https://github.com/RaspberryPiFoundation/editor-api/blob/b4bd337d09a88b1f41ecdc13136f7d11da0dcf89/spec/factories/project.rb#L9 --- .../project/updating_a_project_spec.rb | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/spec/features/project/updating_a_project_spec.rb b/spec/features/project/updating_a_project_spec.rb index 2e8dca217..32df40c20 100644 --- a/spec/features/project/updating_a_project_spec.rb +++ b/spec/features/project/updating_a_project_spec.rb @@ -5,7 +5,8 @@ RSpec.describe 'Updating a project', type: :request do let(:project_type) { Project::Types::PYTHON } let(:user_id) { owner.id } - let!(:project) { create(:project, name: 'Test Project', user_id:, locale: 'en', project_type:) } + let(:locale) { 'en' } + let!(:project) { create(:project, name: 'Test Project', user_id:, locale:, project_type:) } let(:owner) { create(:owner, school:) } let(:school) { create(:school) } @@ -27,31 +28,31 @@ end it 'responds 200 OK' do - put("/api/projects/#{project.id}", headers:, params:) + put("/api/projects/#{project.identifier}", headers:, params:) expect(response).to have_http_status(:ok) end it 'responds with the project JSON' do - put("/api/projects/#{project.id}", headers:, params:) + put("/api/projects/#{project.identifier}", headers:, params:) data = JSON.parse(response.body, symbolize_names: true) expect(data[:name]).to eq('New Name') end it 'responds with the components JSON' do - put("/api/projects/#{project.id}", headers:, params:) + put("/api/projects/#{project.identifier}", headers:, params:) data = JSON.parse(response.body, symbolize_names: true) expect(data[:components].first[:content]).to eq('print("hello")') end it 'responds 422 Unprocessable Entity when params are invalid' do - put("/api/projects/#{project.id}", headers:, params: { project: { components: [{ name: ' ' }] } }) + put("/api/projects/#{project.identifier}", headers:, params: { project: { components: [{ name: ' ' }] } }) expect(response).to have_http_status(:unprocessable_entity) end it 'responds 401 Unauthorized when no token is given' do - put("/api/projects/#{project.id}", params:) + put("/api/projects/#{project.identifier}", params:) expect(response).to have_http_status(:unauthorized) end @@ -78,6 +79,29 @@ end end + context 'when locale is nil, i.e. the other fallback locale in ProjectLoader' do + let(:locale) { nil } + + it 'responds 200 OK even though no locale is specified in query string' do + put("/api/projects/#{project.identifier}", headers:, params:) + expect(response).to have_http_status(:ok) + end + end + + context "when locale is 'fr', i.e. not a fallback locale in ProjectLoader" do + let(:locale) { 'fr' } + + it 'responds 200 OK if locale is specified in query string' do + put("/api/projects/#{project.identifier}?locale=fr", headers:, params:) + expect(response).to have_http_status(:ok) + end + + it 'responds 404 Not Found if locale is not specified in query string' do + put("/api/projects/#{project.identifier}", headers:, params:) + expect(response).to have_http_status(:not_found) + end + end + private def headers From 485e5de143cadbd2a444a0af34878cdab7c248db Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 17:22:49 +0100 Subject: [PATCH 4/4] Don't load resource via CanCanCan in Api::ProjectsController As I explained in the previous commit, the `load_project` before action is where we want the project to be loaded, i.e. via `ProjectLoader` so that it's found by a combination of `Project#identifier` and `Project#locale`. To make this clearer, I've changed the `load_and_authorize_resource` before action to `authorize_resource` [1], so the CanCanCan authorization uses the project found by the `load_project` before action. However, this meant that if the project was *not* found by the `load_project` before action an exception was raised in `Project::Update.call` resulting in a 422 Unprocessable Entity response with the following error message: Error persisting changes: undefined method `components' for nil:NilClass To fix this I'm now raising an `ActiveRecord::RecordNotFound` exception in the `load_project` before action if no project is found. This results in the expected 404 Not Found response. I think there's a strong case to be made the this exception raising behaviour should be added to `ProjectLoader#load`. However, that's a bigger change with a lot more risk, so I'm going to leave that for now. Note that I've retained the load resource functionality for the `create` action, because the `load_project` before action isn't triggered for `create` and the authorize resource functionality seems to rely on the project built by the load resource step and I want to keep changes to a minimum. [1]: https://github.com/CanCanCommunity/cancancan/blob/3.4.0/docs/controller_helpers.md#authorize_resource-load_resource-load_and_authorize_resource --- app/controllers/api/projects_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index 2ac99ca99..e3df2ed1c 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -7,7 +7,8 @@ class ProjectsController < ApiController before_action :authorize_user, only: %i[create update index destroy] before_action :load_project, only: %i[show update destroy show_context] before_action :load_projects, only: %i[index] - load_and_authorize_resource + load_resource only: :create + authorize_resource before_action :verify_lesson_belongs_to_school, only: :create after_action :pagination_link_header, only: %i[index] @@ -73,6 +74,7 @@ def load_project else project_loader.load end + raise ActiveRecord::RecordNotFound if @project.blank? end def load_projects