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

Author: chrisroos

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:)

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
 

lib/profile_api_client.rb

@@ -6,6 +6,26 @@ class ProfileApiClient
     owner: 'school:owner'
   }.freeze
 
+  class Error < StandardError; end
+
+  class CreateStudent422Error < Error
+    ERRORS = {
+      'ERR_USER_EXISTS' => 'username has already been taken',
+      'ERR_INVALID' => 'unknown validation error',
+      'ERR_INVALID_PASSWORD' => 'password is invalid',
+      'ERR_UNKNOWN' => 'unknown error'
+    }.freeze
+
+    attr_reader :username, :error
+
+    def initialize(error)
+      @username = error['username']
+      @error = ERRORS[error['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 +166,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

spec/concepts/school_student/create_batch_spec.rb

@@ -21,15 +21,15 @@
 
     # 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
     described_class.call(school:, uploaded_file: file, token:)
 
     # 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

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

spec/lib/profile_api_client_spec.rb

@@ -282,4 +282,91 @@ 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
+
+    it 'includes details of underlying response when exception is raised' do
+      stub_request(:post, create_students_url)
+        .to_return(status: 401)
+
+      expect { create_school_student }.to raise_error('Student not created in Profile API (status code 401)')
+    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

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