ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

LTI previousContextIds.recursive: uniqify courses 

Fixes INTEROP-6389

On the problem course, limits from about 55k to ~2113 (it was 2161
before the code change in October to provide order), so should fix the
LTI launch problem for this course.  But we may want to actually put a
hard limit in here anyway.

On the problem course, this does not slow down the query (about 700ms)

Test plan:
- I tested this by copying and pasting both the original and the new
  function definitions (first making @context an agrument and removing the
  caching call) into a prod console and running it for the course
  mentioned in the bug report (89042), and making sure that the results of
  the new function were equal to the results of the old function made
  unique by ruby's `.uniq`. Not sure if there is a better way.

flag=none

Change-Id: I4772203a31eeeaa4027db5c96683b4ac1c6302be
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/255559
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Mysti Lilla <mysti@instructure.com>
Product-Review: Evan Battaglia <ebattaglia@instructure.com>
Reviewed-by: Mysti Lilla <mysti@instructure.com>
This commit is contained in:
Evan Battaglia 2020-12-17 18:29:20 -07:00
parent 0720d4e5d2
commit a1b29649e9
2 changed files with 69 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

83
lib/lti/substitutions_helper.rb
View File

@ -214,10 +214,6 @@ module Lti
previous_course_ids_and_context_ids.map(&:last).compact.join(',')
end
def recursively_fetch_previous_lti_context_ids
recursively_fetch_previous_course_ids_and_context_ids.map(&:last).compact.join(',')
end
def previous_course_ids
previous_course_ids_and_context_ids.map(&:first).sort.join(',')
end
@ -248,6 +244,48 @@ module Lti
e || @user.email
end
def recursively_fetch_previous_lti_context_ids
return '' unless @context.is_a?(Course)
# now find all parents for locked folders
last_migration_id = @context.content_migrations.where(workflow_state: :imported).order(id: :desc).limit(1).pluck(:id).first
return '' unless last_migration_id
# we can cache on the last migration because even if copies are done elsewhere they won't affect anything
# until a new copy is made to _this_ course
Rails.cache.fetch(["recursive_copied_course_lti_context_ids", @context.global_id, last_migration_id].cache_key) do
# Finds content migrations for this course and recursively, all content
# migrations for the source course of the migration -- that is, all
# content migrations that directly or indirectly provided content to
# this course. From there we get theunique list of courses, ordering by
# which has the migration with the latest timestamp.
results = Course.connection.select_rows(<<-SQL)
WITH RECURSIVE all_contexts AS (
SELECT context_id, source_course_id
FROM #{ContentMigration.quoted_table_name}
WHERE context_id=#{@context.id}
UNION
SELECT content_migrations.context_id, content_migrations.source_course_id
FROM #{ContentMigration.quoted_table_name}
INNER JOIN all_contexts t ON content_migrations.context_id = t.source_course_id
),
results AS (
SELECT DISTINCT ON (courses.lti_context_id) courses.id, ct.finished_at, courses.lti_context_id
FROM #{Course.quoted_table_name}
INNER JOIN #{ContentMigration.quoted_table_name} ct
ON ct.source_course_id = courses.id
AND ct.workflow_state = 'imported'
AND (ct.context_id IN (
SELECT x.context_id
FROM all_contexts x))
ORDER BY courses.lti_context_id, ct.finished_at DESC
)
SELECT lti_context_id FROM results ORDER BY finished_at DESC
SQL
results.map(&:last).compact.join(',')
end
end
private
def lti1?
@ -260,42 +298,5 @@ module Lti
"EXISTS (?)", ContentMigration.where(context_id: @context.id, workflow_state: :imported).where("content_migrations.source_course_id = courses.id")
).pluck(:id, :lti_context_id)
end
def recursively_fetch_previous_course_ids_and_context_ids
return [] unless @context.is_a?(Course)
# now find all parents for locked folders
last_migration_id = @context.content_migrations.where(workflow_state: :imported).order(id: :desc).limit(1).pluck(:id).first
return [] unless last_migration_id
# we can cache on the last migration because even if copies are done elsewhere they won't affect anything
# until a new copy is made to _this_ course
Rails.cache.fetch(["recursive_copied_course_lti_ids", @context.global_id, last_migration_id].cache_key) do
Course.connection.select_rows(<<-SQL)
WITH RECURSIVE all_contexts AS (
SELECT context_id, source_course_id
FROM #{ContentMigration.quoted_table_name}
WHERE context_id=#{@context.id}
UNION
SELECT content_migrations.context_id, content_migrations.source_course_id
FROM #{ContentMigration.quoted_table_name}
INNER JOIN all_contexts t ON content_migrations.context_id = t.source_course_id
),
interesting_contexts AS (
SELECT DISTINCT context_id
FROM all_contexts
)
SELECT courses.id, ct.finished_at, courses.lti_context_id
FROM #{Course.quoted_table_name}
INNER JOIN #{ContentMigration.quoted_table_name} ct
ON ct.source_course_id = courses.id
AND ct.workflow_state = 'imported'
AND (ct.context_id IN (
SELECT x.context_id
FROM interesting_contexts x))
ORDER BY ct.finished_at DESC
SQL
end
end
end
end

47
spec/lib/lti/substitutions_helper_spec.rb
View File

@ -483,13 +483,13 @@ module Lti
end
end
describe '#recursively_fetch_previous_course_ids_and_context_ids' do
describe '#recursively_fetch_previous_lti_context_ids' do
context 'without any course copies' do
before do
course.save!
end
it "should return empty to previous lti context_ids" do
it "should return empty (no previous lti context_ids)" do
expect(subject.recursively_fetch_previous_lti_context_ids).to be_empty
end
end
@ -503,12 +503,10 @@ module Lti
@c1.lti_context_id = 'abc'
@c1.save
course.content_migrations.create!.tap do |cm|
cm.context = course
cm.workflow_state = 'imported'
cm.source_course = @c1
cm.save!
end
course.content_migrations.create!(
workflow_state: 'imported',
source_course: @c1
)
@c2 = Course.create!
@c2.root_account = root_account
@ -516,12 +514,10 @@ module Lti
@c2.lti_context_id = 'def'
@c2.save!
course.content_migrations.create!.tap do |cm|
cm.context = course
cm.workflow_state = 'imported'
cm.source_course = @c2
cm.save!
end
course.content_migrations.create!(
workflow_state: 'imported',
source_course: @c2
)
@c3 = Course.create!
@c3.root_account = root_account
@ -529,12 +525,10 @@ module Lti
@c3.lti_context_id = 'ghi'
@c3.save!
@c1.content_migrations.create!.tap do |cm|
cm.context = @c1
cm.workflow_state = 'imported'
cm.source_course = @c3
cm.save!
end
@c1.content_migrations.create!(
workflow_state: 'imported',
source_course: @c3
)
end
it "should return previous lti context_ids" do
@ -553,6 +547,19 @@ module Lti
expect(subject.recursively_fetch_previous_lti_context_ids).to eq 'mno,jkl,ghi,def,abc'
end
end
context "when there are multiple content migrations pointing to the same source course" do
before do
@c3.content_migrations.create!(
workflow_state: 'imported',
source_course: @c2
)
end
it "should not return duplicates but still return in chronological order" do
expect(subject.recursively_fetch_previous_lti_context_ids).to eq 'def,ghi,abc'
end
end
end
describe "section substitutions" do