From dcee370f662df35a296ed0b5bf784136a56b8376 Mon Sep 17 00:00:00 2001 From: wdransfield Date: Fri, 5 Oct 2018 13:17:50 -0600 Subject: [PATCH] Save Custom Fields Closes PLAT-3820 Test Plan: * Specify custom fields in the customization form * Save the key & configuration * create a tool from that configuration and verify the custom fields have been added Change-Id: I00a89f7a5f7daceacb4260465e4c099da418bcb9 Reviewed-on: https://gerrit.instructure.com/167702 Tested-by: Jenkins Reviewed-by: Cody Cutrer Reviewed-by: Marc Phillips QA-Review: Marc Phillips Product-Review: Weston Dransfield --- .../lti/tool_configurations_api_controller.rb | 19 +++++++++++---- app/jsx/developer_keys/CustomizationForm.js | 23 ++++++++++-------- app/jsx/developer_keys/NewKeyModal.js | 5 +++- app/jsx/developer_keys/ToolConfiguration.js | 2 +- .../actions/__tests__/ltiKeyActions.test.js | 7 ++++-- .../developer_keys/actions/ltiKeyActions.js | 3 ++- .../reducers/createLtiKeyReducer.js | 2 +- app/models/lti/tool_configuration.rb | 1 + ...ustom_fields_to_lti_tool_configurations.rb | 24 +++++++++++++++++++ ...tool_configurations_api_controller_spec.rb | 8 ++++++- .../jsx/developer_keys/NewKeyModalSpec.js | 2 +- spec/models/lti/tool_configuration_spec.rb | 7 ++++-- 12 files changed, 79 insertions(+), 24 deletions(-) create mode 100644 db/migrate/20181005184917_add_custom_fields_to_lti_tool_configurations.rb diff --git a/app/controllers/lti/tool_configurations_api_controller.rb b/app/controllers/lti/tool_configurations_api_controller.rb index 2d2ae508757..4b17ab66476 100644 --- a/app/controllers/lti/tool_configurations_api_controller.rb +++ b/app/controllers/lti/tool_configurations_api_controller.rb @@ -72,13 +72,19 @@ class Lti::ToolConfigurationsApiController < ApplicationController # placements should be excluded from the # tool configuration. # + # @argument custom_fields [String] + # A new line seperated string of key/value pairs + # to be used as custom fields in the LTI launch. + # Example: foo=bar\ncourse=$Canvas.course.id + # # @returns ToolConfiguration def create tool_config = Lti::ToolConfiguration.create!( developer_key: DeveloperKey.create!(account: account), settings: tool_configuration_params[:settings], settings_url: tool_configuration_params[:settings_url], - disabled_placements: tool_configuration_params[:disabled_placements] + disabled_placements: tool_configuration_params[:disabled_placements], + custom_fields: tool_configuration_params[:custom_fields] ) update_developer_key!(tool_config) render json: tool_config @@ -113,6 +119,10 @@ class Lti::ToolConfigurationsApiController < ApplicationController # An array of strings indicating which Canvas # placements should be excluded from the # tool configuration. + # @argument custom_fields [String] + # A new line seperated string of key/value pairs + # to be used as custom fields in the LTI launch. + # Example: foo=bar\ncourse=$Canvas.course.id # # @returns ToolConfiguration def update @@ -120,7 +130,8 @@ class Lti::ToolConfigurationsApiController < ApplicationController tool_config.update!( settings: tool_configuration_params[:settings], settings_url: tool_configuration_params[:settings_url], - disabled_placements: tool_configuration_params[:disabled_placements] + disabled_placements: tool_configuration_params[:disabled_placements], + custom_fields: tool_configuration_params[:custom_fields] ) update_developer_key!(tool_config) @@ -169,8 +180,8 @@ class Lti::ToolConfigurationsApiController < ApplicationController end def tool_configuration_params - params.require(:tool_configuration).permit(:settings_url, disabled_placements: []).merge( - { settings: params.require(:tool_configuration)[:settings]&.to_unsafe_h } + params.require(:tool_configuration).permit(:settings_url, :custom_fields, disabled_placements: []).merge( + {settings: params.require(:tool_configuration)[:settings]&.to_unsafe_h} ) end diff --git a/app/jsx/developer_keys/CustomizationForm.js b/app/jsx/developer_keys/CustomizationForm.js index 5c0a5a08689..324211b153e 100644 --- a/app/jsx/developer_keys/CustomizationForm.js +++ b/app/jsx/developer_keys/CustomizationForm.js @@ -51,7 +51,9 @@ export default class CustomizationForm extends React.Component { } // Intersection of requested scopes and valid scopes - return toolConfiguration.scopes.filter(scope => validScopeNames.includes(scope)).map(s => validScopes[s]) + return toolConfiguration.scopes + .filter(scope => validScopeNames.includes(scope)) + .map(s => validScopes[s]) } get placements() { @@ -62,9 +64,9 @@ export default class CustomizationForm extends React.Component { } // Get Canvas specific extensions from the tool config - const extension = toolConfiguration.extensions.find(ext => ( - ext.platform === 'canvas.instructure.com' - )) + const extension = toolConfiguration.extensions.find( + ext => ext.platform === 'canvas.instructure.com' + ) if (!(extension && extension.settings)) { return [] @@ -75,13 +77,13 @@ export default class CustomizationForm extends React.Component { } componentDidMount() { - const { dispatch, setEnabledScopes } = this.props - const initialScopes = this.scopes.map((s) => this.invertedScopes[s]) + const {dispatch, setEnabledScopes} = this.props + const initialScopes = this.scopes.map(s => this.invertedScopes[s]) dispatch(setEnabledScopes(initialScopes)) } - handleScopeChange = (e) => { + handleScopeChange = e => { const {dispatch, setEnabledScopes} = this.props const value = this.invertedScopes[e.target.value] const newEnabledScopes = this.props.enabledScopes.slice() @@ -89,8 +91,8 @@ export default class CustomizationForm extends React.Component { dispatch(setEnabledScopes(this.toggleArrayItem(newEnabledScopes, value))) } - handlePlacementChange = (e) => { - const { dispatch, setDisabledPlacements, validPlacements } = this.props + handlePlacementChange = e => { + const {dispatch, setDisabledPlacements, validPlacements} = this.props const value = e.target.value const newDisabledPlacements = this.props.disabledPlacements.slice() @@ -118,7 +120,7 @@ export default class CustomizationForm extends React.Component { name={I18n.t('Services')} options={scopes} onOptionToggle={this.handleScopeChange} - selectedOptions={this.props.enabledScopes.map((s) => (this.props.validScopes[s]))} + selectedOptions={this.props.enabledScopes.map(s => this.props.validScopes[s])} type="scope" /> ) @@ -147,6 +149,7 @@ export default class CustomizationForm extends React.Component { maxHeight="20rem" width="50%" messages={[{text: I18n.t('One per line. Format: name=value'), type: 'hint'}]} + name="custom_fields" /> ) } diff --git a/app/jsx/developer_keys/NewKeyModal.js b/app/jsx/developer_keys/NewKeyModal.js index 143147a8565..d3349af6c88 100644 --- a/app/jsx/developer_keys/NewKeyModal.js +++ b/app/jsx/developer_keys/NewKeyModal.js @@ -59,12 +59,15 @@ export default class DeveloperKeyModal extends React.Component { } saveCustomizations = () => { + const customFields = new FormData(this.submissionForm).get('custom_fields') const { store, actions, createLtiKeyState, createOrEditDeveloperKeyState } = this.props + store.dispatch(actions.ltiKeysUpdateCustomizations( createLtiKeyState.enabledScopes, createLtiKeyState.disabledPlacements, createOrEditDeveloperKeyState.developerKey.id, - createLtiKeyState.toolConfiguration + createLtiKeyState.toolConfiguration, + customFields )) this.closeModal() } diff --git a/app/jsx/developer_keys/ToolConfiguration.js b/app/jsx/developer_keys/ToolConfiguration.js index 8cc981d6f03..51e82d92317 100644 --- a/app/jsx/developer_keys/ToolConfiguration.js +++ b/app/jsx/developer_keys/ToolConfiguration.js @@ -64,6 +64,6 @@ ToolConfiguration.propTypes = { toolConfiguration: PropTypes.object.isRequired, toolConfigurationUrl: PropTypes.string.isRequired, enabledScopes: PropTypes.arrayOf(PropTypes.string).isRequired, - disabledPlacements: PropTypes.arrayOf(PropTypes.string).isRequired + disabledPlacements: PropTypes.arrayOf(PropTypes.string).isRequired, }).isRequired } diff --git a/app/jsx/developer_keys/actions/__tests__/ltiKeyActions.test.js b/app/jsx/developer_keys/actions/__tests__/ltiKeyActions.test.js index 9c995a360ba..8dc3c9b06c2 100644 --- a/app/jsx/developer_keys/actions/__tests__/ltiKeyActions.test.js +++ b/app/jsx/developer_keys/actions/__tests__/ltiKeyActions.test.js @@ -74,13 +74,15 @@ describe('ltiKeysUpdateCustomizations', () => { const disabledPlacements = ['account_navigation', 'course_navigaiton'] const developerKeyId = 123 const toolConfiguration = {} + const customFields = "foo=bar\r\nkey=value" const update = (dispatch) => { actions.ltiKeysUpdateCustomizations( scopes, disabledPlacements, developerKeyId, - toolConfiguration + toolConfiguration, + customFields )(dispatch) } @@ -95,7 +97,8 @@ describe('ltiKeysUpdateCustomizations', () => { }, tool_configuration: { disabled_placements: disabledPlacements, - settings: toolConfiguration + settings: toolConfiguration, + custom_fields: customFields } } ) diff --git a/app/jsx/developer_keys/actions/ltiKeyActions.js b/app/jsx/developer_keys/actions/ltiKeyActions.js index baecda2f2d5..9cf94ed8559 100644 --- a/app/jsx/developer_keys/actions/ltiKeyActions.js +++ b/app/jsx/developer_keys/actions/ltiKeyActions.js @@ -137,7 +137,7 @@ actions.ltiKeysUpdateCustomizationsSuccessful = payload => ({ }) actions.LTI_KEYS_UPDATE_CUSTOMIZATIONS = 'LTI_KEYS_UPDATE_CUSTOMIZATIONS' -actions.ltiKeysUpdateCustomizations = (scopes, disabled_placements, developerKeyId, toolConfiguration) => dispatch => { +actions.ltiKeysUpdateCustomizations = (scopes, disabled_placements, developerKeyId, toolConfiguration, customFields) => dispatch => { dispatch(actions.ltiKeysUpdateCustomizationsStart()) const url = `/api/lti/developer_keys/${developerKeyId}/tool_configuration` @@ -147,6 +147,7 @@ actions.ltiKeysUpdateCustomizations = (scopes, disabled_placements, developerKey scopes }, tool_configuration: { + custom_fields: customFields, disabled_placements, settings: toolConfiguration } diff --git a/app/jsx/developer_keys/reducers/createLtiKeyReducer.js b/app/jsx/developer_keys/reducers/createLtiKeyReducer.js index 975ba0e41a6..1e71610169c 100644 --- a/app/jsx/developer_keys/reducers/createLtiKeyReducer.js +++ b/app/jsx/developer_keys/reducers/createLtiKeyReducer.js @@ -30,7 +30,7 @@ const initialState = { disabledPlacements: [], updateCustomizationsPending: false, updateCustomizationsSuccessful: false, - updateCustomizationsError: null, + updateCustomizationsError: null } const ltiKeysHandlers = { diff --git a/app/models/lti/tool_configuration.rb b/app/models/lti/tool_configuration.rb index 3f05da88588..e7a562dfdf6 100644 --- a/app/models/lti/tool_configuration.rb +++ b/app/models/lti/tool_configuration.rb @@ -41,6 +41,7 @@ module Lti false ) tool.developer_key = developer_key + tool.custom_fields_string = tool.custom_fields_string + "\n#{custom_fields}" tool end diff --git a/db/migrate/20181005184917_add_custom_fields_to_lti_tool_configurations.rb b/db/migrate/20181005184917_add_custom_fields_to_lti_tool_configurations.rb new file mode 100644 index 00000000000..54d5c7815b4 --- /dev/null +++ b/db/migrate/20181005184917_add_custom_fields_to_lti_tool_configurations.rb @@ -0,0 +1,24 @@ +# +# 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 . + +class AddCustomFieldsToLtiToolConfigurations < ActiveRecord::Migration[5.1] + tag :predeploy + + def change + add_column :lti_tool_configurations, :custom_fields, :text + end +end diff --git a/spec/controllers/lti/tool_configurations_api_controller_spec.rb b/spec/controllers/lti/tool_configurations_api_controller_spec.rb index 6811ca1bff1..246d338c4e8 100644 --- a/spec/controllers/lti/tool_configurations_api_controller_spec.rb +++ b/spec/controllers/lti/tool_configurations_api_controller_spec.rb @@ -159,7 +159,8 @@ RSpec.describe Lti::ToolConfigurationsApiController, type: :controller do developer_key_id: developer_key.id, tool_configuration: { settings_url: url, - disabled_placements: ['course_navigation', 'account_navigation'] + disabled_placements: ['course_navigation', 'account_navigation'], + custom_fields: "foo=bar\r\nkey=value" } } end @@ -181,6 +182,11 @@ RSpec.describe Lti::ToolConfigurationsApiController, type: :controller do valid_parameters.dig(:tool_configuration, :disabled_placements) ) end + + it 'sets the "custom_fields"' do + subject + expect(config_from_response.custom_fields).to eq valid_parameters.dig(:tool_configuration, :custom_fields) + end end context 'when the request times out' do diff --git a/spec/javascripts/jsx/developer_keys/NewKeyModalSpec.js b/spec/javascripts/jsx/developer_keys/NewKeyModalSpec.js index af75f7c531e..c35418320b8 100644 --- a/spec/javascripts/jsx/developer_keys/NewKeyModalSpec.js +++ b/spec/javascripts/jsx/developer_keys/NewKeyModalSpec.js @@ -396,6 +396,6 @@ test('saves customizations', () => { /> ) wrapper.instance().saveCustomizations() - ok(ltiStub.calledWith(['https://www.test.com/lineitem'], ['account_navigation'], 22, {})) + ok(ltiStub.calledWith(['https://www.test.com/lineitem'], ['account_navigation'], 22, {}, null)) wrapper.unmount() }) diff --git a/spec/models/lti/tool_configuration_spec.rb b/spec/models/lti/tool_configuration_spec.rb index 827ef341bc1..27c007adc0a 100644 --- a/spec/models/lti/tool_configuration_spec.rb +++ b/spec/models/lti/tool_configuration_spec.rb @@ -232,7 +232,10 @@ module Lti let(:extensions) { settings['extensions'].first } - before { tool_configuration.developer_key = developer_key } + before do + tool_configuration.developer_key = developer_key + tool_configuration.custom_fields = "key=value\nfoo=bar" + end shared_examples_for 'a new context external tool' do context 'when "disabled_placements" is set' do @@ -273,7 +276,7 @@ module Lti end it 'uses the correct top-level custom params' do - expect(subject.custom_fields).to eq settings['custom_fields'] + expect(subject.custom_fields).to eq({"has_expansion"=>"$Canvas.user.id", "no_expansion"=>"foo", "key"=>"value", "foo"=>"bar"}) end it 'uses the correct icon url' do