Add Rails caching to rollups api

This PS also corrects rubric filtering for the current course

closes OUT-5430
flag=outcome_service_results_to_canvas

test plan:
- Jenkins Passes

Change-Id: I3458eaefeff0832a01318e3446760c651f572375
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/309489
Reviewed-by: Martin Yosifov <martin.yosifov@instructure.com>
Reviewed-by: Dave Wenzlick <david.wenzlick@instructure.com>
QA-Review: Dave Wenzlick <david.wenzlick@instructure.com>
Product-Review: Kyle Rosenbaum <krosenbaum@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>

app/controllers/outcome_results_controller.rb

@@ -187,6 +187,7 @@
 #     }
 
 class OutcomeResultsController < ApplicationController
+  CACHE_EXPIRATION = 60.seconds
   include Api::V1::OutcomeResults
   include Outcomes::Enrollments
   include Outcomes::ResultAnalytics
@@ -235,12 +236,8 @@ class OutcomeResultsController < ApplicationController
   # used in sLMGB
   def index
     include_hidden_value = value_to_boolean(params[:include_hidden])
-    @results = find_results(
-      include_hidden: include_hidden_value
-    )
-    @outcome_service_results = find_outcomes_service_results(
-      include_hidden: include_hidden_value
-    )
+    @results, @outcome_service_results = find_canvas_os_results(include_hidden: include_hidden_value)
+
     json = nil
     if @outcome_service_results.nil?
       @results = Api.paginate(@results, self, api_v1_course_outcome_results_url)
@@ -359,34 +356,83 @@ class OutcomeResultsController < ApplicationController
 
   private
 
-  def find_results(opts = {})
-    find_outcome_results(
-      @current_user,
-      users: opts[:all_users] ? @all_users : @users,
-      context: @context,
-      outcomes: @outcomes,
-      **opts
-    )
+  def fetch_os_results(opts)
+    Rails.cache.fetch(generate_cache_results_key(opts), expires_in: CACHE_EXPIRATION) do
+      results = find_outcomes_service_outcome_results(
+        @current_user,
+        users: opts[:all_users] ? @all_users : @users,
+        context: @context,
+        outcomes: @outcomes,
+        **opts
+      )
+      # storing only the attributes that are required for (s)lmgb in the cache
+      results&.map { |r| r.slice(:external_outcome_id, :associated_asset_id, :user_uuid, :points, :points_possible, :submitted_at) }
+    end
   end
 
-  def find_outcomes_service_results(opts = {})
-    find_outcomes_service_outcome_results(
+  def fetch_and_convert_os_results(opts)
+    os_results_json = fetch_os_results(opts)
+    return if os_results_json.nil?
+
+    # converts json to LearningOutcomeResult objects and removes duplicate rubric results, if found.
+    handle_outcome_service_results(os_results_json, @context)
+  end
+
+  def find_canvas_os_results(opts = { all_users: false })
+    canvas_results = find_outcome_results(
       @current_user,
       users: opts[:all_users] ? @all_users : @users,
       context: @context,
       outcomes: @outcomes,
       **opts
     )
+
+    os_results = fetch_and_convert_os_results(opts)
+
+    [canvas_results, os_results]
+  end
+
+  # there are 2 different types of opt params in LMGB & sLMGB: {:all_users: false} or {:all_users: true}
+  # {} is the same exact option as {:all_users :false}.  Given this, there will be two keys created for
+  # OS that will either contain {:all_users: false} or {:all_users: true}.  Example:
+  # lmgb_{all_users=>true}, lmgb_{all_users=>false}
+  # In addition to these cache keys there are two other parameters that could be concatenated that are
+  # specific when viewing sLMGB and course section LMGB
+  # For sLMGB ... user_ids plus a delimited list of students' user ids will be present in the cache key in the form of:
+  # slmgb_user_ids_1|2|3_{all_users=>true}
+  # For course section LMGB ... student_id plus the section id will be present in the cache key in the form of:
+  # lmgb_section_id_123_{all_users=>true}
+  # FURTHERMORE... to ensure the cache key is unique, the key also includes the @current_user.uuid, @context.uuid, & @domain_root_account.uuid
+  # example of cache key:
+  # slmgb_user_ids_5319_{:include_hidden=>false}/context_uuid/xDlV3Ca2nBRtRHX2K0ie0Wxng6grJKzEXSuIGoey/
+  #    current_user_uuid/dPu5lwmdwEJxBUqfiNlzyod3jbvVtdD0u8GrnVje/account_uuid/SYMqtl31AbcfmV6WfKkO5gqwpNr7Mvx21RHgG1bc
+  def generate_cache_results_key(opts)
+    # lmgb overall course
+    results_type = "lmgb"
+    # if section_id params is present then it is a course section and should be cached with section_id param
+    results_type = "lmgb_section_id_#{params[:section_id]}" unless params[:section_id].nil?
+    # slmgb is identified with the user_ids parameter and should be cached with user_ids params
+    results_type = "slmgb_user_ids_#{params[:user_ids].join("|")}" unless params[:user_ids].nil?
+
+    cache_key =  "#{results_type}_#{opts}"
+    # Adding the currently logged in course, currently logged in user, and domain_root_account for session uniqueness
+    # looking around at other Rails.cache implementations, context and/or current_user and/or domain_root_account
+    # are used for uniqueness. We will use all 3's uuid for tripley safe measures.
+    # Refer to the below controllers for examples of cache keys
+    #   app/controllers/quizzes/quizzes_controller.rb
+    #   app/controllers/quizzes_next/quizzes_api_controller.rb
+    #   app/controllers/application_controller.rb
+    [cache_key, "context_uuid", @context.uuid, "current_user_uuid", @current_user.uuid, "account_uuid", @domain_root_account.uuid]
   end
 
   # used in sLMGB/LMGB
-  def user_rollups(opts = {})
+  def user_rollups(opts = { all_users: false })
     excludes = Api.value_to_array(params[:exclude]).uniq
     filter_users_by_excludes
 
-    @results = find_results(opts).preload(:user)
+    @results, @outcome_service_results = find_canvas_os_results(opts)
+    @results = @results.preload(:user)
     ActiveRecord::Associations.preload(@results, :learning_outcome)
-    @outcome_service_results = find_outcomes_service_results(opts)
     if @outcome_service_results.nil?
       outcome_results_rollups(results: @results, users: @users, excludes: excludes, context: @context)
     else
@@ -434,8 +480,8 @@ class OutcomeResultsController < ApplicationController
   # Flagging potential issue - no reason to pull all the results for finding users
   # why not send the already pulled results to the definition and use that to filter
   def remove_users_with_no_results
-    userids_with_results = find_results.pluck(:user_id).uniq
-    os_userids_with_results = find_outcomes_service_results
+    userids_with_results, os_userids_with_results = find_canvas_os_results
+    userids_with_results = userids_with_results.pluck(:user_id).uniq
 
     if os_userids_with_results.nil?
       @users = @users.select { |u| userids_with_results.include? u.id }
@@ -510,10 +556,11 @@ class OutcomeResultsController < ApplicationController
     # calculating averages for all users in the context and only returning one
     # rollup, so don't paginate users in this method.
     filter_users_by_excludes(true)
-    @results = find_results(all_users: false).preload(:user)
-    ActiveRecord::Associations.preload(@results, :learning_outcome)
 
-    @outcome_service_results = find_outcomes_service_results(all_users: false)
+    @results, @outcome_service_results = find_canvas_os_results(all_users: false)
+    @results = @results.preload(:user)
+
+    ActiveRecord::Associations.preload(@results, :learning_outcome)
     aggregate_rollups = nil
     if @outcome_service_results.nil?
       aggregate_rollups = [aggregate_outcome_results_rollup(@results, @context, params[:aggregate_stat])]

app/helpers/outcome_result_resolver_helper.rb

@@ -20,9 +20,9 @@
 module OutcomeResultResolverHelper
   include OutcomesServiceAuthoritativeResultsHelper
 
-  def resolve_outcome_results(authoritative_results)
+  def resolve_outcome_results(authoritative_results, context)
     results = convert_to_learning_outcome_results(authoritative_results)
-    rubric_results = LearningOutcomeResult.preload(:learning_outcome).active.where(association_type: "RubricAssociation")
+    rubric_results = LearningOutcomeResult.preload(:learning_outcome).active.where(context: context, association_type: "RubricAssociation")
     results.reject { |res| rubric_result?(res, rubric_results) }
   end
 

lib/outcomes/result_analytics.rb

@@ -72,7 +72,7 @@ module Outcomes
     #        :context  - The context to lookup results for (required)
     #        :outcomes - The outcomes to lookup results for (required)
     #
-    # Returns a relation of the results
+    # Returns json object
     def find_outcomes_service_outcome_results(user, opts)
       required_opts = %i[users context outcomes]
       required_opts.each { |p| raise "#{p} option is required" unless opts[p] }
@@ -93,12 +93,16 @@ module Outcomes
         end
 
       outcome_ids = outcomes.pluck(:id).join(",")
-      handle_outcome_service_results(
-        get_lmgb_results(context, assignment_ids, "canvas.assignment.quizzes", outcome_ids, user_uuids),
-        context
-      )
+      get_lmgb_results(context, assignment_ids, "canvas.assignment.quizzes", outcome_ids, user_uuids)
     end
 
+    # Converts json results from OS API to LearningOutcomeResults and removes duplicate result data
+    # Tech debt: decouple conversion and removing duplicates
+    #
+    # results - OS api results json (see get_lmgb_results)
+    # context - results context (aka current course)
+    #
+    # Returns an array of LearningOutcomeResult objects
     def handle_outcome_service_results(results, context)
       # if results are nil - FF is turned off for the given context
       # if results are empty - no results were matched
@@ -108,7 +112,7 @@ module Outcomes
       end
       # if results are not nil or empty (aka not blank) - results were found
       # return resolved results list of Rollup objects
-      resolve_outcome_results(results)
+      resolve_outcome_results(results, context)
     end
 
     # Internal: Add an order clause to a relation so results are returned in an

spec/apis/v1/outcome_results_api_spec.rb

@@ -498,11 +498,12 @@ describe "Outcome Results API", type: :request do
 
       describe "user_ids parameter" do
         it "restricts results to specified users" do
+          # api endpoint requires an array of strings
           student_ids = outcome_students[0..1].map(&:id).map(&:to_s)
-          student_id_str = student_ids.join(",")
           @user = @teacher
-          api_call(:get, outcome_rollups_url(outcome_course, user_ids: student_id_str, include: ["users"]),
-                   controller: "outcome_results", action: "rollups", format: "json", course_id: outcome_course.id.to_s, user_ids: student_id_str, include: ["users"])
+
+          api_call(:get, outcome_rollups_url(outcome_course, user_ids: student_ids, include: ["users"]),
+                   controller: "outcome_results", action: "rollups", format: "json", course_id: outcome_course.id.to_s, user_ids: student_ids, include: ["users"])
           json = JSON.parse(response.body)
           expect(json.keys.sort).to eq %w[linked meta rollups]
           expect(json["rollups"].size).to eq 2
@@ -531,9 +532,10 @@ describe "Outcome Results API", type: :request do
           pseudonym.user_id = @student.id
           pseudonym.sis_user_id = "123"
           pseudonym.save
-          api_call(:get, outcome_results_url(outcome_course, user_ids: "sis_user_id:123", include: ["users"]),
+          # rollups api requires user_ids to be an array
+          api_call(:get, outcome_results_url(outcome_course, user_ids: ["sis_user_id:123"], include: ["users"]),
                    controller: "outcome_results", action: "index", format: "json", course_id: outcome_course.id.to_s,
-                   user_ids: "sis_user_id:123", include: ["users"])
+                   user_ids: ["sis_user_id:123"], include: ["users"])
           json = JSON.parse(response.body)
           expect(json["linked"]["users"][0]["id"].to_i).to eq @student.id
         end
@@ -546,11 +548,11 @@ describe "Outcome Results API", type: :request do
           pseudonym.sis_user_id = "123"
           pseudonym.save
           student_ids << "sis_user_id:123"
-          student_id_str = student_ids.join(",")
           @user = @teacher
-          api_call(:get, outcome_rollups_url(outcome_course, user_ids: student_id_str, include: ["users"]),
+          #  rollups api requires that student_ids is an array
+          api_call(:get, outcome_rollups_url(outcome_course, user_ids: student_ids, include: ["users"]),
                    controller: "outcome_results", action: "rollups", format: "json", course_id: outcome_course.id.to_s,
-                   user_ids: student_id_str, include: ["users"])
+                   user_ids: student_ids, include: ["users"])
           json = JSON.parse(response.body)
           expect(json["linked"]["users"].size).to eq 3
           expect(json["linked"]["users"].map { |h| h["id"].to_i }.max).to eq sis_id_student.id
@@ -750,7 +752,8 @@ describe "Outcome Results API", type: :request do
       describe "user_ids parameter" do
         it "restricts aggregate to specified users" do
           outcome_students
-          student_id_str = outcome_students[0..1].map(&:id).join(",")
+          #  rollups api requires that user_ids is an array
+          student_id_str = outcome_students[0..1].map(&:id).map(&:to_s)
           @user = @teacher
           api_call(:get, outcome_rollups_url(outcome_course, aggregate: "course", user_ids: student_id_str),
                    controller: "outcome_results", action: "rollups", format: "json",

spec/controllers/outcome_results_controller_spec.rb

@@ -18,6 +18,8 @@
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 #
 
+require_relative "../apis/api_spec_helper"
+
 describe OutcomeResultsController do
   def context_outcome(context)
     @outcome_group = context.root_outcome_group
@@ -158,6 +160,25 @@ describe OutcomeResultsController do
     JSON.parse(response.body)
   end
 
+  def mock_os_api_results(user_uuid, outcome_id, associated_asset_id, score, points, points_possible, submitted_at)
+    {
+      user_uuid: user_uuid,
+      percent_score: score, points: points, points_possible: points_possible,
+      external_outcome_id: outcome_id, submitted_at: submitted_at,
+      attempts: [{ id: 1, authoritative_result_id: 1, points: points,
+                   points_possible: points_possible, event_created_at: Time.zone.now,
+                   event_updated_at: Time.zone.now, deleted_at: nil,
+                   created_at: Time.zone.now, updated_at: Time.zone.now,
+                   metadata: { quiz_metadata: { title: "Quiz Title", points: points, quiz_id: "1",
+                                                points_possible: points_possible } },
+                   submitted_at: submitted_at,
+                   attempt_number: 1 }],
+      associated_asset_type: "canvas.assignment.quizzes",
+      associated_asset_id: associated_asset_id, artifact_type: "quizzes.quiz", artifact_id: "1",
+      mastery: nil
+    }
+  end
+
   def mock_os_lor_results(user, outcome, assignment, score, args = {})
     title = "#{user.name}, #{assignment.name}"
     mastery = (score || 0) >= outcome.mastery_points
@@ -405,7 +426,7 @@ describe OutcomeResultsController do
           user_session(user)
           @course.disable_feature!(:outcome_service_results_to_canvas)
           create_result(student.id, @outcome, @assignment, 2, { possible: 5 })
-          expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(nil)
+          expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(nil)
           json = parse_response(get_results({ user_ids: [student], include: ["assignments"] }))
           expect(json["outcome_results"].length).to be 1
           expect(json["linked"]["assignments"].length).to be 1
@@ -419,7 +440,7 @@ describe OutcomeResultsController do
 
           it "no OS results found - display canvas results only" do
             create_result(student.id, @outcome, @assignment, 2, { possible: 5 })
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(nil)
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(nil)
             json = parse_response(get_results({ user_ids: [student], include: ["assignments"] }))
             expect(json["outcome_results"].length).to be 1
             expect(json["linked"]["assignments"].length).to be 1
@@ -427,7 +448,7 @@ describe OutcomeResultsController do
 
           it "OS results found - no Canvas results - displays only OS results" do
             mocked_results = mock_os_lor_results(student, @outcome, @assignment2, 2)
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
               [mocked_results]
             )
             json = parse_response(get_results({ user_ids: [student], include: ["assignments"] }))
@@ -440,7 +461,7 @@ describe OutcomeResultsController do
           it "OS results found - display both Canvas and OS results" do
             create_result(student.id, @outcome, @assignment, 2, { possible: 5 })
             mocked_results = mock_os_lor_results(student, @outcome, @assignment2, 2)
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
               [mocked_results]
             )
             json = parse_response(get_results({ user_ids: [student], include: ["assignments"] }))
@@ -455,7 +476,7 @@ describe OutcomeResultsController do
             create_result(student.id, @outcome, @assignment, 2, { possible: 5 })
             mocked_results_1 = mock_os_lor_results(student, @outcome, @assignment2, 2)
             mocked_results_2 = mock_os_lor_results(student, outcome2, @assignment2, 2)
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
               [mocked_results_1, mocked_results_2]
             )
             json = parse_response(get_results({ user_ids: [student], include: ["assignments"] }))
@@ -502,6 +523,10 @@ describe OutcomeResultsController do
                      format: "json"
     end
 
+    def outcome_rollups_url(context, params = {})
+      api_v1_course_outcome_rollups_url(context, params)
+    end
+
     it "includes rating percents" do
       json = parse_response(get_rollups(rating_percents: true, include: ["outcomes"]))
       expect(json["linked"]["outcomes"][0]["ratings"].map { |r| r["percent"] }).to eq [50, 50]
@@ -509,12 +534,138 @@ describe OutcomeResultsController do
 
     context "with outcome_service_results_to_canvas FF" do
       context "user_rollups" do
-        it "disabled - only display rollups for canvas" do
-          @course.disable_feature!(:outcome_service_results_to_canvas)
-          create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
-          expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(nil)
-          json = parse_response(get_rollups(sort_by: "student", sort_order: "desc", per_page: 1, page: 1))
-          expect(json["rollups"].length).to be 1
+        context "disabled FF" do
+          before do
+            @course.disable_feature!(:outcome_service_results_to_canvas)
+          end
+
+          it "only display rollups for canvas" do
+            create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(nil)
+            json = parse_response(get_rollups(sort_by: "student", sort_order: "desc", per_page: 1, page: 1))
+            expect(json["rollups"].length).to be 1
+          end
+
+          context "caching - calling os/canvas api/db for outcome results only once and pulling subsequent from rails cache store" do
+            before do
+              controller.instance_variable_set(:@domain_root_account, @account)
+            end
+
+            it "caches lgmb rollup request per opts, course, user, and user shard id" do
+              # creating a student result in @course aka outcome_course
+              create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
+              expect(controller).to receive(:find_outcomes_service_outcome_results).with(any_args).once.and_return(nil)
+
+              enable_cache do
+                user_session @teacher
+                teacher_json = parse_response(get_rollups(include: %w[outcomes users outcome_paths],
+                                                          exclude: %w[concluded_enrollments inactive_enrollments missing_user_rollups],
+                                                          sort_by: "student",
+                                                          sort_order: "desc",
+                                                          per_page: 1,
+                                                          page: 1))
+                # validating the data returned is correct.  Should have 1 rollup.
+                expect(teacher_json["rollups"].length).to be 1
+                # should have one key in the cache for OS
+                expect(Rails.cache.exist?(["lmgb_{:all_users=>false}", "context_uuid", @course.uuid, "current_user_uuid", @teacher.uuid, "account_uuid", @account.uuid])).to be_truthy
+              end
+            end
+
+            it "caches lmgb rollup requests separately per user session enrolled in the same course" do
+              # creating a student result in @course aka outcome_course
+              create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
+              expect(controller).to receive(:find_outcomes_service_outcome_results).with(any_args).twice.and_return(nil)
+
+              enable_cache do
+                user_session @teacher
+                teacher1_json = parse_response(get_rollups(include: %w[outcomes users outcome_paths],
+                                                           exclude: %w[concluded_enrollments inactive_enrollments missing_user_rollups],
+                                                           sort_by: "student",
+                                                           sort_order: "desc",
+                                                           per_page: 1,
+                                                           page: 1))
+                expect(Rails.cache.exist?(["lmgb_{:all_users=>false}", "context_uuid", @course.uuid, "current_user_uuid", @teacher.uuid, "account_uuid", @account.uuid])).to be_truthy
+                expect(Rails.cache.instance_variable_get(:@data).keys.grep(/lmgb/i).count).to eq 1
+
+                # creating and enrolling teacher 2 in @course aka outcomes_course
+                teacher2 = teacher_in_course(course: @course, active_all: true).user
+                teacher2.save!
+                user_session teacher2
+                teacher2_json = parse_response(get_rollups(include: %w[outcomes users outcome_paths],
+                                                           exclude: %w[concluded_enrollments inactive_enrollments missing_user_rollups],
+                                                           sort_by: "student",
+                                                           sort_order: "desc",
+                                                           per_page: 1,
+                                                           page: 1))
+                expect(Rails.cache.exist?(["lmgb_{:all_users=>false}", "context_uuid", @course.uuid, "current_user_uuid", teacher2.uuid, "account_uuid", @account.uuid])).to be_truthy
+
+                # validating the rollups returned is the same for Teacher 1 and Teacher 2
+                expect(teacher2_json["rollups"]).to eq teacher1_json["rollups"]
+                # validating that there are 2 keys for lmgb
+                expect(Rails.cache.instance_variable_get(:@data).keys.grep(/lmgb_/i).count).to eq 2
+              end
+            end
+
+            it "manually created course and user to test caching with different course than outcome_course" do
+              manually_created_course = Course.create!(name: "Advanced Strength & Speed", account: @account)
+              super_teacher = User.create(name: "Black Panther")
+              super_teacher.pseudonyms.create(unique_id: "black@panther.com")
+              expect(controller).to receive(:find_outcomes_service_outcome_results).with(any_args).once.and_return(nil)
+
+              @course = manually_created_course
+              @user = super_teacher
+              @course.enroll_teacher(@user, enrollment_state: :active)
+
+              enable_cache do
+                user_session @user
+
+                super_teacher_json = parse_response(get_rollups(include: %w[outcomes users outcome_paths],
+                                                                exclude: %w[concluded_enrollments inactive_enrollments missing_user_rollups],
+                                                                sort_by: "student",
+                                                                sort_order: "desc",
+                                                                per_page: 1,
+                                                                page: 1))
+                # validating the data returned is correct. Should not have any rollups
+                expect(super_teacher_json["rollups"].length).to be 0
+                # should have one key in the cache for OS
+                expect(Rails.cache.exist?(["lmgb_{:all_users=>false}", "context_uuid", manually_created_course.uuid, "current_user_uuid", super_teacher.uuid, "account_uuid", @account.uuid])).to be_truthy
+                expect(Rails.cache.instance_variable_get(:@data).keys.grep(/lmgb_/i).count).to eq 1
+              end
+            end
+
+            it "caches slgmb rollup request per opts, user_ids param, course, user, and user shard id" do
+              create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
+              expect(controller).to receive(:find_outcomes_service_outcome_results).with(any_args).once.and_return(nil)
+
+              enable_cache do
+                user_session @teacher
+                get_rollups({ user_ids: [@student.id], per_page: 100 })
+                user_id_cache_key = "slmgb_user_ids_#{@student.id}"
+                expect(Rails.cache.exist?(["#{user_id_cache_key}_{:all_users=>false}", "context_uuid", @course.uuid, "current_user_uuid", @teacher.uuid, "account_uuid", @account.uuid])).to be_truthy
+                expect(Rails.cache.instance_variable_get(:@data).keys.grep(/#{user_id_cache_key}/i).count).to eq 1
+              end
+            end
+
+            it "caches lmgb section rollup request per opts, section_id param, course, user, and user shard id" do
+              section1 = add_section "s1", course: @course
+              student_in_section section1, user: @student2, allow_multiple_enrollments: true
+              create_result(@student2.id, @outcome, outcome_assignment, 2, { possible: 5 })
+              expect(controller).to receive(:find_outcomes_service_outcome_results).with(any_args).once.and_return(nil)
+
+              enable_cache do
+                user_session @teacher
+                get_rollups({ include: %w[outcomes users outcome_paths],
+                              section_id: section1.id,
+                              sort_by: "student",
+                              sort_order: "desc",
+                              per_page: 1,
+                              page: 1 })
+                section_id_cache_key = "lmgb_section_id_#{section1.id}"
+                expect(Rails.cache.exist?(["#{section_id_cache_key}_{:all_users=>false}", "context_uuid", @course.uuid, "current_user_uuid", @teacher.uuid, "account_uuid", @account.uuid])).to be_truthy
+                expect(Rails.cache.instance_variable_get(:@data).keys.grep(/#{section_id_cache_key}/i).count).to eq 1
+              end
+            end
+          end
         end
 
         context "enabled" do
@@ -524,11 +675,39 @@ describe OutcomeResultsController do
 
           it "no OS results found - display canvas rollups only" do
             create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(nil)
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(nil)
             json = parse_response(get_rollups(sort_by: "student", sort_order: "desc", per_page: 1, page: 1))
             expect(json["rollups"].length).to be 1
           end
 
+          it "OS results found - stores only the minimum parameters needed in cache" do
+            # removing LearningOutcomeResults for those users that have results
+            # creating in the first before do after the rollups context
+            LearningOutcomeResult.where(user_id: @student.id).update(workflow_state: "deleted")
+            LearningOutcomeResult.where(user_id: @student1.id).update(workflow_state: "deleted")
+            LearningOutcomeResult.where(user_id: @student2.id).update(workflow_state: "deleted")
+            student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
+            submitted_at = Time.zone.now
+            mocked_results = mock_os_api_results(student4.uuid, @outcome.id, outcome_assignment.id, "2.0", "2.0", "2.0", submitted_at)
+            expect(controller).to receive(:find_outcomes_service_outcome_results).with(any_args).and_return(
+              [mocked_results]
+            )
+
+            enable_cache do
+              user_session @teacher
+              parse_response(get_rollups(sort_by: "student", sort_order: "desc", per_page: 5, page: 1))
+              # should have one key in the cache for OS
+              expect(Rails.cache.exist?(["lmgb_{:all_users=>false}", "context_uuid", @course.uuid, "current_user_uuid", @teacher.uuid, "account_uuid", @account.uuid])).to be_truthy
+              bare_min = Rails.cache.fetch(["lmgb_{:all_users=>false}", "context_uuid", @course.uuid, "current_user_uuid", @teacher.uuid, "account_uuid", @account.uuid])
+              expect(bare_min).to eq [{ associated_asset_id: outcome_assignment.id,
+                                        external_outcome_id: @outcome.id,
+                                        points: "2.0",
+                                        points_possible: "2.0",
+                                        submitted_at: submitted_at,
+                                        user_uuid: student4.uuid }]
+            end
+          end
+
           it "OS results found - no Canvas results - displays only OS rollups" do
             # removing LearningOutcomeResults for those users that have results
             # creating in the first before do after the rollups context
@@ -537,7 +716,7 @@ describe OutcomeResultsController do
             LearningOutcomeResult.where(user_id: @student2.id).update(workflow_state: "deleted")
             student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
             mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
               [mocked_results]
             )
             json = parse_response(get_rollups(sort_by: "student", sort_order: "desc", per_page: 5, page: 1))
@@ -559,7 +738,7 @@ describe OutcomeResultsController do
             # results are already created for @student2 in Canvas
             student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
             mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
               [mocked_results]
             )
             json = parse_response(get_rollups(sort_by: "student", sort_order: "desc", per_page: 5, page: 1))
@@ -573,7 +752,7 @@ describe OutcomeResultsController do
             # already existing results for @student1 & @student2
             # creating result for @student
             create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(nil)
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(nil)
             json = parse_response(get_rollups(aggregate: "course", aggregate_stat: "mean", per_page: 5, page: 1))
             expect(json["rollups"].length).to be 1
             expect(json["rollups"][0]["scores"][0]["count"]).to be 3
@@ -588,7 +767,7 @@ describe OutcomeResultsController do
               # already existing results for @student1 & @student2
               # creating result for @student
               create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(nil)
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(nil)
               json = parse_response(get_rollups(aggregate: "course", aggregate_stat: "mean", per_page: 5, page: 1))
               expect(json["rollups"].length).to be 1
               expect(json["rollups"][0]["scores"][0]["count"]).to be 3
@@ -602,7 +781,7 @@ describe OutcomeResultsController do
               LearningOutcomeResult.where(user_id: @student2.id).update(workflow_state: "deleted")
               student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
               mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
                 [mocked_results]
               )
               json = parse_response(get_rollups(aggregate: "course", aggregate_stat: "mean", per_page: 5, page: 1))
@@ -617,7 +796,7 @@ describe OutcomeResultsController do
               # results are already created for @student2 in Canvas
               student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
               mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
                 [mocked_results]
               )
               json = parse_response(get_rollups(aggregate: "course", aggregate_stat: "mean", per_page: 5, page: 1))
@@ -633,7 +812,7 @@ describe OutcomeResultsController do
             # already existing results for @student1 & @student2
             # creating result for @student
             create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
-            expect(controller).to receive(:find_outcomes_service_results).with(any_args).twice.and_return(nil)
+            expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).twice.and_return(nil)
             json = parse_response(get_rollups(sort_by: "student", sort_order: "desc",
                                               exclude: ["missing_user_rollups"],
                                               per_page: 5, page: 1))
@@ -649,7 +828,7 @@ describe OutcomeResultsController do
               # already existing results for @student1 & @student2
               # creating result for @student
               create_result(@student.id, @outcome, outcome_assignment, 2, { possible: 5 })
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).twice.and_return(nil)
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).twice.and_return(nil)
               json = parse_response(get_rollups(sort_by: "student", sort_order: "desc",
                                                 exclude: ["missing_user_rollups"],
                                                 per_page: 5, page: 1))
@@ -664,7 +843,7 @@ describe OutcomeResultsController do
               LearningOutcomeResult.where(user_id: @student2.id).update(workflow_state: "deleted")
               student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
               mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).twice.and_return(
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).twice.and_return(
                 [mocked_results]
               )
               # per_page is the number of students to display on 1 page of results
@@ -681,7 +860,7 @@ describe OutcomeResultsController do
               # results are already created for @student2 in Canvas
               student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
               mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).twice.and_return(
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).twice.and_return(
                 [mocked_results]
               )
               # per_page is the number of students to display on 1 page of results
@@ -701,7 +880,7 @@ describe OutcomeResultsController do
               # and will not create results for this student
               student_in_course(active_all: true, course: outcome_course)
               mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).twice.and_return(
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).twice.and_return(
                 [mocked_results]
               )
               # per_page is the number of students to display on 1 page of results
@@ -719,7 +898,7 @@ describe OutcomeResultsController do
               # creating and enrolling student 4 in the course
               student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
               mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
                 [mocked_results]
               )
               # concluding student 3 in the course which will remove the student from the results
@@ -737,7 +916,7 @@ describe OutcomeResultsController do
               # creating and enrolling student 4 in the course
               student4 = student_in_course(active_all: true, course: outcome_course, name: "OS user").user
               mocked_results = mock_os_lor_results(student4, @outcome, outcome_assignment, 2)
-              expect(controller).to receive(:find_outcomes_service_results).with(any_args).and_return(
+              expect(controller).to receive(:fetch_and_convert_os_results).with(any_args).and_return(
                 [mocked_results]
               )
               # deactivating student 3 in the course which will remove the student from the results

spec/helpers/outcome_result_resolver_helper_spec.rb

@@ -48,7 +48,7 @@ describe OutcomeResultResolverHelper do
       create_alignment_with_rubric({ assignment: @assignment })
       create_learning_outcome_result_from_rubric @students[0], 1.0
 
-      expect(resolve_outcome_results(authoritative_results_from_db).size).to eq 0
+      expect(resolve_outcome_results(authoritative_results_from_db, @course).size).to eq 0
     end
 
     it "if a rubric result exists for multiple students for the same assignment and outcome" do
@@ -67,7 +67,7 @@ describe OutcomeResultResolverHelper do
       create_learning_outcome_result_from_rubric @students[0], 1.0
       create_learning_outcome_result_from_rubric @students[1], 1.0
 
-      expect(resolve_outcome_results(authoritative_results_from_db).size).to eq 0
+      expect(resolve_outcome_results(authoritative_results_from_db, @course).size).to eq 0
     end
 
     it "for just one student if a rubric result exists for their assignment and outcome" do
@@ -83,7 +83,7 @@ describe OutcomeResultResolverHelper do
       create_alignment_with_rubric({ assignment: @assignment })
       create_learning_outcome_result_from_rubric @students[0], 1.0
 
-      expect(resolve_outcome_results(authoritative_results_from_db).size).to eq 1
+      expect(resolve_outcome_results(authoritative_results_from_db, @course).size).to eq 1
     end
 
     it "for an assignment with multiple outcomes aligned, where one outcome has a rubric result for a student" do
@@ -102,7 +102,7 @@ describe OutcomeResultResolverHelper do
       create_alignment_with_rubric({ assignment: @assignment })
       create_learning_outcome_result_from_rubric @students[0], 1.0
 
-      expect(resolve_outcome_results(authoritative_results_from_db).size).to eq 1
+      expect(resolve_outcome_results(authoritative_results_from_db, @course).size).to eq 1
     end
   end
 
@@ -112,7 +112,7 @@ describe OutcomeResultResolverHelper do
       create_alignment
       create_learning_outcome_result @students[0], 1.0
 
-      expect(resolve_outcome_results(authoritative_results_from_db).size).to eq 1
+      expect(resolve_outcome_results(authoritative_results_from_db, @course).size).to eq 1
     end
 
     it "if there is no rubric result for that student" do
@@ -123,7 +123,7 @@ describe OutcomeResultResolverHelper do
       create_alignment_with_rubric({ assignment: @assignment })
       create_learning_outcome_result_from_rubric @students[1], 1.0
 
-      expect(resolve_outcome_results(authoritative_results_from_db).size).to eq 1
+      expect(resolve_outcome_results(authoritative_results_from_db, @course).size).to eq 1
     end
 
     it "if there is no rubric result for that assignment" do
@@ -134,7 +134,7 @@ describe OutcomeResultResolverHelper do
       create_alignment_with_rubric
       create_learning_outcome_result_from_rubric @students[2], 1.0
 
-      expect(resolve_outcome_results(authoritative_results_from_db).size).to eq 1
+      expect(resolve_outcome_results(authoritative_results_from_db, @course).size).to eq 1
     end
 
     it "if there is no rubric result for that outcome" do
@@ -146,7 +146,7 @@ describe OutcomeResultResolverHelper do
       create_alignment_with_rubric
       create_learning_outcome_result_from_rubric @students[0], 1.0
 
-      expect(resolve_outcome_results(authoritative_results_from_db).size).to eq 1
+      expect(resolve_outcome_results(authoritative_results_from_db, @course).size).to eq 1
     end
   end
 end

spec/lib/outcomes/result_analytics_spec.rb

@@ -119,7 +119,7 @@ describe Outcomes::ResultAnalytics do
             attempts: {}
           }
         ]
-        expect(ra).to receive(:resolve_outcome_results).with(os_results)
+        expect(ra).to receive(:resolve_outcome_results).with(os_results, @course)
         ra.send(:handle_outcome_service_results, os_results, @course)
       end
     end