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

part 3: change lookup_id and resource_link_id to UUID datatype 

This is part 3 of changing the datatype from varchar to UUID of
lookup_id and resource_link_id from lti_resource_links.

We start to read from the new columns created lookup_uuid and
resource_link_uuid.

Adding a migration to remove the not-null constraint of lookup_id and
resource_link_id columns. As part 3.1 we'll stop writing into these
columns, we need to execute this postdeploy migration at this point.

refs INTEROP-6488
flag=none

test-plan:
* specs should pass;
* you should check if LTI is launching as expected, and if the custom
  params was expanded as expected in all records that were created in the
  part 1 and 2;
* you should be able to new persist custom params, for example you can
  use the RCE editor placement;
* you can follow the test-plan:
    * https://gerrit.instructure.com/c/canvas-lms/+/256029
    * https://gerrit.instructure.com/c/canvas-lms/+/254453

[fsc-timeout=30]

Change-Id: I401f53a82f4dbef66c45932eb2eed8727488313d
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/258246
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Tucker Mcknight <tmcknight@instructure.com>
Reviewed-by: Xander Moffatt <xmoffatt@instructure.com>
Product-Review: Wagner Goncalves <wagner.goncalves@instructure.com>
This commit is contained in:
Wagner Gonçalves 2021-02-04 18:56:35 -03:00 committed by Wagner Goncalves
parent d74aa2d489
commit 005e90871c
23 changed files with 87 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/external_tools_controller.rb
View File

@ -515,7 +515,7 @@ class ExternalToolsController < ApplicationController
return nil unless params[:url]
resource_link = Lti::ResourceLink.where(
lookup_id: params[:resource_link_lookup_id],
lookup_uuid: params[:resource_link_lookup_id],
context: @context,
root_account_id: tool.root_account_id
).active.take.tap do |resource_link|

2
app/controllers/lti/ims/concerns/deep_linking_services.rb
View File

@ -59,7 +59,7 @@ module Lti::Ims::Concerns
lti_resource_links.each do |content_item|
resource_link = Lti::ResourceLink.create_with(context, tool, content_item[:custom])
content_item[:lookup_id] = resource_link&.lookup_id
content_item[:lookup_uuid] = resource_link&.lookup_uuid
end
end

3
app/controllers/lti/ims/concerns/gradebook_services.rb
View File

@ -64,7 +64,8 @@ module Lti::Ims::Concerns
def verify_line_item_in_context
line_item_context_id = Assignment.where(id: line_item.assignment_id).pluck(:context_id).first
raise ActiveRecord::RecordNotFound if line_item_context_id != params[:course_id].to_i || context.blank?
return if params[:resourceLinkId].blank? || line_item.resource_link.resource_link_id == params[:resourceLinkId]
return if params[:resourceLinkId].blank? || line_item.resource_link.resource_link_uuid == params[:resourceLinkId]
render_error("The specified LTI link ID is not associated with the line item.")
end

4
app/controllers/lti/ims/line_items_controller.rb
View File

@ -249,7 +249,7 @@ module Lti
def resource_link
@_resource_link ||= ResourceLink.find_by(
resource_link_id: params[:resourceLinkId],
resource_link_uuid: params[:resourceLinkId],
context_external_tool: tool
)
end
@ -263,7 +263,7 @@ module Lti
{
context: context,
lti_line_items: { client_id: developer_key.global_id }
}.merge!(rlid.present? ? { lti_resource_links: { resource_link_id: rlid } } : {})
}.merge!(rlid.present? ? { lti_resource_links: { resource_link_uuid: rlid } } : {})
)
{

2
app/controllers/lti/ims/providers/memberships_provider.rb
View File

@ -91,7 +91,7 @@ module Lti::Ims::Providers
def resource_link
return nil unless rlid?
@resource_link ||= begin
rl = Lti::ResourceLink.find_by(resource_link_id: rlid)
rl = Lti::ResourceLink.find_by(resource_link_uuid: rlid)
# context here is a decorated context, we want the original
if rl.present? && rl.current_external_tool(Lti::Ims::Providers::MembershipsProvider.unwrap(context))&.id != tool.id
raise Lti::Ims::AdvantageErrors::InvalidResourceLinkIdFilter.new(

11
app/jsx/deep_linking/models/LinkContentItem.js
View File

@ -32,7 +32,16 @@ export default class LinkContentItem extends ContentItem {
}
get properties() {
return Object.freeze(['url', 'title', 'text', 'icon', 'thumbnail', 'iframe', 'custom', 'lookup_id'])
return Object.freeze([
'url',
'title',
'text',
'icon',
'thumbnail',
'iframe',
'custom',
'lookup_uuid',
])
}
toHtmlString() {

8
app/jsx/deep_linking/models/ResourceLinkContentItem.js
View File

@ -21,14 +21,14 @@ import LinkContentItem from './LinkContentItem'
export default class ResourceLinkContentItem extends LinkContentItem {
constructor(json, ltiEndpoint, selection) {
super(json, ltiEndpoint, selection)
this.url = `${ltiEndpoint}?${this.ltiEndpointParams(json.url, json.lookup_id)}`
this.url = `${ltiEndpoint}?${this.ltiEndpointParams(json.url, json.lookup_uuid)}`
}
ltiEndpointParams(url, lookupId) {
ltiEndpointParams(url, lookupUuid) {
let endpointParams = 'display=borderless'
if (lookupId !== null && lookupId !== undefined) {
endpointParams += `&resource_link_lookup_id=${lookupId}`
if (lookupUuid !== null && lookupUuid !== undefined) {
endpointParams += `&resource_link_lookup_id=${lookupUuid}`
}
return `${endpointParams}&url=${encodeURIComponent(url)}`

6
app/jsx/deep_linking/models/__tests__/LinkContentItem.test.js
View File

@ -31,7 +31,7 @@ const json = {
root_account_id: '$Canvas.rootAccount.id',
referer: 'LTI test tool example'
},
lookup_id: '0b8fbc86-fdd7-4950-852d-ffa789b37ff2'
lookup_uuid: '0b8fbc86-fdd7-4950-852d-ffa789b37ff2',
}
const linkContentItem = (overrides, selection) => {
@ -68,8 +68,8 @@ describe('constructor', () => {
expect(linkContentItem().custom).toEqual(json.custom)
})
it('sets the lookup_id when present', () => {
expect(linkContentItem().lookup_id).toEqual(json.lookup_id)
it('sets the lookup_uuid when present', () => {
expect(linkContentItem().lookup_uuid).toEqual(json.lookup_uuid)
})
describe('when there is a user selection', () => {

4
app/jsx/deep_linking/models/__tests__/ResourceLinkContentItem.test.js
View File

@ -21,11 +21,11 @@ import ResourceLinkContentItem from '../ResourceLinkContentItem'
const url = 'https://www.test.com/launch'
const endpoint = 'http://test.canvas.com/accounts/1/external_tools/retrieve'
const title = 'Tool Title'
const lookup_id = '0b8fbc86-fdd7-4950-852d-ffa789b37ff2'
const lookup_uuid = '0b8fbc86-fdd7-4950-852d-ffa789b37ff2'
const json = {
url,
title,
lookup_id
lookup_uuid,
}
const resourceLinkContentItem = (overrides, launchEndpoint) => {

5
app/models/assignment.rb
View File

@ -1096,7 +1096,8 @@ class Assignment < ActiveRecord::Base
rl = Lti::ResourceLink.create!(
context: self,
custom: lti_resource_link_custom_params_as_hash,
resource_link_id: lti_context_id,
resource_link_id: lti_context_id, # we'll remove this on part 4 commit
resource_link_uuid: lti_context_id,
context_external_tool: ContextExternalTool.from_content_tag(
external_tool_tag,
context
@ -1127,7 +1128,7 @@ class Assignment < ActiveRecord::Base
def primary_resource_link
@primary_resource_link ||= begin
lti_resource_links.find_by(
resource_link_id: lti_context_id,
resource_link_uuid: lti_context_id,
context: self
)
end

2
app/serializers/lti/ims/line_items_serializer.rb
View File

@ -29,7 +29,7 @@ module Lti::Ims
label: @line_item.label,
resourceId: @line_item.resource_id,
tag: @line_item.tag,
resourceLinkId: @line_item.resource_link&.resource_link_id
resourceLinkId: @line_item.resource_link&.resource_link_uuid
}.merge(@line_item.extensions).compact
end
end

28
db/migrate/20210202120000_change_lookup_id_and_resource_link_id_to_null_at_lti_resource_links.rb Normal file
View File

@ -0,0 +1,28 @@
# frozen_string_literal: true
# Copyright (C) 2021 - 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 ChangeLookupIdAndResourceLinkIdToNullAtLtiResourceLinks < ActiveRecord::Migration[5.2]
tag :postdeploy
disable_ddl_transaction!
def change
change_column_null :lti_resource_links, :lookup_id, true
change_column_null :lti_resource_links, :resource_link_id, true
end
end

2
lib/lti/messages/resource_link_request.rb
View File

@ -65,7 +65,7 @@ module Lti::Messages
def add_resource_link_request_claims!
resource_link = assignment_resource_link
assignment = line_item_for_assignment&.assignment
@message.resource_link.id = resource_link&.resource_link_id || context_resource_link_id
@message.resource_link.id = resource_link&.resource_link_uuid || context_resource_link_id
@message.resource_link.description = resource_link && assignment&.description
@message.resource_link.title = resource_link && assignment&.title
end

4
spec/controllers/application_controller_spec.rb
View File

@ -1019,7 +1019,7 @@ RSpec.describe ApplicationController do
assignment.line_items.destroy_all
Lti::ResourceLink.where(
resource_link_id: assignment.lti_context_id
resource_link_uuid: assignment.lti_context_id
).destroy_all
assignment.update!(lti_context_id: SecureRandom.uuid)
@ -1033,7 +1033,7 @@ RSpec.describe ApplicationController do
it 'creates the LTI resource link' do
expect(
Lti::ResourceLink.where(resource_link_id: assignment.lti_context_id)
Lti::ResourceLink.where(resource_link_uuid: assignment.lti_context_id)
).to be_present
end
end

6
spec/controllers/external_tools_controller_spec.rb
View File

@ -768,7 +768,7 @@ describe ExternalToolsController do
get 'retrieve', params: {
course_id: @course.id,
url: 'http://www.example.com/launch',
resource_link_lookup_id: rl.lookup_id
resource_link_lookup_id: rl.lookup_uuid
}
}
@ -2133,7 +2133,7 @@ describe ExternalToolsController do
assignment.line_items.destroy_all
Lti::ResourceLink.where(
resource_link_id: assignment.lti_context_id
resource_link_uuid: assignment.lti_context_id
).destroy_all
get :generate_sessionless_launch, params: params
@ -2144,7 +2144,7 @@ describe ExternalToolsController do
end
it 'creates the missing resource link' do
expect(Lti::ResourceLink.where(resource_link_id: assignment.lti_context_id)).to be_present
expect(Lti::ResourceLink.where(resource_link_uuid: assignment.lti_context_id)).to be_present
end
end
end

4
spec/controllers/lti/ims/deep_linking_controller_spec.rb
View File

@ -95,13 +95,13 @@ module Lti
expect(account.lti_resource_links.first.context).to eq account
end
it 'add the lookup_id to the to the content item' do
it 'add the lookup_uuid to the to the content item' do
subject
content_items = controller.content_items
lti_resource_links = account.lti_resource_links
expect(content_items.first['lookup_id']).to eq lti_resource_links.first.lookup_id
expect(content_items.first['lookup_uuid']).to eq lti_resource_links.first.lookup_uuid
end
end

15
spec/controllers/lti/ims/line_items_controller_spec.rb
View File

@ -32,7 +32,7 @@ module Lti
let(:unknown_context_id) { (Course.maximum(:id) || 0) + 1 }
let(:resource_link) do
if tool.present? && tool.use_1_3?
resource_link_model(overrides: {resource_link_id: assignment.lti_context_id})
resource_link_model(overrides: {resource_link_uuid: assignment.lti_context_id})
else
resource_link_model
end
@ -147,7 +147,7 @@ module Lti
label: label,
resourceId: resource_id,
tag: tag,
resourceLinkId: item.resource_link.resource_link_id
resourceLinkId: item.resource_link.resource_link_uuid
}.with_indifferent_access
expect(parsed_response_body).to eq expected_response
@ -307,7 +307,8 @@ module Lti
it 'creates a line item with resource link, tag, and extensions' do
send_request
expect(item.resource_link).to_not be_blank
expect(item.resource_link.resource_link_id).to_not be_blank
expect(item.resource_link.resource_link_id).to_not be_blank # we'll remove this on part 4 commit
expect(item.resource_link.resource_link_uuid).to_not be_blank
expect(item.tag).to_not be_blank
expect(item.extensions).to_not be_blank
end
@ -324,7 +325,7 @@ module Lti
label: label,
resourceId: resource_id,
tag: tag,
resourceLinkId: item.resource_link.resource_link_id
resourceLinkId: item.resource_link.resource_link_uuid
}.with_indifferent_access
expect(parsed_response_body).to eq expected_response
@ -486,7 +487,7 @@ module Lti
id: "http://test.host/api/lti/courses/#{course.id}/line_items/#{line_item.id}",
scoreMaximum: 10.0,
label: 'Test Line Item',
resourceLinkId: line_item.resource_link.resource_link_id
resourceLinkId: line_item.resource_link.resource_link_uuid
}.with_indifferent_access
expect(parsed_response_body).to eq expected_response
@ -527,7 +528,7 @@ module Lti
scoreMaximum: 10.0,
label: 'Test Line Item',
tag: tag,
resourceLinkId: line_item.resource_link.resource_link_id
resourceLinkId: line_item.resource_link.resource_link_uuid
}.with_indifferent_access
expect(parsed_response_body).to eq expected_response
end
@ -659,7 +660,7 @@ module Lti
tool: tool
)
end
let(:params_overrides) { super().merge(resource_link_id: line_item_new_lti_link.resource_link.resource_link_id) }
let(:params_overrides) { super().merge(resource_link_id: line_item_new_lti_link.resource_link.resource_link_uuid) }
it 'correctly queries by resource_link_id' do
send_request

2
spec/factories/line_item_factory.rb
View File

@ -53,7 +53,7 @@ module Factories
base_line_item_params(assignment).merge(resource_link: overrides.fetch(
:resource_link,
overrides[:with_resource_link] ?
resource_link_model(overrides: overrides.merge(resource_link_id: assignment.lti_context_id)) :
resource_link_model(overrides: overrides.merge(resource_link_uuid: assignment.lti_context_id)) :
nil
))
end

3
spec/factories/resource_link_factory.rb
View File

@ -20,7 +20,8 @@
module Factories
def resource_link_model(overrides: {})
return Lti::ResourceLink.find_by!(resource_link_id: overrides[:resource_link_id]) if overrides.key?(:resource_link_id)
overrides[:resource_link_id] = overrides[:resource_link_uuid] if overrides.key?(:resource_link_uuid) # we'll remove this on part 4 commit
return Lti::ResourceLink.find_by!(resource_link_uuid: overrides[:resource_link_uuid]) if overrides.key?(:resource_link_uuid)
context ||= Course.create!(name: 'Course')
assignment = Assignment.create!(course: context, name: 'Assignment')

3
spec/lib/data_fixup/lti/fill_custom_claim_columns_for_resource_link_spec.rb
View File

@ -28,7 +28,8 @@ RSpec.describe DataFixup::Lti::FillCustomClaimColumnsForResourceLink do
Lti::ResourceLink.create!(context_external_tool: tool,
context_id: 999,
context_type: 'Account',
resource_link_id: assignment.lti_context_id)
resource_link_id: assignment.lti_context_id, # this should be removed in part 4
resource_link_uuid: assignment.lti_context_id)
end
describe '.run' do

2
spec/lib/lti/messages/resource_link_request_spec.rb
View File

@ -311,7 +311,7 @@ describe Lti::Messages::ResourceLinkRequest do
# these take `claims` as a method arg b/c we sometimes need to test two different jws structs in the same example where
# it is not practical to define one or both with `let`
def expect_assignment_resource_link_id(claims)
rlid = expected_assignment_line_item.resource_link.resource_link_id
rlid = expected_assignment_line_item.resource_link.resource_link_uuid
expect(claims.dig('https://purl.imsglobal.org/spec/lti/claim/resource_link', 'id')).to eq rlid
end

9
spec/models/assignment_spec.rb
View File

@ -8995,7 +8995,8 @@ describe Assignment do
expect(assignment.line_items.first.score_maximum).to eq assignment.points_possible
expect(assignment.line_items.first.coupled).to eq true
expect(assignment.line_items.first.resource_link).not_to be_nil
expect(assignment.line_items.first.resource_link.resource_link_id).to eq assignment.lti_context_id
expect(assignment.line_items.first.resource_link.resource_link_id).to eq assignment.lti_context_id # we'll remove this on part 4 commit
expect(assignment.line_items.first.resource_link.resource_link_uuid).to eq assignment.lti_context_id
expect(assignment.line_items.first.resource_link.context_id).to eq assignment.id
expect(assignment.line_items.first.resource_link.context_type).to eq 'Assignment'
expect(assignment.line_items.first.resource_link.custom).to eq custom_params.with_indifferent_access
@ -9110,7 +9111,7 @@ describe Assignment do
assignment.line_items.destroy_all
Lti::ResourceLink.where(
resource_link_id: assignment.lti_context_id
resource_link_uuid: assignment.lti_context_id
).destroy_all
assignment.update!(lti_context_id: SecureRandom.uuid)
@ -9125,7 +9126,7 @@ describe Assignment do
it 'creates the LTI resource link' do
expect(
Lti::ResourceLink.where(
resource_link_id: subject.lti_context_id
resource_link_uuid: subject.lti_context_id
)
).to be_present
end
@ -9142,7 +9143,7 @@ describe Assignment do
expect {
assignment.prepare_for_ags_if_needed!(tool)
}.not_to change {
Lti::ResourceLink.where(resource_link_id: subject.lti_context_id).
Lti::ResourceLink.where(resource_link_uuid: subject.lti_context_id).
first.
id
}

4
spec/serializers/lti/ims/line_items_serializer_spec.rb
View File

@ -20,7 +20,7 @@
require 'spec_helper'
RSpec.describe Lti::Ims::LineItemsSerializer do
let(:resource_link) { resource_link_model(overrides: {resource_link_id: assignment.lti_context_id}) }
let(:resource_link) { resource_link_model(overrides: {resource_link_uuid: assignment.lti_context_id}) }
let_once(:course) { course_model }
let(:tool) {
ContextExternalTool.create!(
@ -72,7 +72,7 @@ RSpec.describe Lti::Ims::LineItemsSerializer do
label: line_item.label,
resourceId: line_item.resource_id,
tag: line_item.tag,
resourceLinkId: line_item.resource_link&.resource_link_id
resourceLinkId: line_item.resource_link&.resource_link_uuid
}
)
end