instructure / canvas-lms

The open LMS by Instructure, Inc.
https://github.com/instructure/canvas-lms/wiki
GNU Affero General Public License v3.0
5.57k stars 2.48k forks source link

Canvas beta changed LTI deeplinking behaviour for assignment titles #2211

Closed marcospri closed 1 year ago

marcospri commented 1 year ago

Summary:

Canvas beta changed the interaction between assignment titles and deep linking.

In production deep linked content with no title provided by the tool defaults to the assignment's own name, leaving it unmodified while editing and creating new assignments.

In Beta this behavior changes, defaulting to the tool's own name for the title of the assignment which is arguably a much worse default for the title of the current assignment.

Steps to reproduce:

Editing

  1. Use an external tool as "Submission Type" in an existing assignment, note the assignment's current title.

  2. The tool returns a deep linking message with no title, eg the following content type in LTI1.3 (although the problem also occurs in LTI1.1):

    
    https://purl.imsglobal.org/spec/lti-dl/claim/content_items": [                                                                                                                                                                                      
    {"type": "ltiResourceLink", "url": "SOME URL"}       
    ]


Note the message is missing the optional "title" value: https://www.imsglobal.org/spec/lti-dl/v2p0#lti-resource-link

3. Complete the deep linking flow and click "Select" on the "Configure External Tool" modal.
4. The title of the assignment has changed, it now has the value of the tool's name.

### Expected behavior:

The assignment title shouldn't change after a deep-linked request if the content linked doesn't have a title. This is how this behaves in production now.

### Actual behavior:

The assignment title changes.

### Additional notes:

I'm not 100% sure but this could be related to the changes made recently here:

https://github.com/instructure/canvas-lms/commit/58a743fe31bce8142e9c32d52354582aaccf24f3#diff-7944a1f02e4f5f56d89cd41310b9347bc54b18fb4c435e69aad09b40fad0b717R589
marcospri commented 1 year ago

@pfgray apologies for pinging you here directly, I've seen some recent commits from you around this.

I was looking around and I reckon that due to:

https://github.com/instructure/canvas-lms/blob/02b3497a9bdb920fe51d1ea1e93b80c4a83b3470/ui/shared/select-content-dialog/jquery/select_content_dialog.tsx#L255

data['item[title]'] it's always truthy (defaulting to the tool's name) around:

https://github.com/instructure/canvas-lms/commit/58a743fe31bce8142e9c32d52354582aaccf24f3#diff-7944a1f02e4f5f56d89cd41310b9347bc54b18fb4c435e69aad09b40fad0b717R587

so for our tool, which never provides a title on the deep linking response, this results in getting the assignment's title changed to the tool's name.

nairiboo commented 1 year ago

@marcospri Had an update from Canvas support:

This is happening due to the enabled feature flag “LTI Deep Linking Line Items on Assignment create/edit page.” being enabled, which resulted in edits to line items that tools did not expect to happen. We released that feature previously and rolled it back as a courtesy to tools which communicated with us that they needed time to implement a fix.

This feature can be turned off via the “LTI Deep Linking Line Items on Assignment create/edit page.” in the account settings. The current plan is for it to go live in prod with our August release, so our Partners have time to fix the tool’s behavior, but this is ultimately intended behavior - if the tool is sending us a value, we will assume that they want us to use that value. They should not send us values they do not want used.

The list of impacted fields is: assignment field -> content item field points possible -> lineItem.scoreMaximum (only in 1.3) name -> lineItem.label (only in 1.3), title description -> text due dates will be impacted in the future, but has not been updated yet.

marcospri commented 1 year ago

Thanks @nairiboo

Regarding:

if the tool is sending us a value, we will assume that they want us to use that value. They should not send us values they do not want to be used.

I reckon the issue we are having is the opposite, we are not sending a value and Canvas has changed the behavior for that.

nairiboo commented 1 year ago

FYI I have asked Canvas support re above to understand if this is a bug

marcospri commented 1 year ago

This was fixed by https://github.com/instructure/canvas-lms/commit/7fdf1fba7f485c2b4cf5c4ef97cec2c403666394