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 <svc.cloudjenkins@instructure.com>
Reviewed-by: Tucker Mcknight <tmcknight@instructure.com>
QA-Review: Tucker Mcknight <tmcknight@instructure.com>
Product-Review: Alexis Nast <alexis.nast@instructure.com>

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

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

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

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",

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

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

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

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

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)