From 99e04c58e1c1639db71bdab7b5ab49e2b9261e1e Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 2 Jul 2024 16:14:43 +0100 Subject: [PATCH 01/11] DRY up `ProfileApiClient.create_school` examples --- spec/lib/profile_api_client_spec.rb | 51 ++++++++++++++++------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb index 3926de7c6..ae2a45f62 100644 --- a/spec/lib/profile_api_client_spec.rb +++ b/spec/lib/profile_api_client_spec.rb @@ -8,63 +8,64 @@ let(:api_key) { 'api-key' } let(:token) { SecureRandom.uuid } let(:school) { build(:school, id: SecureRandom.uuid, code: SecureRandom.uuid) } + let(:create_school_url) { "#{api_url}/api/v1/schools" } before do - stub_request(:post, "#{api_url}/api/v1/schools").to_return(status: 201, body: '{}') + stub_request(:post, create_school_url).to_return(status: 201, body: '{}') allow(ENV).to receive(:fetch).with('IDENTITY_URL').and_return(api_url) allow(ENV).to receive(:fetch).with('PROFILE_API_KEY').and_return(api_key) end it 'makes a request to the profile api host' do - described_class.create_school(token:, school:) - expect(WebMock).to have_requested(:post, "#{api_url}/api/v1/schools") + create_school + expect(WebMock).to have_requested(:post, create_school_url) end it 'includes token in the authorization request header' do - described_class.create_school(token:, school:) - expect(WebMock).to have_requested(:post, "#{api_url}/api/v1/schools").with(headers: { authorization: "Bearer #{token}" }) + create_school + expect(WebMock).to have_requested(:post, create_school_url).with(headers: { authorization: "Bearer #{token}" }) end it 'includes the profile api key in the x-api-key request header' do - described_class.create_school(token:, school:) - expect(WebMock).to have_requested(:post, "#{api_url}/api/v1/schools").with(headers: { 'x-api-key' => api_key }) + create_school + expect(WebMock).to have_requested(:post, create_school_url).with(headers: { 'x-api-key' => api_key }) end it 'sets content-type of request to json' do - described_class.create_school(token:, school:) - expect(WebMock).to have_requested(:post, "#{api_url}/api/v1/schools").with(headers: { 'content-type' => 'application/json' }) + create_school + expect(WebMock).to have_requested(:post, create_school_url).with(headers: { 'content-type' => 'application/json' }) end it 'sets accept header to json' do - described_class.create_school(token:, school:) - expect(WebMock).to have_requested(:post, "#{api_url}/api/v1/schools").with(headers: { 'accept' => 'application/json' }) + create_school + expect(WebMock).to have_requested(:post, create_school_url).with(headers: { 'accept' => 'application/json' }) end it 'sends the school id and code in the request body as json' do - described_class.create_school(token:, school:) + create_school expected_body = { id: school.id, schoolCode: school.code }.to_json - expect(WebMock).to have_requested(:post, "#{api_url}/api/v1/schools").with(body: expected_body) + expect(WebMock).to have_requested(:post, create_school_url).with(body: expected_body) end it 'returns the created school if successful' do data = { 'id' => 'school-id', 'schoolCode' => 'school-code' } - stub_request(:post, "#{api_url}/api/v1/schools") + stub_request(:post, create_school_url) .to_return(status: 201, body: data.to_json) - expect(described_class.create_school(token:, school:)).to eq(data) + expect(create_school).to eq(data) end it 'raises exception if anything other than a 201 status code is returned' do - stub_request(:post, "#{api_url}/api/v1/schools") + stub_request(:post, create_school_url) .to_return(status: 200) - expect { described_class.create_school(token:, school:) }.to raise_error(RuntimeError) + expect { create_school }.to raise_error(RuntimeError) end it 'includes details of underlying response when exception is raised' do - stub_request(:post, "#{api_url}/api/v1/schools") + stub_request(:post, create_school_url) .to_return(status: 401) - expect { described_class.create_school(token:, school:) }.to raise_error('School not created in Profile API. HTTP response code: 401') + expect { create_school }.to raise_error('School not created in Profile API. HTTP response code: 401') end describe 'when BYPASS_OAUTH is true' do @@ -73,14 +74,20 @@ end it 'does not make a request to Profile API' do - described_class.create_school(token:, school:) - expect(WebMock).not_to have_requested(:post, "#{api_url}/api/v1/schools") + create_school + expect(WebMock).not_to have_requested(:post, create_school_url) end it 'returns the id and code of the school supplied' do expected = { 'id' => school.id, 'schoolCode' => school.code } - expect(described_class.create_school(token:, school:)).to eq(expected) + expect(create_school).to eq(expected) end end + + private + + def create_school + described_class.create_school(token:, school:) + end end end From c1adae37d66aa37f6ced4e2aeebe5ed5992ab3c9 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 4 Jul 2024 11:11:06 +0100 Subject: [PATCH 02/11] Avoid passing domain object to `ProfileApiClient` I think it probably makes sense to think of ProfileApiClient as an external library that wraps Profile API but doesn't know anything about our domain objects. --- app/services/school_verification_service.rb | 2 +- lib/profile_api_client.rb | 8 ++++---- spec/lib/profile_api_client_spec.rb | 2 +- spec/services/school_verification_service_spec.rb | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/services/school_verification_service.rb b/app/services/school_verification_service.rb index 711dde4ab..38b6f4ff1 100644 --- a/app/services/school_verification_service.rb +++ b/app/services/school_verification_service.rb @@ -13,7 +13,7 @@ def verify(token:) school.verify! Role.owner.create!(user_id: school.creator_id, school:) Role.teacher.create!(user_id: school.creator_id, school:) - ProfileApiClient.create_school(token:, school:) + ProfileApiClient.create_school(token:, id: school.id, code: school.code) end rescue StandardError => e Sentry.capture_exception(e) diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index c898b0a4c..c8f221328 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -4,14 +4,14 @@ class ProfileApiClient class << self # TODO: Replace with HTTP requests once the profile API has been built. - def create_school(token:, school:) - return { 'id' => school.id, 'schoolCode' => school.code } if ENV['BYPASS_OAUTH'].present? + def create_school(token:, id:, code:) + return { 'id' => id, 'schoolCode' => code } if ENV['BYPASS_OAUTH'].present? response = connection.post('/api/v1/schools') do |request| apply_default_headers(request, token) request.body = { - id: school.id, - schoolCode: school.code + id:, + schoolCode: code }.to_json end diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb index ae2a45f62..8bdccde27 100644 --- a/spec/lib/profile_api_client_spec.rb +++ b/spec/lib/profile_api_client_spec.rb @@ -87,7 +87,7 @@ private def create_school - described_class.create_school(token:, school:) + described_class.create_school(token:, id: school.id, code: school.code) end end end diff --git a/spec/services/school_verification_service_spec.rb b/spec/services/school_verification_service_spec.rb index 628b3ccc2..28c4dd3ae 100644 --- a/spec/services/school_verification_service_spec.rb +++ b/spec/services/school_verification_service_spec.rb @@ -44,7 +44,7 @@ it 'creates the school in Profile API' do service.verify(token:) - expect(ProfileApiClient).to have_received(:create_school).with(token:, school:) + expect(ProfileApiClient).to have_received(:create_school).with(token:, id: school.id, code: school.code) end it 'returns true' do From c922b7ffb90960b16d82f24fa77fb647b47c29dc Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 4 Jul 2024 12:37:07 +0100 Subject: [PATCH 03/11] Condense error message in `ProfileApiClient.create_school` --- lib/profile_api_client.rb | 2 +- spec/lib/profile_api_client_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index c8f221328..b86034c87 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -15,7 +15,7 @@ def create_school(token:, id:, code:) }.to_json end - raise "School not created in Profile API. HTTP response code: #{response.status}" unless response.status == 201 + raise "School not created in Profile API (status code #{response.status})" unless response.status == 201 JSON.parse(response.body) end diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb index 8bdccde27..a6e6d2aaa 100644 --- a/spec/lib/profile_api_client_spec.rb +++ b/spec/lib/profile_api_client_spec.rb @@ -65,7 +65,7 @@ stub_request(:post, create_school_url) .to_return(status: 401) - expect { create_school }.to raise_error('School not created in Profile API. HTTP response code: 401') + expect { create_school }.to raise_error('School not created in Profile API (status code 401)') end describe 'when BYPASS_OAUTH is true' do From 084dcc0bb893397047b656453439632f167399b1 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 4 Jul 2024 14:18:23 +0100 Subject: [PATCH 04/11] Use Faraday JSON request middleware To automatically serialise the request body as JSON and set the `Content-Type` header[1]. [1]: https://lostisland.github.io/faraday/#/middleware/included/json?id=json-requests --- lib/profile_api_client.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index b86034c87..19381318c 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -12,7 +12,7 @@ def create_school(token:, id:, code:) request.body = { id:, schoolCode: code - }.to_json + } end raise "School not created in Profile API (status code #{response.status})" unless response.status == 201 @@ -201,13 +201,14 @@ def delete_school_student(token:, student_id:, organisation_id:) private def connection - Faraday.new(ENV.fetch('IDENTITY_URL')) + Faraday.new(ENV.fetch('IDENTITY_URL')) do |faraday| + faraday.request :json + end end def apply_default_headers(request, token) request.headers['Accept'] = 'application/json' request.headers['Authorization'] = "Bearer #{token}" - request.headers['Content-Type'] = 'application/json' request.headers['X-API-KEY'] = ENV.fetch('PROFILE_API_KEY') request end From 942e549680838489fe8f9aef98976fa0fe04eae6 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 4 Jul 2024 14:26:35 +0100 Subject: [PATCH 05/11] Use Faraday JSON response middleware To automatically deserialize the response body when the `Content-Type` indicates that it contains JSON[1] [1]: https://lostisland.github.io/faraday/#/middleware/included/json?id=json-responses --- lib/profile_api_client.rb | 3 ++- spec/lib/profile_api_client_spec.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index 19381318c..c6c73164d 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -17,7 +17,7 @@ def create_school(token:, id:, code:) raise "School not created in Profile API (status code #{response.status})" unless response.status == 201 - JSON.parse(response.body) + response.body end # The API should enforce these constraints: @@ -203,6 +203,7 @@ def delete_school_student(token:, student_id:, organisation_id:) def connection Faraday.new(ENV.fetch('IDENTITY_URL')) do |faraday| faraday.request :json + faraday.response :json end end diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb index a6e6d2aaa..30bc6aa37 100644 --- a/spec/lib/profile_api_client_spec.rb +++ b/spec/lib/profile_api_client_spec.rb @@ -50,7 +50,7 @@ it 'returns the created school if successful' do data = { 'id' => 'school-id', 'schoolCode' => 'school-code' } stub_request(:post, create_school_url) - .to_return(status: 201, body: data.to_json) + .to_return(status: 201, body: data.to_json, headers: { 'Content-Type' => 'application/json' }) expect(create_school).to eq(data) end From efae2c73feabd5de8d4b5709d7d9d9753b1bee49 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 4 Jul 2024 14:30:19 +0100 Subject: [PATCH 06/11] Set headers on Faraday connection object This is probably more idiomatic than setting them in the `apply_default_headers` method. --- lib/profile_api_client.rb | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index c6c73164d..b2d273112 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -7,8 +7,7 @@ class << self def create_school(token:, id:, code:) return { 'id' => id, 'schoolCode' => code } if ENV['BYPASS_OAUTH'].present? - response = connection.post('/api/v1/schools') do |request| - apply_default_headers(request, token) + response = connection(token).post('/api/v1/schools') do |request| request.body = { id:, schoolCode: code @@ -200,18 +199,16 @@ def delete_school_student(token:, student_id:, organisation_id:) private - def connection + def connection(token) Faraday.new(ENV.fetch('IDENTITY_URL')) do |faraday| faraday.request :json faraday.response :json + faraday.headers = { + 'Accept' => 'application/json', + 'Authorization' => "Bearer #{token}", + 'X-API-KEY' => ENV.fetch('PROFILE_API_KEY') + } end end - - def apply_default_headers(request, token) - request.headers['Accept'] = 'application/json' - request.headers['Authorization'] = "Bearer #{token}" - request.headers['X-API-KEY'] = ENV.fetch('PROFILE_API_KEY') - request - end end end From 69246b0d20f6b3144b775d3b03b0ef8ce510ec29 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 2 Jul 2024 16:09:30 +0100 Subject: [PATCH 07/11] Add `ProfileApiClient.safeguarding_flags` To list the Safeguarding flags for the user associated with the access token. Note that I'm intentionally not doing anything different if `BYPASS_OAUTH` is set. Given how tightly coupled Editor API and Profile API are I'm less convinced it makes sense to attempt to run one without the other in development. API docs: https://my.raspberrypi.org/api/v1/documentation/#/Safeguarding%20Flags/get_safeguarding_flags --- lib/profile_api_client.rb | 10 ++++ spec/lib/profile_api_client_spec.rb | 80 +++++++++++++++++++++++++++-- 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index b2d273112..c3581e38e 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -197,6 +197,16 @@ def delete_school_student(token:, student_id:, organisation_id:) response.deep_symbolize_keys end + def safeguarding_flags(token:) + response = connection(token).get('/api/v1/safeguarding-flags') + + unless response.status == 200 + raise "Safeguarding flags cannot be retrieved from Profile API (status code #{response.status})" + end + + response.body.map(&:deep_symbolize_keys) + end + private def connection(token) diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb index 30bc6aa37..a730e0237 100644 --- a/spec/lib/profile_api_client_spec.rb +++ b/spec/lib/profile_api_client_spec.rb @@ -3,17 +3,21 @@ require 'rails_helper' RSpec.describe ProfileApiClient do + let(:api_url) { 'http://example.com' } + let(:api_key) { 'api-key' } + let(:token) { SecureRandom.uuid } + + before do + allow(ENV).to receive(:fetch).with('IDENTITY_URL').and_return(api_url) + allow(ENV).to receive(:fetch).with('PROFILE_API_KEY').and_return(api_key) + end + describe '.create_school' do - let(:api_url) { 'http://example.com' } - let(:api_key) { 'api-key' } - let(:token) { SecureRandom.uuid } let(:school) { build(:school, id: SecureRandom.uuid, code: SecureRandom.uuid) } let(:create_school_url) { "#{api_url}/api/v1/schools" } before do stub_request(:post, create_school_url).to_return(status: 201, body: '{}') - allow(ENV).to receive(:fetch).with('IDENTITY_URL').and_return(api_url) - allow(ENV).to receive(:fetch).with('PROFILE_API_KEY').and_return(api_key) end it 'makes a request to the profile api host' do @@ -90,4 +94,70 @@ def create_school described_class.create_school(token:, id: school.id, code: school.code) end end + + describe '.safeguarding_flags' do + let(:list_safeguarding_flags_url) { "#{api_url}/api/v1/safeguarding-flags" } + + before do + stub_request(:get, list_safeguarding_flags_url).to_return(status: 200, body: '[]', headers: { 'content-type' => 'application/json' }) + end + + it 'makes a request to the profile api host' do + list_safeguarding_flags + expect(WebMock).to have_requested(:get, list_safeguarding_flags_url) + end + + it 'includes token in the authorization request header' do + list_safeguarding_flags + expect(WebMock).to have_requested(:get, list_safeguarding_flags_url).with(headers: { authorization: "Bearer #{token}" }) + end + + it 'includes the profile api key in the x-api-key request header' do + list_safeguarding_flags + expect(WebMock).to have_requested(:get, list_safeguarding_flags_url).with(headers: { 'x-api-key' => api_key }) + end + + it 'sets accept header to json' do + list_safeguarding_flags + expect(WebMock).to have_requested(:get, list_safeguarding_flags_url).with(headers: { 'accept' => 'application/json' }) + end + + # rubocop:disable RSpec/ExampleLength + it 'returns list of safeguarding flags if successful' do + flags = [ + { + id: '7ac79585-e187-4d2f-bf0c-a1cbe72ecc9a', + userId: '583ba872-b16e-46e1-9f7d-df89d267550d', + flag: 'school:owner', + isActive: 'true', + createdAt: '2024-07-01T12:49:18.926Z', + updatedAt: '2024-07-01T12:49:18.926Z' + } + ] + stub_request(:get, list_safeguarding_flags_url) + .to_return(status: 200, body: flags.to_json, headers: { 'content-type' => 'application/json' }) + expect(list_safeguarding_flags).to eq(flags) + end + # rubocop:enable RSpec/ExampleLength + + it 'raises exception if anything other than a 200 status code is returned' do + stub_request(:get, list_safeguarding_flags_url) + .to_return(status: 201) + + expect { list_safeguarding_flags }.to raise_error(RuntimeError) + end + + it 'includes details of underlying response when exception is raised' do + stub_request(:get, list_safeguarding_flags_url) + .to_return(status: 401) + + expect { list_safeguarding_flags }.to raise_error('Safeguarding flags cannot be retrieved from Profile API (status code 401)') + end + + private + + def list_safeguarding_flags + described_class.safeguarding_flags(token:) + end + end end From 1379992a10114f597497fc646a87288634ced954 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 2 Jul 2024 16:19:29 +0100 Subject: [PATCH 08/11] Add `ProfileApiClient.create_safeguarding_flag` In preparation for creating the relevant teacher or owner safeguarding flags for the authenticated user so that they have permission to CRUD students in Profile API. Note that Profile API returns 201 if the flag was created and 303 if it already exists. Note that I'm intentionally not doing anything different if `BYPASS_OAUTH` is set. Given how tightly coupled Editor API and Profile API are I'm less convinced it makes sense to attempt to run one without the other in development. API docs: https://my.raspberrypi.org/api/v1/documentation/#/Safeguarding%20Flags/post_safeguarding_flags --- lib/profile_api_client.rb | 15 +++++++ spec/lib/profile_api_client_spec.rb | 69 +++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index c3581e38e..65bbcd448 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -1,6 +1,11 @@ # frozen_string_literal: true class ProfileApiClient + SAFEGUARDING_FLAGS = { + teacher: 'school:teacher', + owner: 'school:owner' + }.freeze + class << self # TODO: Replace with HTTP requests once the profile API has been built. @@ -207,6 +212,16 @@ def safeguarding_flags(token:) response.body.map(&:deep_symbolize_keys) end + def create_safeguarding_flag(token:, flag:) + response = connection(token).post('/api/v1/safeguarding-flags') do |request| + request.body = { flag: } + end + + return if response.status == 201 || response.status == 303 + + raise "Safeguarding flag not created in Profile API (status code #{response.status})" + end + private def connection(token) diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb index a730e0237..9cd1cbe1e 100644 --- a/spec/lib/profile_api_client_spec.rb +++ b/spec/lib/profile_api_client_spec.rb @@ -160,4 +160,73 @@ def list_safeguarding_flags described_class.safeguarding_flags(token:) end end + + describe '.create_safeguarding_flag' do + let(:flag) { 'school:owner' } + let(:create_safeguarding_flag_url) { "#{api_url}/api/v1/safeguarding-flags" } + + before do + stub_request(:post, create_safeguarding_flag_url).to_return(status: 201, body: '{}', headers: { 'content-type' => 'application/json' }) + end + + it 'makes a request to the profile api host' do + create_safeguarding_flag + expect(WebMock).to have_requested(:post, create_safeguarding_flag_url) + end + + it 'includes token in the authorization request header' do + create_safeguarding_flag + expect(WebMock).to have_requested(:post, create_safeguarding_flag_url).with(headers: { authorization: "Bearer #{token}" }) + end + + it 'includes the profile api key in the x-api-key request header' do + create_safeguarding_flag + expect(WebMock).to have_requested(:post, create_safeguarding_flag_url).with(headers: { 'x-api-key' => api_key }) + end + + it 'sets content-type of request to json' do + create_safeguarding_flag + expect(WebMock).to have_requested(:post, create_safeguarding_flag_url).with(headers: { 'content-type' => 'application/json' }) + end + + it 'sets accept header to json' do + create_safeguarding_flag + expect(WebMock).to have_requested(:post, create_safeguarding_flag_url).with(headers: { 'accept' => 'application/json' }) + end + + it 'sends the safeguarding flag in the request body' do + create_safeguarding_flag + expect(WebMock).to have_requested(:post, create_safeguarding_flag_url).with(body: { flag: }.to_json) + end + + it 'returns empty body if created successfully' do + stub_request(:post, create_safeguarding_flag_url) + .to_return(status: 201) + expect(create_safeguarding_flag).to be_nil + end + + it 'returns empty body if 303 response returned to indicate that the flag already exists' do + stub_request(:post, create_safeguarding_flag_url) + .to_return(status: 303) + expect(create_safeguarding_flag).to be_nil + end + + it 'raises exception if anything other than a 201 or 303 status code is returned' do + stub_request(:post, create_safeguarding_flag_url) + .to_return(status: 200) + + expect { create_safeguarding_flag }.to raise_error(RuntimeError) + end + + it 'includes details of underlying response when exception is raised' do + stub_request(:post, create_safeguarding_flag_url) + .to_return(status: 401) + + expect { create_safeguarding_flag }.to raise_error('Safeguarding flag not created in Profile API (status code 401)') + end + + def create_safeguarding_flag + described_class.create_safeguarding_flag(token:, flag:) + end + end end From 7a9ff1d9c51b463e90d66d7f686880fe81c4541f Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 2 Jul 2024 16:29:45 +0100 Subject: [PATCH 09/11] Add `ProfileApiClient.delete_safeguarding_flag` We're not using this at the moment but it makes it easier to test in development if we can create and delete safeguarding flags for a user. Note that I'm intentionally not doing anything different if `BYPASS_OAUTH` is set. Given how tightly coupled Editor API and Profile API are I'm less convinced it makes sense to attempt to run one without the other in development. API docs: https://my.raspberrypi.org/api/v1/documentation/#/Safeguarding%20Flags/delete_safeguarding_flags__flag_ --- lib/profile_api_client.rb | 8 +++++ spec/lib/profile_api_client_spec.rb | 53 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index 65bbcd448..72c22d306 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -222,6 +222,14 @@ def create_safeguarding_flag(token:, flag:) raise "Safeguarding flag not created in Profile API (status code #{response.status})" end + def delete_safeguarding_flag(token:, flag:) + response = connection(token).delete("/api/v1/safeguarding-flags/#{flag}") + + return if response.status == 204 + + raise "Safeguarding flag not deleted from Profile API (status code #{response.status})" + end + private def connection(token) diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb index 9cd1cbe1e..b21532f9a 100644 --- a/spec/lib/profile_api_client_spec.rb +++ b/spec/lib/profile_api_client_spec.rb @@ -229,4 +229,57 @@ def create_safeguarding_flag described_class.create_safeguarding_flag(token:, flag:) end end + + describe '.delete_safeguarding_flag' do + let(:flag) { 'school:owner' } + let(:delete_safeguarding_flag_url) { "#{api_url}/api/v1/safeguarding-flags/#{flag}" } + + before do + stub_request(:delete, delete_safeguarding_flag_url).to_return(status: 204, body: '') + end + + it 'makes a request to the profile api host' do + delete_safeguarding_flag + expect(WebMock).to have_requested(:delete, delete_safeguarding_flag_url) + end + + it 'includes token in the authorization request header' do + delete_safeguarding_flag + expect(WebMock).to have_requested(:delete, delete_safeguarding_flag_url).with(headers: { authorization: "Bearer #{token}" }) + end + + it 'includes the profile api key in the x-api-key request header' do + delete_safeguarding_flag + expect(WebMock).to have_requested(:delete, delete_safeguarding_flag_url).with(headers: { 'x-api-key' => api_key }) + end + + it 'sets accept header to json' do + delete_safeguarding_flag + expect(WebMock).to have_requested(:delete, delete_safeguarding_flag_url).with(headers: { 'accept' => 'application/json' }) + end + + it 'returns empty body if successful' do + stub_request(:delete, delete_safeguarding_flag_url) + .to_return(status: 204, body: '') + expect(delete_safeguarding_flag).to be_nil + end + + it 'raises exception if anything other than a 204 status code is returned' do + stub_request(:delete, delete_safeguarding_flag_url) + .to_return(status: 200) + + expect { delete_safeguarding_flag }.to raise_error(RuntimeError) + end + + it 'includes details of underlying response when exception is raised' do + stub_request(:delete, delete_safeguarding_flag_url) + .to_return(status: 401) + + expect { delete_safeguarding_flag }.to raise_error('Safeguarding flag not deleted from Profile API (status code 401)') + end + + def delete_safeguarding_flag + described_class.delete_safeguarding_flag(token:, flag:) + end + end end From 13d098a68c9f70bf15725651299bf27482ef7a4e Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 2 Jul 2024 17:10:00 +0100 Subject: [PATCH 10/11] Create Safeguarding flags in `SchoolStudentsController` The Profile API requires that the authenticated user has: - either the school:owner or school:teacher safeguarding flags set in order to be able to create, edit and list students - the school:owner safeguarding flag set in order to be able to delete students Safeguarding flags can only be set in Profile API for the authenticated user so we're conditionally creating them here before the user then attempts to interact with Profile API to CRUD students. --- .../api/school_students_controller.rb | 24 +++++++++++++++++ ...reating_a_batch_of_school_students_spec.rb | 27 +++++++++++++++++++ .../creating_a_school_student_spec.rb | 27 +++++++++++++++++++ .../deleting_a_school_student_spec.rb | 27 +++++++++++++++++++ .../listing_school_students_spec.rb | 27 +++++++++++++++++++ .../updating_a_school_student_spec.rb | 27 +++++++++++++++++++ spec/support/profile_api_mock.rb | 4 +++ 7 files changed, 163 insertions(+) diff --git a/app/controllers/api/school_students_controller.rb b/app/controllers/api/school_students_controller.rb index 4faec95b9..83abd59d3 100644 --- a/app/controllers/api/school_students_controller.rb +++ b/app/controllers/api/school_students_controller.rb @@ -5,6 +5,7 @@ class SchoolStudentsController < ApiController before_action :authorize_user load_and_authorize_resource :school authorize_resource :school_student, class: false + before_action :create_safeguarding_flags def index result = SchoolStudent::List.call(school: @school, token: current_user.token) @@ -62,5 +63,28 @@ def destroy def school_student_params params.require(:school_student).permit(:username, :password, :name) end + + def create_safeguarding_flags + create_teacher_safeguarding_flag + create_owner_safeguarding_flag + end + + def create_teacher_safeguarding_flag + return unless current_user.school_teacher?(@school) + + ProfileApiClient.create_safeguarding_flag( + token: current_user.token, + flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher] + ) + end + + def create_owner_safeguarding_flag + return unless current_user.school_owner?(@school) + + ProfileApiClient.create_safeguarding_flag( + token: current_user.token, + flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner] + ) + end end end diff --git a/spec/features/school_student/creating_a_batch_of_school_students_spec.rb b/spec/features/school_student/creating_a_batch_of_school_students_spec.rb index ffd8bb3f9..50dd724e3 100644 --- a/spec/features/school_student/creating_a_batch_of_school_students_spec.rb +++ b/spec/features/school_student/creating_a_batch_of_school_students_spec.rb @@ -6,6 +6,7 @@ before do authenticated_in_hydra_as(owner) stub_profile_api_create_school_student + stub_profile_api_create_safeguarding_flag end let(:headers) { { Authorization: UserProfileMock::TOKEN } } @@ -15,6 +16,16 @@ let(:file) { fixture_file_upload('students.csv') } + it 'creates the school owner safeguarding flag' do + post("/api/schools/#{school.id}/students/batch", headers:, params: { file: }) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'does not create the school teacher safeguarding flag' do + post("/api/schools/#{school.id}/students/batch", headers:, params: { file: }) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 204 No Content' do post("/api/schools/#{school.id}/students/batch", headers:, params: { file: }) expect(response).to have_http_status(:no_content) @@ -28,6 +39,22 @@ expect(response).to have_http_status(:no_content) end + it 'does not create the school owner safeguarding flag when the user is a school-teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + post("/api/schools/#{school.id}/students/batch", headers:, params: { file: }) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'creates the school teacher safeguarding flag when the user is a school-teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + post("/api/schools/#{school.id}/students/batch", headers:, params: { file: }) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 422 Unprocessable Entity when params are invalid' do post("/api/schools/#{school.id}/students/batch", headers:, params: {}) expect(response).to have_http_status(:unprocessable_entity) diff --git a/spec/features/school_student/creating_a_school_student_spec.rb b/spec/features/school_student/creating_a_school_student_spec.rb index 852a9d022..29d18bf64 100644 --- a/spec/features/school_student/creating_a_school_student_spec.rb +++ b/spec/features/school_student/creating_a_school_student_spec.rb @@ -6,6 +6,7 @@ before do authenticated_in_hydra_as(owner) stub_profile_api_create_school_student + stub_profile_api_create_safeguarding_flag end let(:headers) { { Authorization: UserProfileMock::TOKEN } } @@ -22,6 +23,16 @@ } end + it 'creates the school owner safeguarding flag' do + post("/api/schools/#{school.id}/students", headers:, params:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'does not create the school teacher safeguarding flag' do + post("/api/schools/#{school.id}/students", headers:, params:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 204 No Content' do post("/api/schools/#{school.id}/students", headers:, params:) expect(response).to have_http_status(:no_content) @@ -35,6 +46,22 @@ expect(response).to have_http_status(:no_content) end + it 'does not create the school owner safeguarding flag when the user is a school teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + post("/api/schools/#{school.id}/students", headers:, params:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'creates the school teacher safeguarding flag when the user is a school teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + post("/api/schools/#{school.id}/students", headers:, params:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 400 Bad Request when params are missing' do post("/api/schools/#{school.id}/students", headers:) expect(response).to have_http_status(:bad_request) diff --git a/spec/features/school_student/deleting_a_school_student_spec.rb b/spec/features/school_student/deleting_a_school_student_spec.rb index 10fc11d77..2053daeba 100644 --- a/spec/features/school_student/deleting_a_school_student_spec.rb +++ b/spec/features/school_student/deleting_a_school_student_spec.rb @@ -6,6 +6,7 @@ before do authenticated_in_hydra_as(owner) stub_profile_api_delete_school_student + stub_profile_api_create_safeguarding_flag end let(:headers) { { Authorization: UserProfileMock::TOKEN } } @@ -13,6 +14,16 @@ let(:student_id) { SecureRandom.uuid } let(:owner) { create(:owner, school:) } + it 'creates the school owner safeguarding flag' do + delete("/api/schools/#{school.id}/students/#{student_id}", headers:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'does not create the school teacher safeguarding flag' do + delete("/api/schools/#{school.id}/students/#{student_id}", headers:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 204 No Content' do delete("/api/schools/#{school.id}/students/#{student_id}", headers:) expect(response).to have_http_status(:no_content) @@ -39,6 +50,22 @@ expect(response).to have_http_status(:forbidden) end + it 'does not create the school owner safeguarding flag when logged in as a teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + delete("/api/schools/#{school.id}/students/#{student_id}", headers:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'does not create the school teacher safeguarding flag when logged in as a teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + delete("/api/schools/#{school.id}/students/#{student_id}", headers:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 403 Forbidden when the user is a school-student' do student = create(:student, school:) authenticated_in_hydra_as(student) diff --git a/spec/features/school_student/listing_school_students_spec.rb b/spec/features/school_student/listing_school_students_spec.rb index 413c34830..af6262126 100644 --- a/spec/features/school_student/listing_school_students_spec.rb +++ b/spec/features/school_student/listing_school_students_spec.rb @@ -7,6 +7,7 @@ authenticated_in_hydra_as(owner) stub_profile_api_list_school_students(user_id: student.id) stub_user_info_api_for(student) + stub_profile_api_create_safeguarding_flag end let(:headers) { { Authorization: UserProfileMock::TOKEN } } @@ -19,6 +20,16 @@ expect(response).to have_http_status(:ok) end + it 'creates the school owner safeguarding flag' do + get("/api/schools/#{school.id}/students", headers:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'does not create the school teacher safeguarding flag' do + get("/api/schools/#{school.id}/students", headers:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 200 OK when the user is a school-teacher' do teacher = create(:teacher, school:) authenticated_in_hydra_as(teacher) @@ -27,6 +38,22 @@ expect(response).to have_http_status(:ok) end + it 'does not create the school owner safeguarding flag when the user is a school teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + get("/api/schools/#{school.id}/students", headers:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'creates the school teacher safeguarding flag when the user is a school teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + get("/api/schools/#{school.id}/students", headers:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds with the school students JSON' do get("/api/schools/#{school.id}/students", headers:) data = JSON.parse(response.body, symbolize_names: true) diff --git a/spec/features/school_student/updating_a_school_student_spec.rb b/spec/features/school_student/updating_a_school_student_spec.rb index 1ecccd8a7..37e5cf9c6 100644 --- a/spec/features/school_student/updating_a_school_student_spec.rb +++ b/spec/features/school_student/updating_a_school_student_spec.rb @@ -6,6 +6,7 @@ before do authenticated_in_hydra_as(owner) stub_profile_api_update_school_student + stub_profile_api_create_safeguarding_flag end let(:headers) { { Authorization: UserProfileMock::TOKEN } } @@ -23,6 +24,16 @@ } end + it 'creates the school owner safeguarding flag' do + put("/api/schools/#{school.id}/students/#{student_id}", headers:, params:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'does not create the school teacher safeguarding flag' do + put("/api/schools/#{school.id}/students/#{student_id}", headers:, params:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 204 No Content' do put("/api/schools/#{school.id}/students/#{student_id}", headers:, params:) expect(response).to have_http_status(:no_content) @@ -36,6 +47,22 @@ expect(response).to have_http_status(:no_content) end + it 'does not create the school owner safeguarding flag when the user is a school teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + put("/api/schools/#{school.id}/students/#{student_id}", headers:, params:) + expect(ProfileApiClient).not_to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner]) + end + + it 'creates the school teacher safeguarding flag when the user is a school teacher' do + teacher = create(:teacher, school:) + authenticated_in_hydra_as(teacher) + + put("/api/schools/#{school.id}/students/#{student_id}", headers:, params:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token: UserProfileMock::TOKEN, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher]) + end + it 'responds 401 Unauthorized when no token is given' do put("/api/schools/#{school.id}/students/#{student_id}", params:) expect(response).to have_http_status(:unauthorized) diff --git a/spec/support/profile_api_mock.rb b/spec/support/profile_api_mock.rb index 25d680075..8e715328c 100644 --- a/spec/support/profile_api_mock.rb +++ b/spec/support/profile_api_mock.rb @@ -38,4 +38,8 @@ def stub_profile_api_update_school_student def stub_profile_api_delete_school_student allow(ProfileApiClient).to receive(:delete_school_student) end + + def stub_profile_api_create_safeguarding_flag + allow(ProfileApiClient).to receive(:create_safeguarding_flag) + end end From 2d8ec2e0effae961df3f1d2fd3414d2d7d574dba Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 2 Jul 2024 16:46:41 +0100 Subject: [PATCH 11/11] Implement `ProfileApiClient.create_school_student` Note that if the student is successfully created in Profile API then we receive their ID in the response. We use this to create the relevant student role and also make it available in the OperationResponse returned by `SchoolStudent::Create.call` so that clients of Editor API can use it if necessary. I'm stripping any extraneous leading and trailing spaces from name, username and password in ProfileApiClient.create_school_student to avoid the sorts of problems that can occur when usernames and passwords include these unnecessary spaces. I've tested the Profile API and it's not (currently, at least) doing any kind of normalizing of what we pass to it. Note that I'm intentionally not doing anything different if `BYPASS_OAUTH` is set. Given how tightly coupled Editor API and Profile API are I'm less convinced it makes sense to attempt to run one without the other in development. API docs: https://my.raspberrypi.org/api/v1/documentation/#/Students/post_schools__schoolId__students --- lib/concepts/school_student/create.rb | 13 ++- lib/concepts/school_student/create_batch.rb | 2 +- lib/profile_api_client.rb | 44 +++++++-- .../school_student/create_batch_spec.rb | 4 +- spec/concepts/school_student/create_spec.rb | 45 ++++++++- spec/lib/profile_api_client_spec.rb | 99 +++++++++++++++++++ spec/support/profile_api_mock.rb | 4 +- 7 files changed, 190 insertions(+), 21 deletions(-) diff --git a/lib/concepts/school_student/create.rb b/lib/concepts/school_student/create.rb index f6649be29..6bb8e4129 100644 --- a/lib/concepts/school_student/create.rb +++ b/lib/concepts/school_student/create.rb @@ -5,7 +5,11 @@ class Create class << self def call(school:, school_student_params:, token:) response = OperationResponse.new - create_student(school, school_student_params, token) + response[:student_id] = create_student(school, school_student_params, token) + response + rescue ProfileApiClient::CreateStudent422Error => e + Sentry.capture_exception(e) + response[:error] = "Error creating school student: #{e.error}" response rescue StandardError => e Sentry.capture_exception(e) @@ -16,14 +20,17 @@ def call(school:, school_student_params:, token:) private def create_student(school, school_student_params, token) - organisation_id = school.id + school_id = school.id username = school_student_params.fetch(:username) password = school_student_params.fetch(:password) name = school_student_params.fetch(:name) validate(school:, username:, password:, name:) - ProfileApiClient.create_school_student(token:, username:, password:, name:, organisation_id:) + response = ProfileApiClient.create_school_student(token:, username:, password:, name:, school_id:) + user_id = response[:created].first + Role.student.create!(school:, user_id:) + user_id end def validate(school:, username:, password:, name:) diff --git a/lib/concepts/school_student/create_batch.rb b/lib/concepts/school_student/create_batch.rb index 5329466a6..388b2b193 100644 --- a/lib/concepts/school_student/create_batch.rb +++ b/lib/concepts/school_student/create_batch.rb @@ -23,7 +23,7 @@ def create_batch(school, uploaded_file, token) validate(school:, sheet:) non_header_rows_with_content(sheet:).each do |name, username, password| - ProfileApiClient.create_school_student(token:, username:, password:, name:, organisation_id: school.id) + ProfileApiClient.create_school_student(token:, username:, password:, name:, school_id: school.id) end end diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index 72c22d306..ad5a7e8d3 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -6,6 +6,27 @@ class ProfileApiClient owner: 'school:owner' }.freeze + class Error < StandardError; end + + class CreateStudent422Error < Error + DEFAULT_ERROR = 'unknown error' + ERRORS = { + 'ERR_USER_EXISTS' => 'username has already been taken', + 'ERR_INVALID' => 'unknown validation error', + 'ERR_INVALID_PASSWORD' => 'password is invalid', + 'ERR_UNKNOWN' => DEFAULT_ERROR + }.freeze + + attr_reader :username, :error + + def initialize(error) + @username = error['username'] + @error = ERRORS.fetch(error['error'], DEFAULT_ERROR) + + super "Student not created in Profile API (status code 422, username '#{@username}', error '#{@error}')" + end + end + class << self # TODO: Replace with HTTP requests once the profile API has been built. @@ -146,19 +167,24 @@ def list_school_students(token:, organisation_id:) # The API should respond: # - 404 Not Found if the user doesn't exist # - 422 Unprocessable if the constraints are not met - def create_school_student(token:, username:, password:, name:, organisation_id:) + # rubocop:disable Metrics/AbcSize + def create_school_student(token:, username:, password:, name:, school_id:) return nil if token.blank? - _ = username - _ = password - _ = name - _ = organisation_id + response = connection(token).post("/api/v1/schools/#{school_id}/students") do |request| + request.body = [{ + name: name.strip, + username: username.strip, + password: password.strip + }] + end - # TODO: We should make Faraday raise a Ruby error for a non-2xx status - # code so that SchoolStudent::Create propagates the error in the response. - response = {} - response.deep_symbolize_keys + raise CreateStudent422Error, response.body['errors'].first if response.status == 422 + raise "Student not created in Profile API (status code #{response.status})" unless response.status == 201 + + response.body.deep_symbolize_keys end + # rubocop:enable Metrics/AbcSize # The API should enforce these constraints: # - The token has the school-owner or school-teacher role for the given organisation ID diff --git a/spec/concepts/school_student/create_batch_spec.rb b/spec/concepts/school_student/create_batch_spec.rb index 0cba73b3b..ba82fd3c1 100644 --- a/spec/concepts/school_student/create_batch_spec.rb +++ b/spec/concepts/school_student/create_batch_spec.rb @@ -21,7 +21,7 @@ # TODO: Replace with WebMock assertion once the profile API has been built. expect(ProfileApiClient).to have_received(:create_school_student) - .with(token:, username: 'jane123', password: 'secret123', name: 'Jane Doe', organisation_id: school.id) + .with(token:, username: 'jane123', password: 'secret123', name: 'Jane Doe', school_id: school.id) end it "makes a profile API call to create John Doe's account" do @@ -29,7 +29,7 @@ # TODO: Replace with WebMock assertion once the profile API has been built. expect(ProfileApiClient).to have_received(:create_school_student) - .with(token:, username: 'john123', password: 'secret456', name: 'John Doe', organisation_id: school.id) + .with(token:, username: 'john123', password: 'secret456', name: 'John Doe', school_id: school.id) end context 'when an .xlsx file is provided' do diff --git a/spec/concepts/school_student/create_spec.rb b/spec/concepts/school_student/create_spec.rb index 240c5d811..395c07eb0 100644 --- a/spec/concepts/school_student/create_spec.rb +++ b/spec/concepts/school_student/create_spec.rb @@ -5,6 +5,7 @@ RSpec.describe SchoolStudent::Create, type: :unit do let(:token) { UserProfileMock::TOKEN } let(:school) { create(:verified_school) } + let(:user_id) { SecureRandom.uuid } let(:school_student_params) do { @@ -15,7 +16,7 @@ end before do - stub_profile_api_create_school_student + stub_profile_api_create_school_student(user_id:) end it 'returns a successful operation response' do @@ -28,13 +29,23 @@ # TODO: Replace with WebMock assertion once the profile API has been built. expect(ProfileApiClient).to have_received(:create_school_student) - .with(token:, username: 'student-to-create', password: 'at-least-8-characters', name: 'School Student', organisation_id: school.id) + .with(token:, username: 'student-to-create', password: 'at-least-8-characters', name: 'School Student', school_id: school.id) + end + + it 'creates a role associating the student with the school' do + described_class.call(school:, school_student_params:, token:) + expect(Role.student.where(school:, user_id:)).to exist + end + + it 'returns the ID of the student created in Profile API' do + response = described_class.call(school:, school_student_params:, token:) + expect(response[:student_id]).to eq(user_id) end context 'when creation fails' do let(:school_student_params) do { - username: ' ', + username: '', password: 'at-least-8-characters', name: 'School Student' } @@ -56,7 +67,7 @@ it 'returns the error message in the operation response' do response = described_class.call(school:, school_student_params:, token:) - expect(response[:error]).to match(/username ' ' is invalid/) + expect(response[:error]).to match(/username '' is invalid/) end it 'sent the exception to Sentry' do @@ -73,4 +84,30 @@ expect(response[:error]).to match(/school is not verified/) end end + + context 'when the student cannot be created in profile api because of a 422 response' do + let(:error) { { 'username' => 'username', 'error' => 'ERR_USER_EXISTS' } } + let(:exception) { ProfileApiClient::CreateStudent422Error.new(error) } + + before do + allow(ProfileApiClient).to receive(:create_school_student).and_raise(exception) + end + + it 'adds a useful error message' do + response = described_class.call(school:, school_student_params:, token:) + expect(response[:error]).to eq('Error creating school student: username has already been taken') + end + end + + context 'when the student cannot be created in profile api because of a response other than 422' do + before do + allow(ProfileApiClient).to receive(:create_school_student) + .and_raise('Student not created in Profile API (status code 401)') + end + + it 'adds a useful error message' do + response = described_class.call(school:, school_student_params:, token:) + expect(response[:error]).to eq('Error creating school student: Student not created in Profile API (status code 401)') + end + end end diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb index b21532f9a..920b6c91f 100644 --- a/spec/lib/profile_api_client_spec.rb +++ b/spec/lib/profile_api_client_spec.rb @@ -12,6 +12,25 @@ allow(ENV).to receive(:fetch).with('PROFILE_API_KEY').and_return(api_key) end + describe ProfileApiClient::CreateStudent422Error do + subject(:exception) { described_class.new(error) } + + let(:error_code) { 'ERR_USER_EXISTS' } + let(:error) { { 'username' => 'username', 'error' => error_code } } + + it 'includes status code, username and translated error code in the message' do + expect(exception.message).to eq("Student not created in Profile API (status code 422, username 'username', error 'username has already been taken')") + end + + context "when the error isn't recognised" do + let(:error_code) { 'unrecognised-code' } + + it 'includes a default error message' do + expect(exception.message).to match(/error 'unknown error'/) + end + end + end + describe '.create_school' do let(:school) { build(:school, id: SecureRandom.uuid, code: SecureRandom.uuid) } let(:create_school_url) { "#{api_url}/api/v1/schools" } @@ -282,4 +301,84 @@ def delete_safeguarding_flag described_class.delete_safeguarding_flag(token:, flag:) end end + + describe '.create_school_student' do + let(:username) { 'username' } + let(:password) { 'password' } + let(:name) { 'name' } + let(:school) { build(:school, id: SecureRandom.uuid) } + let(:create_students_url) { "#{api_url}/api/v1/schools/#{school.id}/students" } + + before do + stub_request(:post, create_students_url).to_return(status: 201, body: '{}', headers: { 'content-type' => 'application/json' }) + end + + it 'makes a request to the profile api host' do + create_school_student + expect(WebMock).to have_requested(:post, create_students_url) + end + + it 'includes token in the authorization request header' do + create_school_student + expect(WebMock).to have_requested(:post, create_students_url).with(headers: { authorization: "Bearer #{token}" }) + end + + it 'includes the profile api key in the x-api-key request header' do + create_school_student + expect(WebMock).to have_requested(:post, create_students_url).with(headers: { 'x-api-key' => api_key }) + end + + it 'sets content-type of request to json' do + create_school_student + expect(WebMock).to have_requested(:post, create_students_url).with(headers: { 'content-type' => 'application/json' }) + end + + it 'sets accept header to json' do + create_school_student + expect(WebMock).to have_requested(:post, create_students_url).with(headers: { 'accept' => 'application/json' }) + end + + it 'sends the student details in the request body' do + create_school_student + expect(WebMock).to have_requested(:post, create_students_url).with(body: [{ name:, username:, password: }].to_json) + end + + it 'returns the id of the created student(s) if successful' do + response = { created: ['student-id'] } + stub_request(:post, create_students_url) + .to_return(status: 201, body: response.to_json, headers: { 'content-type' => 'application/json' }) + expect(create_school_student).to eq(response) + end + + it 'raises 422 exception if 422 status code is returned' do + response = { errors: [username: 'username', error: 'ERR_USER_EXISTS'] } + stub_request(:post, create_students_url) + .to_return(status: 422, body: response.to_json, headers: { 'content-type' => 'application/json' }) + + expect { create_school_student }.to raise_error(ProfileApiClient::CreateStudent422Error) + .with_message("Student not created in Profile API (status code 422, username 'username', error 'username has already been taken')") + end + + it 'raises exception if anything other that 201 status code is returned' do + stub_request(:post, create_students_url) + .to_return(status: 200) + + expect { create_school_student }.to raise_error('Student not created in Profile API (status code 200)') + end + + context 'when there are extraneous leading and trailing spaces in the student params' do + let(:username) { ' username ' } + let(:password) { ' password ' } + let(:name) { ' name ' } + + it 'strips the extraneous spaces' do + create_school_student + expect(WebMock).to have_requested(:post, create_students_url).with(body: [{ name: 'name', username: 'username', password: 'password' }].to_json) + end + end + + def create_school_student + described_class.create_school_student(token:, username:, password:, name:, school_id: school.id) + end + end end diff --git a/spec/support/profile_api_mock.rb b/spec/support/profile_api_mock.rb index 8e715328c..3e75d2576 100644 --- a/spec/support/profile_api_mock.rb +++ b/spec/support/profile_api_mock.rb @@ -27,8 +27,8 @@ def stub_profile_api_list_school_students(user_id:) allow(ProfileApiClient).to receive(:list_school_students).and_return(ids: [user_id]) end - def stub_profile_api_create_school_student - allow(ProfileApiClient).to receive(:create_school_student) + def stub_profile_api_create_school_student(user_id: SecureRandom.uuid) + allow(ProfileApiClient).to receive(:create_school_student).and_return(created: [user_id]) end def stub_profile_api_update_school_student