From f63456f8c0a4b955be7ee8d6bea6c4d45049cb78 Mon Sep 17 00:00:00 2001 From: Steve McGee Date: Wed, 3 Jan 2024 13:24:18 -0700 Subject: [PATCH] remove variable_substitution_numeric_to_string feature flag closes INTEROP-8353 flag=variable_substitution_numeric_to_string [fsc-timeout=30] test plan: - check that the FF is not available in the ui - all tests still pass with code removed Change-Id: I5d48a89bd50eb052ead417be5974e50edaf54cd6 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/336588 Tested-by: Service Cloud Jenkins Reviewed-by: Tucker Mcknight QA-Review: Tucker Mcknight Product-Review: Alexis Nast --- .../feature_flags/interop_release_flags.yml | 6 - lib/lti/variable_expander.rb | 4 +- .../external_tools_controller_spec.rb | 6 +- .../ims/names_and_roles_controller_spec.rb | 6 +- spec/lib/lti/messages/jwt_message_spec.rb | 2 +- .../messages/resource_link_request_spec.rb | 10 +- spec/lib/lti/variable_expander_spec.rb | 327 ------------------ spec/models/lti/lti_outbound_adapter_spec.rb | 1 - .../ims/names_and_roles_serializer_spec.rb | 6 +- 9 files changed, 16 insertions(+), 352 deletions(-) diff --git a/config/feature_flags/interop_release_flags.yml b/config/feature_flags/interop_release_flags.yml index 807a1dae157..4816cce6455 100644 --- a/config/feature_flags/interop_release_flags.yml +++ b/config/feature_flags/interop_release_flags.yml @@ -66,12 +66,6 @@ api_auth_error_updates: the "status" field will not be localized; it will always be given in English. applies_to: SiteAdmin -variable_substitution_numeric_to_string: - state: hidden - display_name: Returns string values instead of numeric values in variable substitutions - description: |- - If enabled, variable substitutions will return string values instead of numeric values - applies_to: RootAccount ags_scores_multiple_files: state: hidden display_name: Allow multiple file AGS submissions to count as only one submission diff --git a/lib/lti/variable_expander.rb b/lib/lti/variable_expander.rb index a0bfcc05f7d..d1610292ebd 100644 --- a/lib/lti/variable_expander.rb +++ b/lib/lti/variable_expander.rb @@ -157,9 +157,7 @@ module Lti v end - if @root_account&.feature_enabled?(:variable_substitution_numeric_to_string) && - @tool.is_a?(ContextExternalTool) && @tool.use_1_3? && - output.is_a?(Numeric) + if @tool.is_a?(ContextExternalTool) && @tool.use_1_3? && output.is_a?(Numeric) output&.to_s else output diff --git a/spec/controllers/external_tools_controller_spec.rb b/spec/controllers/external_tools_controller_spec.rb index d4574379b21..a5464a2566e 100644 --- a/spec/controllers/external_tools_controller_spec.rb +++ b/spec/controllers/external_tools_controller_spec.rb @@ -1057,7 +1057,7 @@ describe ExternalToolsController do get_page expect(launch_hash["https://purl.imsglobal.org/spec/lti/claim/custom"]).to include( "abc" => "def", - "expans" => @teacher.id + "expans" => @teacher.id.to_s ) end @@ -1070,7 +1070,7 @@ describe ExternalToolsController do expect(launch_hash["https://purl.imsglobal.org/spec/lti/claim/custom"]).to include( "abc" => "def", - "expans" => @teacher.id + "expans" => @teacher.id.to_s ) end @@ -1146,7 +1146,7 @@ describe ExternalToolsController do get_page expect( launch_params["https://purl.imsglobal.org/spec/lti/claim/custom"] - ).to eq({ "abc" => "def", "expans" => @teacher.id }) + ).to eq({ "abc" => "def", "expans" => @teacher.id.to_s }) end it "if parent_frame_context is not given it does not include it in lti_message_hint" do diff --git a/spec/controllers/lti/ims/names_and_roles_controller_spec.rb b/spec/controllers/lti/ims/names_and_roles_controller_spec.rb index 4aeb336515c..0039e08bd61 100644 --- a/spec/controllers/lti/ims/names_and_roles_controller_spec.rb +++ b/spec/controllers/lti/ims/names_and_roles_controller_spec.rb @@ -264,10 +264,10 @@ describe Lti::IMS::NamesAndRolesController do "person_name_family" => "Perkins", "person_name_given" => "Marta", "user_image" => "http://school.edu/image/url.png", - "user_id" => user.id, - "canvas_user_id" => user.id, + "user_id" => user.id.to_s, + "canvas_user_id" => user.id.to_s, "vnd_instructure_user_uuid" => user.uuid, - "canvas_user_globalid" => user.global_id, + "canvas_user_globalid" => user.global_id.to_s, "canvas_user_sissourceid" => @cc_pseud.sis_user_id, "person_sourced_id" => @cc_pseud.sis_user_id, "message_locale" => "de", diff --git a/spec/lib/lti/messages/jwt_message_spec.rb b/spec/lib/lti/messages/jwt_message_spec.rb index f6bf14e7e29..c915263de68 100644 --- a/spec/lib/lti/messages/jwt_message_spec.rb +++ b/spec/lib/lti/messages/jwt_message_spec.rb @@ -721,7 +721,7 @@ describe Lti::Messages::JwtMessage do it "expands variable expansions" do Lti::Messages::JwtMessage.generate_id_token(jwt_message.generate_post_payload) - expect(message_custom["has_expansion"]).to eq user.id + expect(message_custom["has_expansion"]).to eq user.id.to_s end context "when custom parameters claim group disabled" do diff --git a/spec/lib/lti/messages/resource_link_request_spec.rb b/spec/lib/lti/messages/resource_link_request_spec.rb index c696c66d653..af702416457 100644 --- a/spec/lib/lti/messages/resource_link_request_spec.rb +++ b/spec/lib/lti/messages/resource_link_request_spec.rb @@ -83,8 +83,8 @@ describe Lti::Messages::ResourceLinkRequest do context "when link-level custom params are given in resource_link" do it "merges them in with tool/placement parameters" do expect(jws["https://purl.imsglobal.org/spec/lti/claim/custom"]).to eq( - "link_has_expansion2" => assignment.id, - "has_expansion" => user.id, + "link_has_expansion2" => assignment.id.to_s, + "has_expansion" => user.id.to_s, "no_expansion" => "overrides tool param!" ) end @@ -151,8 +151,8 @@ describe Lti::Messages::ResourceLinkRequest do } ) expect(jws["https://purl.imsglobal.org/spec/lti/claim/custom"]).to eq( - "link_has_expansion" => assignment.id, - "has_expansion" => user.id, + "link_has_expansion" => assignment.id.to_s, + "has_expansion" => user.id.to_s, "no_expansion" => "overrides tool param" ) end @@ -165,7 +165,7 @@ describe Lti::Messages::ResourceLinkRequest do ) expect(jws["https://purl.imsglobal.org/spec/lti/claim/custom"]).to eq( - "has_expansion" => user.id, + "has_expansion" => user.id.to_s, "no_expansion" => "foo" ) end diff --git a/spec/lib/lti/variable_expander_spec.rb b/spec/lib/lti/variable_expander_spec.rb index 4ceabe26503..f116fde7135 100644 --- a/spec/lib/lti/variable_expander_spec.rb +++ b/spec/lib/lti/variable_expander_spec.rb @@ -2658,332 +2658,5 @@ module Lti expect(exp_hash[:test]).to eq "string stuff: create_forum,read_forum" end end - - context "when :variable_substitution_numeric_to_string feature flag is ON" do - before do - root_account.enable_feature! :variable_substitution_numeric_to_string - root_account.save! - end - - context "when using a LTI 1.3 tool" do - before do - allow(tool).to receive(:is_a?).with(ContextExternalTool).and_return(true) - allow(tool).to receive(:use_1_3?).and_return(true) - end - - it "echoes registered variable if blacklisted" do - VariableExpander.register_expansion("test_expan", ["a"], -> { @context }) - VariableExpander.register_expansion("variable1", ["a"], -> { 1 }) - variable_expander.variable_blacklist = ["test_expan"] - expanded1 = variable_expander.expand_variables!({ some_name: "$test_expan" }) - expanded2 = variable_expander.expand_variables!({ some_name: "$variable1" }) - expect(expanded1.count).to eq 1 - expect(expanded1[:some_name]).to eq "$test_expan" - expect(expanded2.count).to eq 1 - expect(expanded2[:some_name]).to eq "1" - end - - describe "#variable expansions" do - it "has substitution for com.instructure.OriginalityReport.id" do - exp_hash = { test: "$com.instructure.OriginalityReport.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq originality_report.id.to_s - end - - it "has substitution for com.instructure.Submission.id" do - exp_hash = { test: "$com.instructure.Submission.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq originality_report.submission.id.to_s - end - - it "has substitution for com.instructure.File.id" do - exp_hash = { test: "$com.instructure.File.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq originality_report.attachment.id.to_s - end - - it "has substitution for $Canvas.account.id" do - allow(account).to receive(:id).and_return(12_345) - exp_hash = { test: "$Canvas.account.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "12345" - end - - it "has substitution for $Canvas.rootAccount.id" do - allow(root_account).to receive(:id).and_return(54_321) - exp_hash = { test: "$Canvas.rootAccount.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "54321" - end - - it "has substitution for $Canvas.root_account.id" do - allow(root_account).to receive(:id).and_return(54_321) - exp_hash = { test: "$Canvas.root_account.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "54321" - end - - it "has substitution for $Canvas.root_account.global_id" do - allow(root_account).to receive(:global_id).and_return(10_054_321) - exp_hash = { test: "$Canvas.root_account.global_id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "10054321" - end - - it "has substitution for $Canvas.shard.id" do - exp_hash = { test: "$Canvas.shard.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq Shard.current.id.to_s - end - - context "when launching from a group assignment" do - let(:group) { group_category.groups.create!(name: "test", context: assignment_course) } - let(:group_category) { GroupCategory.create!(name: "test", context: assignment_course) } - let(:new_assignment) { assignment_model(course: assignment_course) } - let(:assignment_course) do - c = course_model(account:) - c.save! - c - end - let(:variable_expander) do - VariableExpander.new( - root_account, - account, - controller, - current_user: user, - tool:, - assignment: new_assignment - ) - end - - before do - group.update!(users: [user]) - new_assignment.update!(group_category:) - end - - describe "com.instructure.Group.id" do - let(:expansion_string) { "$com.instructure.Group.id" } - - it "has a substitution for com.instructure.Group.id" do - exp_hash = { test: expansion_string } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq group.id.to_s - end - end - end - - context "context is a course" do - let(:variable_expander) { VariableExpander.new(root_account, course, controller, current_user: user, tool:) } - - it "has substitution for $Canvas.course.id" do - allow(course).to receive(:id).and_return(123) - exp_hash = { test: "$Canvas.course.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "123" - end - - describe "$com.instructure.User.observees" do - subject do - exp_hash = { test: "$com.instructure.User.observees" } - variable_expander.expand_variables!(exp_hash) - exp_hash[:test] - end - - let(:context) do - c = variable_expander.context - c.save! - c - end - let(:student) { user_factory } - let(:observer) { user_factory } - - before do - context.enroll_student(student) - variable_expander.current_user = observer - end - - context "when the current user is observing users in the context" do - before do - observer_enrollment = context.enroll_user(observer, "ObserverEnrollment") - observer_enrollment.update!(associated_user_id: student.id) - end - - context "the tool in use is not a LTI 1.3 tool" do - before do - allow(tool).to receive(:use_1_3?).and_return(false) - end - - it "produces a comma-separated string of user UUIDs" do - expect(subject.split(",")).to match_array [ - Lti::Asset.opaque_identifier_for(student) - ] - end - end - - context "the tool in use is a LTI 1.3 tool" do - it "returns the users' lti id instead of lti 1.1 user_id" do - expect(subject.split(",")).to match_array [ - student.lti_id - ] - end - end - end - end - end - - context "context is a course and there is a user" do - let(:variable_expander) { VariableExpander.new(root_account, course, controller, current_user: user, tool:) } - let(:user) { user_factory } - - it "has substitution for $Canvas.externalTool.global_id" do - course.save! - tool = course.context_external_tools.create!(domain: "example.com", consumer_key: "12345", shared_secret: "secret", privacy_level: "anonymous", name: "tool", use_1_3: true) - expander = VariableExpander.new(root_account, course, controller, current_user: user, tool:) - exp_hash = { test: "$Canvas.externalTool.global_id" } - expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq tool.global_id.to_s - end - end - - context "context is a course with an assignment and a user" do - let(:variable_expander) { VariableExpander.new(root_account, course, controller, current_user: user, tool:, assignment:) } - - it "has substitution for $Canvas.assignment.id" do - allow(assignment).to receive(:id).and_return(2015) - exp_hash = { test: "$Canvas.assignment.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "2015" - end - - describe "$Canvas.assignment.pointsPossible" do - it "has substitution for $Canvas.assignment.pointsPossible" do - allow(assignment).to receive(:points_possible).and_return(10.0) - exp_hash = { test: "$Canvas.assignment.pointsPossible" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "10" - end - - it "does not round if not whole" do - allow(assignment).to receive(:points_possible).and_return(9.5) - exp_hash = { test: "$Canvas.assignment.pointsPossible" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "9.5" - end - - it "rounds if whole" do - allow(assignment).to receive(:points_possible).and_return(9.0) - exp_hash = { test: "$Canvas.assignment.pointsPossible" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "9" - end - end - - it "has substitution for $Canvas.assignment.allowedAttempts" do - assignment.allowed_attempts = 5 - exp_hash = { allowed_attempts: "$Canvas.assignment.allowedAttempts" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:allowed_attempts]).to eq "5" - end - - describe "$Canvas.assignment.submission.studentAttempts" do - before do - user.save - course.save - assignment.context = course - assignment.save - submission = submission_model(user:, assignment:) - submission.attempt = 2 - submission.save - end - - it "has substitution when the user is a student" do - allow(course).to receive(:user_is_student?).and_return(true) - exp_hash = { attempts: "$Canvas.assignment.submission.studentAttempts" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:attempts]).to eq "2" - end - end - end - - context "user is logged in" do - it "has substitution for $Canvas.user.id" do - allow(user).to receive(:id).and_return(456) - exp_hash = { test: "$Canvas.user.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "456" - end - - it "has substitution for $Canvas.user.globalId" do - allow(user).to receive(:global_id).and_return(456) - exp_hash = { test: "$Canvas.user.globalId" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "456" - end - - it "has substitution for $User.id" do - allow(user).to receive(:id).and_return(456) - exp_hash = { test: "$User.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "456" - end - - context "attachment" do - let(:attachment) do - attachment = attachment_obj_with_context(course) - attachment.media_object = media_object - attachment.usage_rights = usage_rights - attachment - end - let(:media_object) do - mo = MediaObject.new - mo.media_id = "1234" - mo.media_type = "video" - mo.duration = 555 - mo.total_size = 444 - mo.title = "some title" - mo - end - let(:usage_rights) do - ur = UsageRights.new - ur.legal_copyright = "legit" - ur - end - let(:variable_expander) { VariableExpander.new(root_account, account, controller, current_user: user, tool:, attachment:) } - - it "has substitution for $Canvas.file.media.duration" do - exp_hash = { test: "$Canvas.file.media.duration" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "555" - end - - it "has substitution for $Canvas.file.media.size" do - exp_hash = { test: "$Canvas.file.media.size" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "444" - end - end - - it "has substitution for $Canvas.masqueradingUser.id" do - masquerading_user = User.new - allow(masquerading_user).to receive(:id).and_return(7878) - allow(user).to receive(:id).and_return(42) - variable_expander.instance_variable_set(:@current_user, masquerading_user) - exp_hash = { test: "$Canvas.masqueradingUser.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "42" - end - - it "has substitution for Canvas.moduleItem.id" do - content_tag = double("content_tag") - allow(content_tag).to receive(:id).and_return(7878) - variable_expander.instance_variable_set(:@content_tag, content_tag) - exp_hash = { test: "$Canvas.moduleItem.id" } - variable_expander.expand_variables!(exp_hash) - expect(exp_hash[:test]).to eq "7878" - end - end - end - end - end end end diff --git a/spec/models/lti/lti_outbound_adapter_spec.rb b/spec/models/lti/lti_outbound_adapter_spec.rb index 0546c52b9f4..8659fd0c716 100644 --- a/spec/models/lti/lti_outbound_adapter_spec.rb +++ b/spec/models/lti/lti_outbound_adapter_spec.rb @@ -288,7 +288,6 @@ describe Lti::LtiOutboundAdapter do it "does not copy query params to the post body if :disable_post_only is set on root_Account" do allow(account).to receive(:all_account_users_for).with(user).and_return([]) - allow(account).to receive(:feature_enabled?).with(:variable_substitution_numeric_to_string).and_return(false) allow(account).to receive(:feature_enabled?).with(:disable_lti_post_only).and_return(true) adapter.prepare_tool_launch(return_url, variable_expander) payload = adapter.generate_post_payload diff --git a/spec/serializers/lti/ims/names_and_roles_serializer_spec.rb b/spec/serializers/lti/ims/names_and_roles_serializer_spec.rb index 5583e520214..4aff03d23ad 100644 --- a/spec/serializers/lti/ims/names_and_roles_serializer_spec.rb +++ b/spec/serializers/lti/ims/names_and_roles_serializer_spec.rb @@ -161,8 +161,8 @@ describe Lti::IMS::NamesAndRolesSerializer do let(:message_matcher) do { "https://purl.imsglobal.org/spec/lti/claim/custom" => { - "user_id" => user.id, - "canvas_user_id" => user.id, + "user_id" => user.id.to_s, + "canvas_user_id" => user.id.to_s, "unsupported_param_1" => "$unsupported.param.1", "unsupported_param_2" => "$unsupported.param.2" }.merge(custom_params) @@ -282,7 +282,7 @@ describe Lti::IMS::NamesAndRolesSerializer do expect(received_custom_claim["canvas_course_endat"]).to eq course.end_at.utc.iso8601 expect(received_custom_claim["canvas_course_gradepassbacksetting"]).to eq course.grade_passback_setting expect(received_custom_claim["canvas_course_hidedistributiongraphs"]).to eq course.hide_distribution_graphs? - expect(received_custom_claim["canvas_course_id"]).to eq course.id + expect(received_custom_claim["canvas_course_id"]).to eq course.id.to_s expect(received_custom_claim["canvas_course_name"]).to eq course.name lti_helper = Lti::SubstitutionsHelper.new(course, course.root_account, user, tool)