Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-45982

LTI doesn't handle non error messages from a provider

    Details

    • Testing Instructions:
      Hide

      1. Setup / reuse an external tool.
      2. Add to a course using the default details setup under the external tool configuration.
      3. Access the activity
      4. Capture the url for mod/lti/return.php
      5. Append or change lti_errormsg to lti_msg=test
      6. Verify once accessing the return url it displays on screen.

      Show
      1. Setup / reuse an external tool. 2. Add to a course using the default details setup under the external tool configuration. 3. Access the activity 4. Capture the url for mod/lti/return.php 5. Append or change lti_errormsg to lti_msg=test 6. Verify once accessing the return url it displays on screen.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
      mdl45982-moodle27
    • Pull Master Branch:
      mdl45982-master

      Description

      LTI doesn't handle non error messages from a provider like:

      mod/lti/return.php?course=2861&launch_container=4&instanceid=48&lti_msg=Instructor+has+not+completed+the+configuration+of+this+link.

      This is from "Adaptive eLearning" as a student when the associated task hasn't been setup.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            cibot CiBoT added a comment -

            Results for MDL-45982

            Show
            cibot CiBoT added a comment - Results for MDL-45982 Remote repository: https://github.com/tlock/moodle.git Remote branch mdl45982-moodle25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3986 Warning: The mdl45982-moodle25 branch at https://github.com/tlock/moodle.git has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3986/artifact/work/smurf.html Remote branch mdl45982-moodle26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3987 Warning: The mdl45982-moodle26 branch at https://github.com/tlock/moodle.git has not been rebased recently (>20 days ago). Error: The mdl45982-moodle26 branch at https://github.com/tlock/moodle.git does not apply clean to MOODLE_26_STABLE Remote branch mdl45982-moodle27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3988 Error: The mdl45982-moodle27 branch at https://github.com/tlock/moodle.git does not apply clean to MOODLE_27_STABLE Remote branch mdl45982-master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3989 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3989/artifact/work/smurf.html
            Hide
            tlock Tim Lock added a comment -

            Rebased all branches.

            Show
            tlock Tim Lock added a comment - Rebased all branches.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-45982 Remote repository: https://github.com/tlock/moodle.git Remote branch mdl45982-moodle25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3990 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3990/artifact/work/smurf.html Remote branch mdl45982-moodle26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3991 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3991/artifact/work/smurf.html Remote branch mdl45982-moodle27 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3992 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3992/artifact/work/smurf.html Remote branch mdl45982-master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3993 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3993/artifact/work/smurf.html
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Tim,

            Thanks for the patch - it makes sense to me. I don't suppose you have a real life LTI resource that we could use for testing this?

            Normally I would say we should be using s() rather than htmlspecialchars, but I see its the existing style.

            Pinging Mark Nielsen in case he has anything to add/concerns

            Show
            poltawski Dan Poltawski added a comment - Hi Tim, Thanks for the patch - it makes sense to me. I don't suppose you have a real life LTI resource that we could use for testing this? Normally I would say we should be using s() rather than htmlspecialchars, but I see its the existing style. Pinging Mark Nielsen in case he has anything to add/concerns
            Hide
            bushido Mark Nielsen added a comment -

            Looks fine to me.

            Show
            bushido Mark Nielsen added a comment - Looks fine to me.
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Tim - this has been integrated now.

            If you happen to have a tool to test this with and could share it here that would be great - it would certainly make the testers life easier.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Tim - this has been integrated now. If you happen to have a tool to test this with and could share it here that would be great - it would certainly make the testers life easier.
            Hide
            dmonllao David Monllaó added a comment -

            Hi, I've been trying to find a proper external tool to test this for 1h more or less, I will wait until Tim Lock provides an example to continue testing this.

            Show
            dmonllao David Monllaó added a comment - Hi, I've been trying to find a proper external tool to test this for 1h more or less, I will wait until Tim Lock provides an example to continue testing this.
            Hide
            tlock Tim Lock added a comment -

            Hi All,

            Our clients all use paid systems for LTI, so I don't have examples.

            Show
            tlock Tim Lock added a comment - Hi All, Our clients all use paid systems for LTI, so I don't have examples.
            Hide
            dmonllao David Monllaó added a comment -

            Ok, thanks Tim, np; if you have a sec, can you please comment about #4? I've added the lti_msg in $returnurlparams but the string is not shown anywhere, I'm guessing it depends on the package; any tips to find package to test this change? I can't with the ones I've used.

            Show
            dmonllao David Monllaó added a comment - Ok, thanks Tim, np; if you have a sec, can you please comment about #4? I've added the lti_msg in $returnurlparams but the string is not shown anywhere, I'm guessing it depends on the package; any tips to find package to test this change? I can't with the ones I've used.
            Hide
            andyjdavis Andrew Davis added a comment -

            Works as described. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Works as described. Passing.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Just in time for incoming releases, well done. And big thanks!

            We must plan for freedom,
            and not only for security,
            if for no other reason than that
            only freedom can make security secure

            –  Karl Popper

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Just in time for incoming releases, well done. And big thanks! We must plan for freedom, and not only for security, if for no other reason than that only freedom can make security secure –  Karl Popper

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jul/14