hypothesis / lms

LTI app for integrating with learning management systems
BSD 2-Clause "Simplified" License
46 stars 14 forks source link

Wrong/unexpected app instance can be used in SpeedGrader LTI launches #915

Closed robertknight closed 1 year ago

robertknight commented 5 years ago

When Canvas launches our app inside SpeedGrader, it tries to pick the LTI tool install which matches the LTI Launch URL that we submitted as part of the submission to the LTI Outcomes Service when the student launched the assignment.

If there are multiple installations of the LTI tool in the LMS, Canvas prefers installations that match the course, with a fallback to site-wide installs. The external tool associated with the assignment that is being graded is not taken into account.

As a result, Canvas can end up using a different install ("application instance" in the terminology of the LMS app) than the one used when the student did the assignment. This means that per-install configuration parameters (eg. whether provisioning is enabled) and Canvas API keys may be different. This can create confusing problems such as provisioning being enabled when the student completes the assignment, but not when the teacher grades the assignment due to a non-provisioning-enabled install having been selected for use in SpeedGrader.

I filed an issue upstream at https://community.canvaslms.com/message/154504.

seanh commented 4 years ago

https://community.canvaslms.com/message/154504 has few views and no responses. I've tried creating https://github.com/instructure/canvas-lms/issues/1545 instead

klemay commented 4 years ago

After talking with @seanh I've removed this from the sprint board, and put it back in "To Review" on the bug backlog.

From Sean:

Instructure's point of view seems to be that Canvas shows a warning if you try to install the same LTI tool twice in one course, and that's good enough. This implies that other LTI apps may have the same crazy bugs as us in SpeedGrader when you do this, although they don't say that. I wonder how often users install our app twice in the same course? We would have to do a fair bit of work and do something pretty odd to work around the bug. I wonder what the priority of it should be? Maybe keep it open and consider fixing it later, like after sections?

Given this is something that (a) requires a significant amount of work and (b) can be managed “culturally” until it’s fixed (i.e., telling our partners not to have multiple installs, and making “check for multiple installs” part of our troubleshooting + documentation), I agree with Sean's suggestion that this is something we can fix later.

klemay commented 3 years ago

We're working our way through our bug backlog really quickly, so I wanted to share an example of how we see this show up most often (not necessarily moving this up in the queue, just making sure the user story is in place well ahead of when it's time to work on this):

I am an instructor at Annotation University. My school uses Canvas and allows us to install LTI apps at the course level. I am interested in trying out Hypothesis, but my school doesn't currently have an agreement with Hypothesis. I follow these instructions to install Canvas from the Edu App center for my course.

Hypothesis notices that I have installed and have been using the app, and reaches out to form a business relationship with my school. Midway through the semester, Annotation University signs an agreement with Hypothesis and follows these instructions to install Canvas instance-wide.

Now, there are two Hypothesis installs available to my course:

  • The course-level installation I created
  • The instance-wide installation done by my school's IT admin

Because of this bug, when I launch the Speedgrader, Canvas will intermittently launch the instance-wide version of the app. Since my students have annotated in the course-level instance, I won't see their annotations in Speedgrader and will think their annotations are missing. I become very worried and reach out to the Hypothesis support team.

This is one of the biggest sources of support tickets - if we could resolve this, or even show a warning/explanation teachers, it would relieve a big part of our support team's burden.

seanh commented 3 years ago

@klemay We should pass this information on to Instructure. In the ticket that we opened with them about this they said:

Currently in Canvas, if a user attempts to install a duplicate tool in a context, a warning modal is presented to them. This warning allows them to proceed with the installation, or cancel the installation ... One change we've been considering is having a third, default action available in that warning modal: "Replace existing tool." We feel like this would help to reduce the number of accidental duplicate installations ... It's unlikely that we will make changes to support duplicate tools in the same context in the near future. While it would not technically break our LTI specification compliance, it would add a new layer of canvas-specific behavior for tool providers to worry about (which we typically avoid when possible).

And they said "Let us know if you have any thoughts."

It would be good if we could make it clear to them how big of a problem this is for LTI app developers, as they don't seem to understand.

I don't think there's a reasonable way for us to fix this on our end, without Instructure fixing it on their end. We'd have to do a difficult and insecure hack

jon-betts commented 3 years ago

A quick note to say I think that's a wontfix on their side. They've closed the ticket.

marcospri commented 1 year ago

Closing this.

This seems to be a "this is how it's supposed to work" from Canvas PoV. If we wanted to go around this we should hint which AI to use on the params we submit to speed grader.