Expand Profile API client (#379)

This PR makes some general improvements to the `ProfileApiClient` before
expanding it to implement the following additional functionality in
Profile API:

- Listing students
- Getting a student
- Updating a student
- Deleting a student

Overview of other changes in this PR:

- Use Ruby's `Data` class to return value objects from some of our
`ProfileApiClient` methods. I prefer this to passing hashes around as I
think it makes the code easier to reason about and because it means we
fail fast if the response we receive from the Profile API is different
to what we're expecting.
- Configure Faraday to raise exceptions for 4xx and 5xx responses from
Profile API. The only one of these responses we can currently handle is
the 422 Unprocessable Entity response that can be returned when creating
or updating a student.
- Raise a ProfileApiClient::UnexpectedResponse exception if we receive
any other notionally successful response (i.e. not 4xx or 5xx) than the
response we're expecting.
- Remove boiler plate code and unnecessary comments

## Examples of using this client to interact with Profile API

```ruby
> token = '<your-token>'

# Create a school
> school = ProfileApiClient.create_school(token:, id: SecureRandom.uuid, code: SchoolCodeGenerator.generate)
=> #<data ProfileApiClient::School id="bd1ef16c-7f80-4c04-9d2f-8689a7bd4511", schoolCode="06-27-42", updatedAt="2024-07-10T14:24:15.130Z", createdAt="2024-07-10T14:24:15.130Z", discardedAt=nil>

# List safeguarding flags - user only has teacher flag
> ProfileApiClient.safeguarding_flags(token:)
=> 
[#<data ProfileApiClient::SafeguardingFlag
  id="5f741142-ad7b-41bb-9722-f6ea4b37b2ee",
  userId="583ba872-b16e-46e1-9f7d-df89d267550d",
  flag="school:teacher",
  email="[email protected]",
  createdAt="2024-07-10T14:22:20.392Z",
  updatedAt="2024-07-10T14:22:20.392Z",
  discardedAt=nil>]

# Create owner safeguarding flag
> ProfileApiClient.create_safeguarding_flag(token:, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:owner])
=> nil

# Delete teacher safeguarding flag
> ProfileApiClient.delete_safeguarding_flag(token:, flag: ProfileApiClient::SAFEGUARDING_FLAGS[:teacher])
=> nil

# List safeguarding flags - user only has owner flag
> ProfileApiClient.safeguarding_flags(token:)
=> 
[#<data ProfileApiClient::SafeguardingFlag
  id="6137f101-ac60-4365-8ec9-91d5ffbd198b",
  userId="583ba872-b16e-46e1-9f7d-df89d267550d",
  flag="school:owner",
  email="[email protected]",
  createdAt="2024-07-10T14:25:49.912Z",
  updatedAt="2024-07-10T14:25:49.912Z",
  discardedAt=nil>]

# Create a student
> response = ProfileApiClient.create_school_student(token:, school_id: school.id, username: 'username', password: 'password$123', name: 'name')
=> {:created=>["62045970-5957-4ac0-b8a1-96ae59dac58d"]}
> student_id = response[:created].first
=> "62045970-5957-4ac0-b8a1-96ae59dac58d"

# Create a student for non-existent school
> ProfileApiClient.create_school_student(token:, school_id: SecureRandom.uuid, username: 'username', password: 'password$123', name: 'name')
lib/profile_api_client.rb:105:in 'create_school_student': the server responded with status 404 (Faraday::ResourceNotFound)

# Create a student with invalid password
> ProfileApiClient.create_school_student(token:, school_id: school.id, username: 'username', password: 'password', name: 'name')
lib/profile_api_client.rb:117:in 'rescue in create_school_student': Student not saved in Profile API (status code 422, username 'username', error 'password is invalid') (ProfileApiClient::Student422Error)

# Create a student with duplicate username
> ProfileApiClient.create_school_student(token:, school_id: school.id, username: 'username', password: 'password$123', name: 'name')
lib/profile_api_client.rb:117:in 'rescue in create_school_student': Student not saved in Profile API (status code 422, username 'username', error 'username has already been taken') (ProfileApiClient::Student422Error)

# Get student
> ProfileApiClient.school_student(token:, school_id: school.id, student_id:)
=> 
#<data ProfileApiClient::Student
 id="62045970-5957-4ac0-b8a1-96ae59dac58d",
 schoolId="bd1ef16c-7f80-4c04-9d2f-8689a7bd4511",
 name="name",
 username="username",
 createdAt="2024-07-10T14:27:50.147Z",
 updatedAt="2024-07-10T14:27:50.147Z",
 discardedAt=nil>

# Update student
> ProfileApiClient.update_school_student(token:, school_id: school.id, student_id:, username: 'new-username', password: 'new-password', name: 'new-name')
=> 
#<data ProfileApiClient::Student
 id="62045970-5957-4ac0-b8a1-96ae59dac58d",
 schoolId="bd1ef16c-7f80-4c04-9d2f-8689a7bd4511",
 name="new-name",
 username="new-username",
 createdAt="2024-07-10T14:27:50.147Z",
 updatedAt="2024-07-10T14:33:19.707Z",
 discardedAt=nil>

# List students
> ProfileApiClient.list_school_students(token:, school_id: school.id, student_ids: [student_id])
=> 
[#<data ProfileApiClient::Student
  id="62045970-5957-4ac0-b8a1-96ae59dac58d",
  schoolId="bd1ef16c-7f80-4c04-9d2f-8689a7bd4511",
  name="new-name",
  username="new-username",
  createdAt="2024-07-10T14:27:50.147Z",
  updatedAt="2024-07-10T14:33:19.707Z",
  discardedAt=nil>]

# List students - error unless all student ids belong to school in Profile API
> ProfileApiClient.list_school_students(token:, school_id: school.id, student_ids: [student_id, SecureRandom.uuid])
lib/profile_api_client.rb:93:in 'list_school_students': the server responded with status 404 (Faraday::ResourceNotFound)

# Delete student
> ProfileApiClient.delete_school_student(token:, school_id: school.id, student_id:)
=> nil
```

Author: sra405

app/controllers/api/school_students_controller.rb

@@ -39,7 +39,9 @@ def create_batch
     end
 
     def update
-      result = SchoolStudent::Update.call(school: @school, school_student_params:, token: current_user.token)
+      result = SchoolStudent::Update.call(
+        school: @school, student_id: params[:id], school_student_params:, token: current_user.token
+      )
 
       if result.success?
         head :no_content

lib/concepts/school_student/create.rb

@@ -7,7 +7,7 @@ def call(school:, school_student_params:, token:)
         response = OperationResponse.new
         response[:student_id] = create_student(school, school_student_params, token)
         response
-      rescue ProfileApiClient::CreateStudent422Error => e
+      rescue ProfileApiClient::Student422Error => e
         Sentry.capture_exception(e)
         response[:error] = "Error creating school student: #{e.error}"
         response

lib/concepts/school_student/delete.rb

@@ -16,7 +16,7 @@ def call(school:, student_id:, token:)
       private
 
       def delete_student(school, student_id, token)
-        ProfileApiClient.delete_school_student(token:, student_id:, organisation_id: school.id)
+        ProfileApiClient.delete_school_student(token:, student_id:, school_id: school.id)
       end
     end
   end

lib/concepts/school_student/list.rb

@@ -16,10 +16,10 @@ def call(school:, token:)
       private
 
       def list_students(school, token)
-        response = ProfileApiClient.list_school_students(token:, organisation_id: school.id)
-        user_ids = response.fetch(:ids)
-
-        User.from_userinfo(ids: user_ids)
+        student_ids = Role.student.where(school:).map(&:user_id)
+        ProfileApiClient.list_school_students(token:, school_id: school.id, student_ids:).map do |student|
+          User.new(student.to_h.slice(:id, :username, :name))
+        end
       end
     end
   end

lib/concepts/school_student/update.rb

@@ -3,9 +3,9 @@
 module SchoolStudent
   class Update
     class << self
-      def call(school:, school_student_params:, token:)
+      def call(school:, student_id:, school_student_params:, token:)
         response = OperationResponse.new
-        update_student(school, school_student_params, token)
+        update_student(school, student_id, school_student_params, token)
         response
       rescue StandardError => e
         Sentry.capture_exception(e)
@@ -15,17 +15,16 @@ def call(school:, school_student_params:, token:)
 
       private
 
-      def update_student(school, school_student_params, token)
+      def update_student(school, student_id, school_student_params, token)
         username = school_student_params.fetch(:username, nil)
         password = school_student_params.fetch(:password, nil)
         name = school_student_params.fetch(:name, nil)
 
         validate(username:, password:, name:)
 
-        attributes_to_update = { username:, password:, name: }.compact
-        return if attributes_to_update.empty?
-
-        ProfileApiClient.update_school_student(token:, attributes_to_update:, organisation_id: school.id)
+        ProfileApiClient.update_school_student(
+          token:, school_id: school.id, student_id:, username:, password:, name:
+        )
       end
 
       def validate(username:, password:, name:)

lib/profile_api_client.rb

@@ -6,9 +6,13 @@ class ProfileApiClient
     owner: 'school:owner'
   }.freeze
 
+  School = Data.define(:id, :schoolCode, :updatedAt, :createdAt, :discardedAt)
+  SafeguardingFlag = Data.define(:id, :userId, :flag, :email, :createdAt, :updatedAt, :discardedAt)
+  Student = Data.define(:id, :schoolId, :name, :username, :createdAt, :updatedAt, :discardedAt)
+
   class Error < StandardError; end
 
-  class CreateStudent422Error < Error
+  class Student422Error < Error
     DEFAULT_ERROR = 'unknown error'
     ERRORS = {
       'ERR_USER_EXISTS' => 'username has already been taken',
@@ -23,13 +27,23 @@ 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}')"
+      super "Student not saved 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.
+  class UnexpectedResponse < Error
+    attr_reader :response_status, :response_headers, :response_body
+
+    def initialize(response)
+      @response_status = response.status
+      @response_headers = response.headers
+      @response_body = response.body
+
+      super "Unexpected response from Profile API (status code #{response.status})"
+    end
+  end
 
+  class << self
     def create_school(token:, id:, code:)
       return { 'id' => id, 'schoolCode' => code } if ENV['BYPASS_OAUTH'].present?
 
@@ -40,134 +54,51 @@ def create_school(token:, id:, code:)
         }
       end
 
-      raise "School not created in Profile API (status code #{response.status})" unless response.status == 201
+      raise UnexpectedResponse, response unless response.status == 201
 
-      response.body
+      School.new(**response.body)
     end
 
-    # The API should enforce these constraints:
-    # - The token has the school-owner or school-teacher role for the given organisation ID
-    # - The token user or given user should not be under 13
-    # - The email must be verified
-    #
-    # The API should respond:
-    # - 422 Unprocessable if the constraints are not met
-    def list_school_owners(token:, organisation_id:)
-      return [] if token.blank?
-
-      _ = organisation_id
-
-      # TODO: We should make Faraday raise a Ruby error for a non-2xx status
-      # code so that SchoolOwner::Invite propagates the error in the response.
-      response = { 'ids' => ['99999999-9999-9999-9999-999999999999'] }
-      response.deep_symbolize_keys
+    def list_school_owners(*)
+      {}
     end
 
-    # The API should enforce these constraints:
-    # - The token has the school-owner role for the given organisation ID
-    # - The token user or given user should not be under 13
-    # - The email must be verified
-    #
-    # The API should respond:
-    # - 404 Not Found if the user doesn't exist
-    # - 422 Unprocessable if the constraints are not met
-    def invite_school_owner(token:, email_address:, organisation_id:)
-      return nil if token.blank?
-
-      _ = email_address
-      _ = organisation_id
-
-      # TODO: We should make Faraday raise a Ruby error for a non-2xx status
-      # code so that SchoolOwner::Invite propagates the error in the response.
-      response = {}
-      response.deep_symbolize_keys
+    def invite_school_owner(*)
+      {}
     end
 
-    # The API should enforce these constraints:
-    # - The token has the school-owner role for the given organisation ID
-    # - The token user should not be under 13
-    # - The email must be verified
-    #
-    # The API should respond:
-    # - 404 Not Found if the user doesn't exist
-    # - 422 Unprocessable if the constraints are not met
-    def remove_school_owner(token:, owner_id:, organisation_id:)
-      return nil if token.blank?
-
-      _ = owner_id
-      _ = organisation_id
-
-      # TODO: We should make Faraday raise a Ruby error for a non-2xx status
-      # code so that SchoolOwner::Remove propagates the error in the response.
-      response = {}
-      response.deep_symbolize_keys
+    def remove_school_owner(*)
+      {}
     end
 
-    # The API should enforce these constraints:
-    # - The token has the school-owner or school-teacher role for the given organisation ID
-    # - The token user or given user should not be under 13
-    # - The email must be verified
-    #
-    # The API should respond:
-    # - 422 Unprocessable if the constraints are not met
-    def list_school_teachers(token:, organisation_id:)
-      return [] if token.blank?
-
-      _ = organisation_id
+    def list_school_teachers(*)
+      {}
+    end
 
-      # TODO: We should make Faraday raise a Ruby error for a non-2xx status
-      # code so that SchoolOwner::Invite propagates the error in the response.
-      response = { 'ids' => ['99999999-9999-9999-9999-999999999999'] }
-      response.deep_symbolize_keys
+    def remove_school_teacher(*)
+      {}
     end
 
-    # The API should enforce these constraints:
-    # - The token has the school-owner role for the given organisation ID
-    # - The token user should not be under 13
-    # - The email must be verified
-    #
-    # The API should respond:
-    # - 404 Not Found if the user doesn't exist
-    # - 422 Unprocessable if the constraints are not met
-    def remove_school_teacher(token:, teacher_id:, organisation_id:)
-      return nil if token.blank?
+    def school_student(token:, school_id:, student_id:)
+      response = connection(token).get("/api/v1/schools/#{school_id}/students/#{student_id}")
 
-      _ = teacher_id
-      _ = organisation_id
+      raise UnexpectedResponse, response unless response.status == 200
 
-      # TODO: We should make Faraday raise a Ruby error for a non-2xx status
-      # code so that SchoolOwner::Remove propagates the error in the response.
-      response = {}
-      response.deep_symbolize_keys
+      Student.new(**response.body)
     end
 
-    # The API should enforce these constraints:
-    # - The token has the school-owner or school-teacher role for the given organisation ID
-    # - The token user or given user should not be under 13
-    # - The email must be verified
-    #
-    # The API should respond:
-    # - 422 Unprocessable if the constraints are not met
-    def list_school_students(token:, organisation_id:)
+    def list_school_students(token:, school_id:, student_ids:)
       return [] if token.blank?
 
-      _ = organisation_id
+      response = connection(token).post("/api/v1/schools/#{school_id}/students/list") do |request|
+        request.body = student_ids
+      end
+
+      raise UnexpectedResponse, response unless response.status == 200
 
-      # TODO: We should make Faraday raise a Ruby error for a non-2xx status
-      # code so that SchoolOwner::Invite propagates the error in the response.
-      response = { 'ids' => ['99999999-9999-9999-9999-999999999999'] }
-      response.deep_symbolize_keys
+      response.body.map { |attrs| Student.new(**attrs.symbolize_keys) }
     end
 
-    # The API should enforce these constraints:
-    # - The token has the school-owner or school-teacher role for the given organisation ID
-    # - The token user should not be under 13
-    # - The email must be verified
-    #
-    # The API should respond:
-    # - 404 Not Found if the user doesn't exist
-    # - 422 Unprocessable if the constraints are not met
-    # rubocop:disable Metrics/AbcSize
     def create_school_student(token:, username:, password:, name:, school_id:)
       return nil if token.blank?
 
@@ -179,81 +110,61 @@ def create_school_student(token:, username:, password:, name:, school_id:)
         }]
       end
 
-      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
+      raise UnexpectedResponse, response unless response.status == 201
 
       response.body.deep_symbolize_keys
+    rescue Faraday::UnprocessableEntityError => e
+      raise Student422Error, JSON.parse(e.response_body)['errors'].first
     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
-    # - The token user should not be under 13
-    # - The email must be verified
-    # - The student_id must be a school-student for the given organisation ID
-    #
-    # The API should respond:
-    # - 404 Not Found if the user doesn't exist
-    # - 422 Unprocessable if the constraints are not met
-    def update_school_student(token:, attributes_to_update:, organisation_id:)
+    # rubocop:disable Metrics/AbcSize
+    def update_school_student(token:, school_id:, student_id:, name: nil, username: nil, password: nil) # rubocop:disable Metrics/ParameterLists
       return nil if token.blank?
 
-      _ = attributes_to_update
-      _ = organisation_id
+      response = connection(token).patch("/api/v1/schools/#{school_id}/students/#{student_id}") do |request|
+        request.body = {
+          name: name&.strip,
+          username: username&.strip,
+          password: password&.strip
+        }.compact
+      end
+
+      raise UnexpectedResponse, response unless response.status == 200
 
-      # TODO: We should make Faraday raise a Ruby error for a non-2xx status
-      # code so that SchoolOwner::Remove propagates the error in the response.
-      response = {}
-      response.deep_symbolize_keys
+      Student.new(**response.body)
+    rescue Faraday::UnprocessableEntityError => e
+      raise Student422Error, JSON.parse(e.response_body)['errors'].first
     end
+    # rubocop:enable Metrics/AbcSize
 
-    # The API should enforce these constraints:
-    # - The token has the school-owner role for the given organisation ID
-    # - The token user should not be under 13
-    # - The email must be verified
-    # - The student_id must be a school-student for the given organisation ID
-    #
-    # The API should respond:
-    # - 404 Not Found if the user doesn't exist
-    # - 422 Unprocessable if the constraints are not met
-    def delete_school_student(token:, student_id:, organisation_id:)
+    def delete_school_student(token:, school_id:, student_id:)
       return nil if token.blank?
 
-      _ = student_id
-      _ = organisation_id
+      response = connection(token).delete("/api/v1/schools/#{school_id}/students/#{student_id}")
 
-      # TODO: We should make Faraday raise a Ruby error for a non-2xx status
-      # code so that SchoolOwner::Remove propagates the error in the response.
-      response = {}
-      response.deep_symbolize_keys
+      raise UnexpectedResponse, response unless response.status == 204
     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
+      raise UnexpectedResponse, response unless response.status == 200
 
-      response.body.map(&:deep_symbolize_keys)
+      response.body.map { |flag| SafeguardingFlag.new(**flag.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})"
+      raise UnexpectedResponse, response unless [201, 303].include?(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})"
+      raise UnexpectedResponse, response unless response.status == 204
     end
 
     private
@@ -262,6 +173,7 @@ def connection(token)
       Faraday.new(ENV.fetch('IDENTITY_URL')) do |faraday|
         faraday.request :json
         faraday.response :json
+        faraday.response :raise_error
         faraday.headers = {
           'Accept' => 'application/json',
           'Authorization' => "Bearer #{token}",