BRD-20938 - updates following first pass peer review

Author: jamesh-gomo

lib/scorm_engine/api/endpoints/courses.rb

@@ -35,15 +35,25 @@ def get_courses(options = {})
           result = Enumerator.new do |enum|
             if single_course_id
               # Single course endpoint returns course data directly
-              enum << ScormEngine::Models::Course.new_from_api(response.raw_response.body) if response.success? && response.raw_response.body.is_a?(Hash)
+              if response.success? && response.raw_response.body.is_a?(Hash)
+                enum << ScormEngine::Models::Course.new_from_api(response.raw_response.body)
+              end
             else
               # Multiple courses endpoint returns array in "courses" key
               loop do
-                response.success? && response.raw_response.body["courses"].each do |course|
+                break unless response.success? && response.raw_response.body.is_a?(Hash)
+                
+                courses = response.raw_response.body["courses"]
+                break unless courses.is_a?(Array)
+                
+                courses.each do |course|
                   enum << ScormEngine::Models::Course.new_from_api(course)
                 end
-                break if !response.success? || response.raw_response.body["more"].nil?
-                response = get(response.raw_response.body["more"])
+                
+                more_url = response.raw_response.body["more"]
+                break if more_url.nil?
+                
+                response = get(more_url)
               end
             end
           end

lib/scorm_engine/models/course_import.rb

@@ -27,23 +27,31 @@ def self.new_from_api(options = {})
         this.options = options.dup
 
         if options.key?("importResult")
-          # API v2 response structure: status is at top level, importResult contains nested data
-          # Also handles legacy test format with "result" and "importResult"
-          this.id = options["jobId"] || options["result"]
-          this.status = options["status"]&.upcase || options.fetch("importResult", {})["status"]&.upcase
-          this.parser_warnings = options.fetch("importResult", {})["parserWarnings"]
-          # Course data is nested in importResult for API v2
-          this.course = Course.new_from_api(options.fetch("importResult", {})["course"]) if options.fetch("importResult", {}).key?("course")
-        elsif options.key?("result") && options.size == 1
-          # Initial import response format: {"result": "job-id"}
-          this.id = options["result"]
-          this.status = "RUNNING" # Assume running for initial response
+          # API v2 response
+          import_result = options["importResult"] || {}
+
+          this.id              = options["jobId"] || options["result"]
+          this.status          = (options["status"] || import_result["status"])&.upcase
+          this.parser_warnings = import_result["parserWarnings"]
+
+          if import_result.key?("course")
+            this.course = Course.new_from_api(import_result["course"])
+          end
+
+        elsif options.keys == ["result"]
+          # Initial import response (legacy format: {"result" => "job-id"})
+          this.id     = options["result"]
+          this.status = "RUNNING"
+
         else
-          # API v1 response structure or full status response
-          this.id = options["jobId"]
-          this.status = options["status"]&.upcase
-          this.parser_warnings = options["parserWarnings"] # Handle API v1 parserWarnings at top level
-          this.course = Course.new_from_api(options["course"]) if options.key?("course") # unavailable in error states
+          # API v1 or full status response
+          this.id              = options["jobId"]
+          this.status          = options["status"]&.upcase
+          this.parser_warnings = options["parserWarnings"]
+
+          if options.key?("course")
+            this.course = Course.new_from_api(options["course"])
+          end
         end
 
         this

spec/scorm_engine/api/endpoints/courses/import_spec.rb

@@ -23,20 +23,21 @@
     end
 
     describe "arguments posted to the api" do
-      it "works in the general case" do
+      before do
         allow(client).to receive(:post)
+      end
+
+      it "works in the general case" do
         client.post_course_import(course_id: "id123", url: "http://path.to/scorm.zip")
         expect(client).to have_received(:post).with("courses/importJobs", { courseId: "id123", mayCreateNewVersion: false }, { url: "http://path.to/scorm.zip" })
       end
 
       it "allows creating a new version" do
-        allow(client).to receive(:post)
         client.post_course_import(course_id: "id123", url: "http://path.to/scorm.zip", may_create_new_version: true)
         expect(client).to have_received(:post).with("courses/importJobs", { courseId: "id123", mayCreateNewVersion: true }, { url: "http://path.to/scorm.zip" })
       end
 
       it "allows overriding course name" do
-        allow(client).to receive(:post)
         client.post_course_import(course_id: "id123", url: "http://path.to/scorm.zip", name: "the name")
         expect(client).to have_received(:post).with("courses/importJobs", { courseId: "id123", mayCreateNewVersion: false }, { url: "http://path.to/scorm.zip" })
       end

spec/scorm_engine/client_spec.rb

@@ -13,6 +13,25 @@
     it "defaults to API version 2" do
       expect(client.current_api_version).to eq(2)
     end
+
+    context "with tenant_creator provided" do
+      let(:tenant_creator) { instance_spy("TenantCreator") }
+      let(:client_with_creator) { described_class.new(tenant: tenant, tenant_creator: tenant_creator) }
+
+      it "sets the tenant_creator" do
+        expect(client_with_creator.tenant_creator).to eq(tenant_creator)
+      end
+
+      it "stores the tenant_creator for later use" do
+        expect(client_with_creator.instance_variable_get(:@tenant_creator)).to eq(tenant_creator)
+      end
+    end
+
+    context "without tenant_creator" do
+      it "sets tenant_creator to nil" do
+        expect(client.tenant_creator).to be_nil
+      end
+    end
   end
 
   describe "#current_api_version" do

spec/scorm_engine/faraday/connection_spec.rb

@@ -28,7 +28,7 @@
 
   describe "#connection" do
     describe "version" do
-      it "passes on to #base_uri" do
+      it "passes on to #base_uri with version 2" do
         client = ScormEngine::Client.new(tenant: "test")
 
         allow(client).to receive(:base_uri).and_call_original
@@ -37,6 +37,16 @@
 
         expect(client).to have_received(:base_uri).with(version: 2)
       end
+
+      it "passes on to #base_uri with version 1" do
+        client = ScormEngine::Client.new(tenant: "test")
+
+        allow(client).to receive(:base_uri).and_call_original
+
+        client.send(:connection, version: 1)
+
+        expect(client).to have_received(:base_uri).with(version: 1)
+      end
     end
   end
 end

spec/scorm_engine/faraday/request_spec.rb

@@ -197,135 +197,75 @@ def base_uri
     end
 
     describe "with tenant_creator provided" do
-      it "creates tenant and retries request successfully" do
-        tenant_creator = instance_spy("TenantCreator")
-        client = tenant_mock_class.new("test-tenant", tenant_creator: tenant_creator)
-        mock_connection = instance_spy(Faraday::Connection)
-        mock_request = instance_spy(Faraday::Request)
-
-        tenant_not_found_response = instance_double(
+      let(:tenant_creator) { instance_spy("TenantCreator") }
+      let(:client) { tenant_mock_class.new("test-tenant", tenant_creator: tenant_creator) }
+      let(:mock_connection) { instance_spy(Faraday::Connection) }
+      let(:mock_request) { instance_spy(Faraday::Request) }
+      let(:tenant_not_found_response) do
+        instance_double(
           Faraday::Response,
           status: 400,
           success?: false,
           body: { "message" => "test-tenant is not a valid tenant name" }
         )
+      end
 
+      before do
         allow(mock_connection).to receive(:get).and_yield(mock_request).and_return(tenant_not_found_response)
         allow(mock_request).to receive(:headers).and_return({})
         allow(mock_request).to receive(:url)
         allow(client).to receive(:connection).and_return(mock_connection)
+      end
 
+      it "creates tenant and retries request successfully" do
         response = client.send(:request, :get, "courses", {})
         expect(response.raw_response.status).to eq(400)
       end
 
       it "calls the connection when retrying" do
-        tenant_creator = instance_spy("TenantCreator")
-        client = tenant_mock_class.new("test-tenant", tenant_creator: tenant_creator)
-        mock_connection = instance_spy(Faraday::Connection)
-        mock_request = instance_spy(Faraday::Request)
-
-        tenant_not_found_response = instance_double(
-          Faraday::Response,
-          status: 400,
-          success?: false,
-          body: { "message" => "test-tenant is not a valid tenant name" }
-        )
-
-        allow(mock_connection).to receive(:get).and_yield(mock_request).and_return(tenant_not_found_response)
-        allow(mock_request).to receive(:headers).and_return({})
-        allow(mock_request).to receive(:url)
-        allow(client).to receive(:connection).and_return(mock_connection)
-
         client.send(:request, :get, "courses", {})
         expect(mock_connection).to have_received(:get).at_least(:once)
       end
 
       it "calls tenant_creator with the tenant name" do
-        tenant_creator = instance_spy("TenantCreator")
-        client = tenant_mock_class.new("test-tenant", tenant_creator: tenant_creator)
-        mock_connection = instance_spy(Faraday::Connection)
-        mock_request = instance_spy(Faraday::Request)
-
-        tenant_not_found_response = instance_double(
-          Faraday::Response,
-          status: 400,
-          success?: false,
-          body: { "message" => "test-tenant is not a valid tenant name" }
-        )
-
-        allow(mock_connection).to receive(:get).and_yield(mock_request).and_return(tenant_not_found_response)
-        allow(mock_request).to receive(:headers).and_return({})
-        allow(mock_request).to receive(:url)
-        allow(client).to receive(:connection).and_return(mock_connection)
-
         client.send(:request, :get, "courses", {})
         expect(tenant_creator).to have_received(:call).with("test-tenant")
       end
 
       it "returns original error if tenant creation fails" do
-        tenant_creator = instance_spy("TenantCreator")
-        client = tenant_mock_class.new("test-tenant", tenant_creator: tenant_creator)
-        mock_connection = instance_spy(Faraday::Connection)
-        mock_request = instance_spy(Faraday::Request)
-
-        tenant_not_found_response = instance_double(
-          Faraday::Response,
-          status: 400,
-          success?: false,
-          body: { "message" => "test-tenant is not a valid tenant name" }
-        )
-
         allow(tenant_creator).to receive(:call).and_raise(StandardError, "Creation failed")
-        allow(mock_connection).to receive(:get).and_yield(mock_request).and_return(tenant_not_found_response)
-        allow(mock_request).to receive(:headers).and_return({})
-        allow(mock_request).to receive(:url)
-        allow(client).to receive(:connection).and_return(mock_connection)
 
         response = client.send(:request, :get, "courses", {})
         expect(response.raw_response.status).to eq(400)
       end
     end
 
     describe "without tenant_creator" do
-      it "returns the original error" do
-        client = tenant_mock_class.new("test-tenant")
-        mock_connection = instance_spy(Faraday::Connection)
-        mock_request = instance_spy(Faraday::Request)
-
-        tenant_not_found_response = instance_double(
+      let(:client) { tenant_mock_class.new("test-tenant") }
+      let(:mock_connection) { instance_spy(Faraday::Connection) }
+      let(:mock_request) { instance_spy(Faraday::Request) }
+      let(:tenant_not_found_response) do
+        instance_double(
           Faraday::Response,
           status: 400,
           success?: false,
           body: { "message" => "test-tenant is not a valid tenant name" }
         )
+      end
 
+      before do
         allow(mock_connection).to receive(:get).and_yield(mock_request).and_return(tenant_not_found_response)
         allow(mock_request).to receive(:headers).and_return({})
         allow(mock_request).to receive(:url)
         allow(client).to receive(:connection).and_return(mock_connection)
+      end
 
+      it "returns the original error" do
         response = client.send(:request, :get, "courses", {})
         expect(response.raw_response.status).to eq(400)
       end
 
       it "calls the connection" do
-        client = tenant_mock_class.new("test-tenant")
-        mock_connection = instance_spy(Faraday::Connection)
-        mock_request = instance_spy(Faraday::Request)
-
-        tenant_not_found_response = instance_double(
-          Faraday::Response,
-          status: 400,
-          success?: false,
-          body: { "message" => "test-tenant is not a valid tenant name" }
-        )
-
-        allow(mock_connection).to receive(:get).and_yield(mock_request).and_return(tenant_not_found_response)
-        allow(mock_request).to receive(:headers).and_return({})
-        allow(mock_request).to receive(:url)
-        allow(client).to receive(:connection).and_return(mock_connection)
-
         client.send(:request, :get, "courses", {})
         expect(mock_connection).to have_received(:get)
       end
@@ -352,6 +292,35 @@ def base_uri
         wrapped_response = ScormEngine::Response.new(raw_response: server_error)
         expect(client.send(:should_retry_with_tenant_creation?, wrapped_response)).to be false
       end
+
+      context "when response_or_error responds to :response" do
+        it "returns true for tenant not found error in wrapped response" do
+          client = tenant_mock_class.new("test-tenant")
+          tenant_error = instance_double(Faraday::Response, status: 400, body: "test-tenant is not a valid tenant name")
+          error_with_response = instance_double("ErrorObject", response: tenant_error)
+          expect(client.send(:should_retry_with_tenant_creation?, error_with_response)).to be true
+        end
+
+        it "returns false for other 400 errors in wrapped response" do
+          client = tenant_mock_class.new("test-tenant")
+          other_error = instance_double(Faraday::Response, status: 400, body: "Some other validation error")
+          error_with_response = instance_double("ErrorObject", response: other_error)
+          expect(client.send(:should_retry_with_tenant_creation?, error_with_response)).to be false
+        end
+
+        it "returns false for non-400 errors in wrapped response" do
+          client = tenant_mock_class.new("test-tenant")
+          server_error = instance_double(Faraday::Response, status: 500, body: "Internal server error")
+          error_with_response = instance_double("ErrorObject", response: server_error)
+          expect(client.send(:should_retry_with_tenant_creation?, error_with_response)).to be false
+        end
+
+        it "handles nil response gracefully" do
+          client = tenant_mock_class.new("test-tenant")
+          error_with_nil_response = instance_double("ErrorObject", response: nil)
+          expect(client.send(:should_retry_with_tenant_creation?, error_with_nil_response)).to be false
+        end
+      end
     end
   end
 end

spec/scorm_engine/models/course_configuration_spec.rb

@@ -35,6 +35,39 @@
       end
     end
 
+    context "with valid setting items (alternative key)" do
+      let(:valid_setting_items_response) do
+        {
+          "settingItems" => [
+            {
+              "id" => "setting_one",
+              "value" => "value_one"
+            },
+            {
+              "id" => "setting_two",
+              "value" => "value_two"
+            },
+            {
+              "id" => "boolean_setting",
+              "value" => "true"
+            },
+          ]
+        }
+      end
+
+      it "converts setting items to hash correctly type" do
+        settings = described_class.get_settings_from_api(valid_setting_items_response)
+
+        expect(settings).to be_a(Hash)
+      end
+
+      it "converts setting items to hash correctly value" do
+        settings = described_class.get_settings_from_api(valid_setting_items_response)
+
+        expect(settings).to eq({ "setting_one" => "value_one", "setting_two" => "value_two", "boolean_setting" => "true" })
+      end
+    end
+
     context "with empty configuration items" do
       let(:empty_response) do
         {