Moodle
  1. Moodle
  2. MDL-36934

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      46473

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Andrew Nicols added a comment -

          Thanks for that Tim - that should be handled now.

          Andrew

          Show
          Andrew Nicols added a comment - Thanks for that Tim - that should be handled now. Andrew
          Hide
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          Just in time for Moodle 2.4.0 release, thanks!

          Closing, ciao

          Show
          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: