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

Persist nil resource link urls during course copy 

why

With resource_links now having urls, when looked up via retrieve, the
url launched is taken from the resource link's url column. If that
column is nil, then the tool's url will be used. Some resource links
rely on this behavior to launch the url. During course copy, the url in
the export is now included in the export and persisted in the import,
causing all imported resource links to never persist a nil url.

This commit updates the exporter with a new custom property in the cc
export, a resource_link_url, which includes the url column from the
exported resource_link. The importer will use this to populate the new
resource link's url column, and will use the standard cc launch_url
simply to lookup the tool to associate.

fixes INTEROP-7572

flag=none

test plan:
Find an existing course with a resource_link that has a nil url, and
copy that course. the new course should have the same resource_link,
except the url in the new resource link should still be nil.

Change-Id: I62f85689a4a6a7a2cbd8f95f82c6dde343d7b8e0
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/297340
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Xander Moffatt <xmoffatt@instructure.com>
QA-Review: Xander Moffatt <xmoffatt@instructure.com>
Migration-Review: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Paul Gray <paul.gray@instructure.com>
This commit is contained in:
Paul Gray 2022-07-28 17:43:49 -04:00
parent ff4d222182
commit bbe1da583a
7 changed files with 136 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/models/importers/lti_resource_link_importer.rb
View File

@ -49,7 +49,7 @@ module Importers
resource_link_for_course = Lti::ResourceLink.find_or_initialize_for_context_and_lookup_uuid(
context: migration.context,
lookup_uuid: lti_resource_link["lookup_uuid"],
url: lti_resource_link["launch_url"],
url: lti_resource_link["resource_link_url"],
context_external_tool_launch_url: lti_resource_link["launch_url"]
)
resource_link_for_course.custom =

27
db/migrate/20220728000001_fix_resource_links_from_copied_courses_null_urls.rb Normal file
View File

@ -0,0 +1,27 @@
# frozen_string_literal: true
#
# Copyright (C) 2018 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
#
class FixResourceLinksFromCopiedCoursesNullUrls < ActiveRecord::Migration[6.0]
tag :postdeploy
def up
DataFixup::FixResourceLinksFromCopiedCoursesNullUrls.delay_if_production(priority: Delayed::LOW_PRIORITY).run
end
end

5
lib/cc/importer/canvas/lti_resource_link_converter.rb
View File

@ -40,6 +40,7 @@ module CC::Importer::Canvas
custom = {}
lookup_uuid = nil
resource_link_url = nil
document.xpath("//blti:custom//lticm:property").each do |el|
key = el.attributes["name"].value
@ -66,6 +67,7 @@ module CC::Importer::Canvas
document.xpath("//blti:extensions//lticm:property").each do |el|
lookup_uuid = el.content if el.attributes["name"].value == "lookup_uuid"
resource_link_url = el.content if el.attributes["name"].value == "resource_link_url"
end
launch_url = (document.xpath("//blti:launch_url").first || document.xpath("//blti:secure_launch_url").first)&.content
@ -75,7 +77,8 @@ module CC::Importer::Canvas
resource_links << {
custom: custom,
launch_url: launch_url,
lookup_uuid: lookup_uuid
lookup_uuid: lookup_uuid,
resource_link_url: resource_link_url
}
end

18
lib/cc/lti_resource_links.rb
View File

@ -99,15 +99,12 @@ module CC
cartridge_basiclti_link.blti :title, tool.name
cartridge_basiclti_link.blti :description, tool.description
# Prefer the url on the resource_link
url = resource_link.url || tool.url
# URL element (choose secure or not based on protocol)
case url
case tool.url
when %r{^http://}
cartridge_basiclti_link.blti :launch_url, url
cartridge_basiclti_link.blti :launch_url, tool.url
when %r{^https://}
cartridge_basiclti_link.blti :secure_launch_url, url
cartridge_basiclti_link.blti :secure_launch_url, tool.url
end
# Custom parameters from the resource link
@ -124,6 +121,15 @@ module CC
resource_link.lookup_uuid,
name: "lookup_uuid"
)
unless resource_link.url.nil?
extensions.lticm(
:property,
# 'url' refers to the actual target_link_uri, whereas 'launch_url'
# is only used to look up the tool
resource_link.url,
name: "resource_link_url"
)
end
end
end
end

47
lib/data_fixup/fix_resource_links_from_copied_courses_null_urls.rb Normal file
View File

@ -0,0 +1,47 @@
# frozen_string_literal: true
#
# Copyright (C) 2012 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
module DataFixup::FixResourceLinksFromCopiedCoursesNullUrls
def self.resource_links_to_update
# '2022-06-06' is the day before the date of the original commit that added urls to resource links
ActiveRecord::Base.connection.execute(
"WITH original_resource_links AS (
SELECT lti_resource_links.id, lti_resource_links.root_account_id, lti_resource_links.lookup_uuid, lti_resource_links.url,
ROW_NUMBER() OVER(PARTITION BY lti_resource_links.lookup_uuid
ORDER BY lti_resource_links.created_at) AS rk
FROM #{Lti::ResourceLink.quoted_table_name}
)
SELECT copied_resource_links.id as copy_id, original_resource_links.id AS orig_id
FROM #{Lti::ResourceLink.quoted_table_name} AS copied_resource_links
INNER JOIN original_resource_links ON copied_resource_links.lookup_uuid = original_resource_links.lookup_uuid
AND copied_resource_links.root_account_id = original_resource_links.root_account_id
AND copied_resource_links.id <> original_resource_links.id
AND original_resource_links.url IS NULL
AND copied_resource_links.url IS NOT NULL
AND original_resource_links.rk = 1
WHERE copied_resource_links.created_at > '2022-07-26'
AND copied_resource_links.context_type = 'Course'"
)
end
def self.run
copied_ids = resource_links_to_update.map { |r| r["copy_id"] }
Lti::ResourceLink.where(id: copied_ids).in_batches.update_all(url: nil)
end
end

34
spec/lib/cc/lti_resource_links_spec.rb
View File

@ -26,6 +26,7 @@ describe CC::LtiResourceLinks do
let(:resource_link) do
Lti::ResourceLink.create!(
context: tool.context,
lookup_uuid: lookup_uuid,
context_external_tool: tool,
custom: custom,
url: resource_link_url
@ -42,6 +43,7 @@ describe CC::LtiResourceLinks do
)
end
let(:lookup_uuid) { "90cfe684-0f4f-11ed-861d-0242ac120002" }
let(:tool_url) { "https://www.test-tool.com/launch" }
let(:resource_link_url) { "https://www.test-tool.com/launch?foo=bar" }
let(:custom) { { foo: "bar", fiz: "buzz" } }
@ -77,7 +79,7 @@ describe CC::LtiResourceLinks do
end
it "sets the secure launch url" do
expect(subject.at_xpath("//blti:secure_launch_url").text).to eq resource_link_url
expect(subject.at_xpath("//blti:secure_launch_url").text).to eq tool_url
end
it "does not set the launch url" do
@ -92,11 +94,11 @@ describe CC::LtiResourceLinks do
).to eq(custom.stringify_keys)
end
context "when the resource link URL uses HTTP" do
let(:resource_link_url) { "http://www.test-tool.com/launch?foo=bar" }
context "when the tool URL uses HTTP" do
let(:tool_url) { "http://www.test-tool.com/launch?foo=bar" }
it "does set the launch url" do
expect(subject.at_xpath("//blti:launch_url").text).to eq resource_link.url
expect(subject.at_xpath("//blti:launch_url").text).to eq tool_url
end
it "does not set the secure launch url" do
@ -104,11 +106,31 @@ describe CC::LtiResourceLinks do
end
end
def find_extension(document, extension_name)
(document.xpath("//blti:extensions/lticm:property").map do |el|
if el.attribute("name").text == extension_name
el.text
end
end).compact.first
end
context "when the resource link URL is nil" do
let(:resource_link_url) { nil }
it "defaults to the tool url" do
expect(subject.at_xpath("//blti:secure_launch_url").text).to eq tool_url
it "does not include the resource_link_url property" do
expect(find_extension(subject, "resource_link_url")).to eq nil
end
end
context "when the resource link URL is populated" do
it "includes the resource_link_url extension property" do
expect(find_extension(subject, "resource_link_url")).to eq resource_link_url
end
end
context "when the lookup uuid is populated" do
it "includes the lookup_uuid extension property" do
expect(find_extension(subject, "lookup_uuid")).to eq lookup_uuid
end
end
end

21
spec/models/content_migration/course_copy_spec.rb
View File

@ -469,10 +469,19 @@ describe ContentMigration do
@copy_from.alt_name = "drama"
@copy_from.save!
tool = external_tool_1_3_model(context: @copy_from)
@copy_from.lti_resource_links.create!(
context_external_tool: external_tool_1_3_model(context: @copy_from),
context_external_tool: tool,
custom: nil,
lookup_uuid: "1b302c1e-c0a2-42dc-88b6-c029699a7c7a"
lookup_uuid: "1b302c1e-c0a2-42dc-88b6-c029699a7c7a",
url: "http://example.com/resource-link-url"
)
@copy_from.lti_resource_links.create!(
context_external_tool: tool,
custom: nil,
lookup_uuid: "1b302c1e-c0a2-42dc-88b6-c029699a7c7b",
url: nil
)
run_course_copy
@ -501,8 +510,12 @@ describe ContentMigration do
end
expect(@copy_to.tab_configuration).to eq @copy_from.tab_configuration
expect(@copy_to.lti_resource_links.size).to eq 1
expect(@copy_to.lti_resource_links.first.lookup_uuid).to eq "1b302c1e-c0a2-42dc-88b6-c029699a7c7a"
expect(@copy_to.lti_resource_links.size).to eq 2
rla = @copy_to.lti_resource_links.find { |rl| rl.lookup_uuid == "1b302c1e-c0a2-42dc-88b6-c029699a7c7a" }
expect(rla.url).to eq "http://example.com/resource-link-url"
rlb = @copy_to.lti_resource_links.find { |rl| rl.lookup_uuid == "1b302c1e-c0a2-42dc-88b6-c029699a7c7b" }
expect(rlb.url).to eq nil
end
context "with prevent_course_availability_editing_by_teachers on" do