Fix to Assignment LTI application URL #812
Conversation
|
@amulgarg I'm going to go ahead and get it started through the process of going through our test suite and review. Could you submit a signed copy of our CLA to opensource@instructure.com? The CLA is found here: https://github.com/instructure/canvas-lms/wiki/ica.pdf |
|
Also could you put the test plan in the actual commit message? That will make it much easier on our end. Additionally, it'd be great if you could cut down the line length of the commit message lines. Try and keep them less than 60 characters per line. |
The matches_url? method should work as - if match_queries_exactly=true, this method should return true only if the URLs are exactly equal. But suppose you have three apps (on the same domain and host) with URLs - www.example.com/app1, www.example.com/app2 and www.example.com/app3. What I saw happening is after adding the app3, the first two stopped working because they were trying to reach the URL of the third app. This was happening because in the method matches_url? after checking if the URLs match exactly it returns true if they indeed match, but proceeds to check for the hostnames if they don't.This happened because of the sorting logic for context_tools (app/models/context_external_tool.rb) - `sorted_external_tools = all_external_tools.sort_by { |t| [contexts.index { |c| c.id == t.context_id && c.class.name == t.context_type }, t.precedence, t.id == preferred_tool_id ? CanvasSort::First : CanvasSort::Last] }` Test plan: 1.Add LTI app with a domain to a course [App1] 2.Add another LTI app with same domain to a course [App2] 3.Add yet another LTI app with same domain [App3] 4.Add these three apps to three assignments [A1,A2,A3] 5.Launch all apps from their Assignments (submission set to external tool) 6.Remove App3 7.Add another LTI app with same domain [App4] 8.Add this app to a new assignment [A4] 9.Verify if A1,A2,A4 launch the correct apps.
|
@claydiffrient I've added the test plan in the commit message and have cut down on the length of commit message lines. |
|
Hey @claydiffrient, any update on this pull request? |
|
@amulgarg It looks like this breaks one of our builds. I'll have the appropriate team look at it and offer some suggestions. |
|
@amulgarg After having the responsible team look at this and the failures that occurred with our internal build, we've come to the conclusion that this change won't work. Our build runs against many different tool launches and this commit breaks several of those launches. Any change in this portion of the code has to be really sensitive to other tools and how they are using the system. Thanks for the PR and we'd love to see more from you in the future, but I'm afraid this one won't work for us at this time. |
The matches_url? method should work as - if match_queries_exactly=true, this method should return true only if the URLs are exactly equal.
But suppose you have three apps (on the same domain and host) with URLs -
www.example.com/app1, www.example.com/app2 and www.example.com/app3. What I saw happening is after adding the app3, the first two stopped working because they were trying to reach the URL of the third app. This was happening because in the method matches_url? after checking if the URLs match exactly, it returns true if they indeed match, but proceeds to check for the hostnames if they don't.
This happened because of the sorting logic for context_tools (app/models/context_external_tool.rb) -
sorted_external_tools = all_external_tools.sort_by { |t| [contexts.index { |c| c.id == t.context_id && c.class.name == t.context_type }, t.precedence, t.id == preferred_tool_id ? CanvasSort::First : CanvasSort::Last] }Test plan :