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

'Move this discussion to...' feature broken in Firefox

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide
      • Add several standard forums to a course
      • Open one and open a new discussion thread
      • Open the thread
      • Test the 'Move this discussion to ...' dropdown
        • Confirm that it never automatically submits
      Show
      Add several standard forums to a course Open one and open a new discussion thread Open the thread Test the 'Move this discussion to ...' dropdown Confirm that it never automatically submits
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36934-master

      Description

      When attempting to move a post to another forum using the 'Move this discussion to...' dropdown menu, the following error message is displayed (before having had chance to click the Move button):

      Error occured

      More information about this error
      Debug info:
      Error code: error
      Stack trace:

      line 467 of /lib/setuplib.php: moodle_exception thrown
      line 40 of /course/jumpto.php: call to print_error()

      I have reproduced the problem on http://qa.moodle.net and on http://moodle.org but only using Firefox. The feature worked fine in Chrome.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              andyjdavis Andrew Davis added a comment -

              print_error() is called if jumpto.php is supplied a URL that doesnt start with a forward slash. For example on my machine the URL looks like /mod/forum/discuss.php?d=4&move=11&sesskey=qQjBa7uqLm. Its working fine on my development machine using Firefox. I'm looking at qa.moodle.net now.

              Show
              andyjdavis Andrew Davis added a comment - print_error() is called if jumpto.php is supplied a URL that doesnt start with a forward slash. For example on my machine the URL looks like /mod/forum/discuss.php?d=4&move=11&sesskey=qQjBa7uqLm. Its working fine on my development machine using Firefox. I'm looking at qa.moodle.net now.
              Hide
              andyjdavis Andrew Davis added a comment -

              Something on qa.moodle.net is causing that form to be submitted before you've actually selected a forum to move the discussion to.

              Show
              andyjdavis Andrew Davis added a comment - Something on qa.moodle.net is causing that form to be submitted before you've actually selected a forum to move the discussion to.
              Hide
              andyjdavis Andrew Davis added a comment -

              I'm able to see this in the latest integration on my machine but not in current master ie this is a shiny new bug.

              Show
              andyjdavis Andrew Davis added a comment - I'm able to see this in the latest integration on my machine but not in current master ie this is a shiny new bug.
              Hide
              andyjdavis Andrew Davis added a comment -

              Linking to the issue that most likely introduced this problem.

              Note: Im using Firefox 17.0 on Ubuntu 11.04 just in case that's important.

              Show
              andyjdavis Andrew Davis added a comment - Linking to the issue that most likely introduced this problem. Note: Im using Firefox 17.0 on Ubuntu 11.04 just in case that's important.
              Hide
              andyjdavis Andrew Davis added a comment - - edited

              The problem appears to be this in lib/yui/formautosubmit/formautosubit.js

              if ((nothing===false || select.get('value') != nothing) && startindex != select.get('selectedIndex') && currentindex != previousindex) {
                  return select;
              }

              The JS is deciding that the form has changed as soon as you initially click on the drop down. Here are the values of the variables involved.

              nothing==undefined
              value==
              startindex==undefined
              selectedIndex==0
              currentindex==0
              previousindex==undefined

              Here is how I got the values just in case that's important.

              s = "nothing=="+nothing+"\n";
              s += "value=="+select.get('value')+"\n";
              s += "startindex=="+startindex+"\n";
              s += "selectedIndex=="+select.get('selectedIndex')+"\n";
              s += "currentindex=="+currentindex+"\n";
              s += "previousindex=="+previousindex+"\n";
              alert(s);

              Show
              andyjdavis Andrew Davis added a comment - - edited The problem appears to be this in lib/yui/formautosubmit/formautosubit.js if ((nothing===false || select.get('value') != nothing) && startindex != select.get('selectedIndex') && currentindex != previousindex) { return select; } The JS is deciding that the form has changed as soon as you initially click on the drop down. Here are the values of the variables involved. nothing==undefined value== startindex==undefined selectedIndex==0 currentindex==0 previousindex==undefined Here is how I got the values just in case that's important. s = "nothing=="+nothing+"\n"; s += "value=="+select.get('value')+"\n"; s += "startindex=="+startindex+"\n"; s += "selectedIndex=="+select.get('selectedIndex')+"\n"; s += "currentindex=="+currentindex+"\n"; s += "previousindex=="+previousindex+"\n"; alert(s);
              Hide
              tsala Helen Foster added a comment -

              Thanks Andrew for investigating this issue so promptly!

              I've just tried moving a discussion using Chrome and found that although the discussion was moved without any error message, it was done without the Move button needing to be pressed.

              Show
              tsala Helen Foster added a comment - Thanks Andrew for investigating this issue so promptly! I've just tried moving a discussion using Chrome and found that although the discussion was moved without any error message, it was done without the Move button needing to be pressed.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks for looking into this. On deeper inspection, it appears to be a mistake (on my part) with adding the 'autosubmit' class to the dropdown when I shouldn't have done so.

              The new formautosubmit system uses a class ('autosubmit') and delegates the various change/click/key events to select.autosubmit across document.body.

              In the case where there's a 'Move' button, we shouldn't be adding the class. The reason that it appears to be the JS rather than the PHP is that the nothing and start values aren't recorded because I was partially respecting the movebutton value.

              Patch incoming...

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks for looking into this. On deeper inspection, it appears to be a mistake (on my part) with adding the 'autosubmit' class to the dropdown when I shouldn't have done so. The new formautosubmit system uses a class ('autosubmit') and delegates the various change/click/key events to select.autosubmit across document.body. In the case where there's a 'Move' button, we shouldn't be adding the class. The reason that it appears to be the JS rather than the PHP is that the nothing and start values aren't recorded because I was partially respecting the movebutton value. Patch incoming...
              Hide
              timhunt Tim Hunt added a comment -

              Your code will add an empty class="" attribute if neither condition is true, won't it? Probably worth tidying that up.

              Show
              timhunt Tim Hunt added a comment - Your code will add an empty class="" attribute if neither condition is true, won't it? Probably worth tidying that up.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks for that Tim - that should be handled now.

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks for that Tim - that should be handled now. Andrew
              Hide
              andyjdavis Andrew Davis added a comment -

              [Y] Syntax
              [NA] Output
              [Y] Whitespace
              [NA] Language
              [NA] Databases
              [?] Testing
              [Y] Security
              [NA] Documentation
              [Y] Git
              [Y] Sanity check

              Looks good. The only minor suggestion I have is to expand the testing instructions to include testing somewhere where automatic submission should happen so we're testing that it doesnt happen where it shouldn't and that it does happen where it should.

              Show
              andyjdavis Andrew Davis added a comment - [Y] Syntax [NA] Output [Y] Whitespace [NA] Language [NA] Databases [?] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check Looks good. The only minor suggestion I have is to expand the testing instructions to include testing somewhere where automatic submission should happen so we're testing that it doesnt happen where it shouldn't and that it does happen where it should.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              I was not able to reproduce the error without the patch applied (only noticed the autosubmit was happening incorrectly).

              And the patch seems to do it work by preventing such wrong autosubmit, so integrated, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - I was not able to reproduce the error without the patch applied (only noticed the autosubmit was happening incorrectly). And the patch seems to do it work by preventing such wrong autosubmit, so integrated, thanks!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Verified now "move discussion" does not autosubmit anymore. And the "move" button does its work.

              Also quick tested (FF, Safari) that other places (add block, add activity, rating... continue working ok.

              So passing, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Verified now "move discussion" does not autosubmit anymore. And the "move" button does its work. Also quick tested (FF, Safari) that other places (add block, add activity, rating... continue working ok. So passing, thanks!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Just in time for Moodle 2.4.0 release, thanks!

              Closing, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Just in time for Moodle 2.4.0 release, thanks! Closing, ciao

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Dec/12