Add retry when calling OS

closes OUT-5227
closes OUT-5391
flag=outcome_service_results_to_canvas

Test Plan
- dc down on outcome service
- In canvas, navigate to account settings
- ensure you have outcome_service_results_to_canvas flag enabled
- go to reports and generate the Outcome Results report
- it will error out because it cannot communicate with OS
- Bring OS back up
- generate the report again and verify it didn't fail

Change-Id: I9ee6ea5220fc458e1dd374bf5def092085403e4b
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/304019
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Martin Yosifov <martin.yosifov@instructure.com>
Reviewed-by: Kyle Rosenbaum <krosenbaum@instructure.com>
Product-Review: Kyle Rosenbaum <krosenbaum@instructure.com>
QA-Review: Chrystal Langston <chrystal.langston@instructure.com>

app/helpers/canvas_outcomes_helper.rb

@@ -19,6 +19,10 @@
 #
 
 module CanvasOutcomesHelper
+  MAX_RETRIES = 3
+
+  class OSFetchError < StandardError; end
+
   def get_outcome_alignments(context, outcome_ids, additional_params = nil)
     return if outcome_ids.blank? || context.blank? || !context.feature_enabled?(:outcome_service_results_to_canvas)
 
@@ -68,25 +72,36 @@ module CanvasOutcomesHelper
     return if domain.nil? || jwt.nil?
 
     protocol = ENV.fetch("OUTCOMES_SERVICE_PROTOCOL", Rails.env.production? ? "https" : "http")
-    response = CanvasHttp.get(
+    retry_count = 0
-      build_request_url(protocol, domain, "api/authoritative_results", params),
+    begin
-      {
+      response = CanvasHttp.get(
-        "Authorization" => jwt
+        build_request_url(protocol, domain, "api/authoritative_results", params),
-      }
+        {
-    )
+          "Authorization" => jwt
+        }
+      )
+    rescue
+      retry_count += 1
+      retry if retry_count < MAX_RETRIES
+      raise OSFetchError, "Failed to fetch results for context #{context.id} #{params}"
+    end
 
     if /^2/.match?(response.code.to_s)
-      results = JSON.parse(response.body).deep_symbolize_keys[:results]
+      begin
-      results.each do |result|
+        results = JSON.parse(response.body).deep_symbolize_keys[:results]
-        next if result[:attempts].nil?
+        results.each do |result|
+          next if result[:attempts].nil?
 
-        result[:attempts].each do |attempt|
+          result[:attempts].each do |attempt|
-          attempt[:metadata] = JSON.parse(attempt[:metadata]).deep_symbolize_keys
+            attempt[:metadata] = JSON.parse(attempt[:metadata]).deep_symbolize_keys unless attempt[:metadata].nil?
+          end
         end
+        results
+      rescue
+        raise OSFetchError, "Error parsing JSON results from Outcomes Service: #{response.body}"
       end
-      results
     else
-      raise "Error retrieving results from Outcomes Service: #{response.body}"
+      raise OSFetchError, "Error retrieving results from Outcomes Service: #{response.body}"
     end
   end
 

gems/plugins/account_reports/spec_canvas/outcome_reports_spec.rb

@@ -525,6 +525,82 @@ describe "Outcome Reports" do
         end
       end
 
+      context ":outcome_service_results_to_canvas - when outcomes service fails" do
+        before do
+          settings = { consumer_key: "key", jwt_secret: "secret", domain: "domain" }
+          @root_account.settings[:provision] = { "outcomes" => settings }
+          @root_account.save!
+          @root_account.set_feature_flag!(:outcome_service_results_to_canvas, "on")
+        end
+
+        after do
+          @root_account.settings[:provision] = nil
+          @root_account.save!
+          @root_account.set_feature_flag!(:outcome_service_results_to_canvas, "off")
+        end
+
+        it "errors the report" do
+          expect(CanvasHttp).to receive(:get).and_raise("failed call").exactly(3).times
+          report_record = run_report(report_type, account: @root_account, params: { "include_deleted" => true })
+          expect(report_record.workflow_state).to eq "error"
+          expect(report_record.message).to start_with "Generating the report, Outcome Results CSV, failed."
+          expect(report_record.parameters["extra_text"]).to eq "Failed, the report failed to generate a file. Please try again."
+        end
+      end
+
+      context ":outcome_service_results_to_canvas - json parsing" do
+        before do
+          settings = { consumer_key: "key", jwt_secret: "secret", domain: "domain" }
+          @root_account.settings[:provision] = { "outcomes" => settings }
+          @root_account.save!
+          @root_account.set_feature_flag!(:outcome_service_results_to_canvas, "on")
+        end
+
+        after do
+          @root_account.settings[:provision] = nil
+          @root_account.save!
+          @root_account.set_feature_flag!(:outcome_service_results_to_canvas, "off")
+        end
+
+        def json_string_null_metadata
+          {
+            results: [
+              {
+                attempts: [
+                  {
+                    points: 1,
+                    points_possible: 2
+                  }
+                ]
+              }
+            ]
+          }.to_json
+        end
+
+        it "can handle null metadata" do
+          response = Net::HTTPSuccess.new(1.1, 200, "OK")
+          expect(response).to receive(:body).and_return(json_string_null_metadata)
+          expect(CanvasHttp).to receive(:get).with(any_args).and_return(response).once
+
+          report_record = run_report(report_type, account: @root_account, params: { "include_deleted" => true })
+          expect(report_record.workflow_state).to eq "complete"
+          expect(report_record.message).to eq "Outcome Results report successfully generated with the following settings. Account: New Account; Term: All Terms; Include Deleted Objects;"
+          expect(report_record.parameters["extra_text"]).to eq "Term: All Terms; Include Deleted Objects;"
+        end
+
+        it "errors if not valid json" do
+          response = Net::HTTPSuccess.new(1.1, 200, "OK")
+          # once for parsing and then once for throwing the error
+          expect(response).to receive(:body).and_return("not_valid_json").twice
+          expect(CanvasHttp).to receive(:get).with(any_args).and_return(response).once
+
+          report_record = run_report(report_type, account: @root_account, params: { "include_deleted" => true })
+          expect(report_record.workflow_state).to eq "error"
+          expect(report_record.message).to start_with "Generating the report, Outcome Results CSV, failed."
+          expect(report_record.parameters["extra_text"]).to eq "Failed, the report failed to generate a file. Please try again."
+        end
+      end
+
       context ":outcome_service_results_to_canvas" do
         # Column indexes
         student_name = 0

spec/helpers/canvas_outcomes_helper_spec.rb

@@ -377,9 +377,14 @@ describe CanvasOutcomesHelper do
             @course.enable_feature!(:outcome_service_results_to_canvas)
           end
 
+          it "throws OSFetchError if call fails" do
+            expect(CanvasHttp).to receive(:get).and_raise("failed call").exactly(3).times
+            expect { subject.get_lmgb_results(@course, "1", "assign.type", "1", one_user_uuid) }.to raise_error(CanvasOutcomesHelper::OSFetchError)
+          end
+
           it "raises error on non 2xx response" do
             stub_get_lmgb_results("associated_asset_id_list=1&associated_asset_type=assign.type&external_outcome_id_list=1&user_uuid_list=#{one_user_uuid}").to_return(status: 401, body: '{"valid_jwt":false}')
-            expect { subject.get_lmgb_results(@course, "1", "assign.type", "1", one_user_uuid) }.to raise_error(RuntimeError, /Error retrieving results from Outcomes Service:/)
+            expect { subject.get_lmgb_results(@course, "1", "assign.type", "1", one_user_uuid) }.to raise_error(CanvasOutcomesHelper::OSFetchError, /Error retrieving results from Outcomes Service:/)
           end
 
           it "returns results with one assignment id" do