From 91d15bac41c53098a0a6faafeb0510bbdb45e2d7 Mon Sep 17 00:00:00 2001 From: Chrystal Langston Date: Tue, 24 Jan 2023 17:14:29 -0500 Subject: [PATCH] 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 Reviewed-by: Dave Wenzlick QA-Review: Dave Wenzlick Product-Review: Kyle Rosenbaum Tested-by: Service Cloud Jenkins --- app/controllers/outcome_results_controller.rb | 95 ++++++-- app/helpers/outcome_result_resolver_helper.rb | 4 +- lib/outcomes/result_analytics.rb | 16 +- spec/apis/v1/outcome_results_api_spec.rb | 21 +- .../outcome_results_controller_spec.rb | 229 ++++++++++++++++-- .../outcome_result_resolver_helper_spec.rb | 16 +- spec/lib/outcomes/result_analytics_spec.rb | 2 +- 7 files changed, 308 insertions(+), 75 deletions(-) diff --git a/app/controllers/outcome_results_controller.rb b/app/controllers/outcome_results_controller.rb index 9414d9f3432..fe442d55fbc 100644 --- a/app/controllers/outcome_results_controller.rb +++ b/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])] diff --git a/app/helpers/outcome_result_resolver_helper.rb b/app/helpers/outcome_result_resolver_helper.rb index c2ec6d6e66f..9cfe4ed86c6 100644 --- a/app/helpers/outcome_result_resolver_helper.rb +++ b/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 diff --git a/lib/outcomes/result_analytics.rb b/lib/outcomes/result_analytics.rb index 46f6b5c12e2..6c20cc6b2ff 100644 --- a/lib/outcomes/result_analytics.rb +++ b/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 diff --git a/spec/apis/v1/outcome_results_api_spec.rb b/spec/apis/v1/outcome_results_api_spec.rb index e66050befec..d0f86e0b221 100644 --- a/spec/apis/v1/outcome_results_api_spec.rb +++ b/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", diff --git a/spec/controllers/outcome_results_controller_spec.rb b/spec/controllers/outcome_results_controller_spec.rb index a7d4d050d74..bcfa9d06af2 100644 --- a/spec/controllers/outcome_results_controller_spec.rb +++ b/spec/controllers/outcome_results_controller_spec.rb @@ -18,6 +18,8 @@ # with this program. If not, see . # +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 diff --git a/spec/helpers/outcome_result_resolver_helper_spec.rb b/spec/helpers/outcome_result_resolver_helper_spec.rb index f651b247fb6..f5466ea6aed 100644 --- a/spec/helpers/outcome_result_resolver_helper_spec.rb +++ b/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 diff --git a/spec/lib/outcomes/result_analytics_spec.rb b/spec/lib/outcomes/result_analytics_spec.rb index e28cbe05cd0..2905176006e 100644 --- a/spec/lib/outcomes/result_analytics_spec.rb +++ b/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