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

HTML editor in the essay question type completely broken

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6
    • Component/s: Libraries, Questions
    • Labels:
    • Testing Instructions:
      Hide

      After applying the "patch" and purging all caches:

      1. Create a quiz.
      2. Add an essay question with option "Response format: HTML editor", and which includes an image.
      3. While editing the essay question, verify that the "manage files" button appears.
      4. Click the "manage files" button, and verify that the displayed dialog shows the included image.
      5. Save the question.
      6. Preview the question, and verify that the HTML editor shows up.
      7. Verify that the "manage files" button is not included.
      8. Modify the essay question so that the "Response format" option is now "HTML editor with file picker".
      9. Preview the question, and verify that the "manage files" button is included.
      Show
      After applying the "patch" and purging all caches: Create a quiz. Add an essay question with option "Response format: HTML editor", and which includes an image. While editing the essay question, verify that the "manage files" button appears. Click the "manage files" button, and verify that the displayed dialog shows the included image. Save the question. Preview the question, and verify that the HTML editor shows up. Verify that the "manage files" button is not included. Modify the essay question so that the "Response format" option is now "HTML editor with file picker". Preview the question, and verify that the "manage files" button is included.
    • Workaround:
      Hide

      Disable the tinymce_managefiles plugin.

      Show
      Disable the tinymce_managefiles plugin.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
      git@github.com:ktemkin/moodle.git
    • Pull Master Branch:
      MDL-41034-essay-broken

      Description

      Steps to reproduce:

      1. Create a quiz.
      2. Add an essay question with option "Response format: HTML editor with file picker"
      3. Either: preivew the question, preview the quiz, or attempt the quiz as a student.

      Expected result: You get a HTML editor where you can type your answer.

      Acutal result: You get a blank space where the editor shoudl be.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            Michael, please can you arrange for whoever at Moodle HQ broke this recently to fix it ASAP. It is blocking both MDL-39980 and MDL-39756.

            Show
            timhunt Tim Hunt added a comment - Michael, please can you arrange for whoever at Moodle HQ broke this recently to fix it ASAP. It is blocking both MDL-39980 and MDL-39756 .
            Hide
            timhunt Tim Hunt added a comment -

            Error in Firebug console:

            TypeError: M.editor_tinymce.filepicker_options[ed.id].image is undefined

            managefiles.itemid = M.editor_tinymce.filepicker_options[ed.id].image.itemid;

            editor_plugin.js (line 97)

            Show
            timhunt Tim Hunt added a comment - Error in Firebug console: TypeError: M.editor_tinymce.filepicker_options [ed.id] .image is undefined managefiles.itemid = M.editor_tinymce.filepicker_options [ed.id] .image.itemid; editor_plugin.js (line 97)
            Hide
            ktemkin Kyle Temkin added a comment -

            The problem seems to stem from the following commit:

            MDL-28019 Added plugin tinymce_managefiles
            9e88661e58208848e5c7c0eab64223725be14269

            Found using git bisect; I've confirmed that disabling that plugin allows the HTML editor in essays to work again.

            Show
            ktemkin Kyle Temkin added a comment - The problem seems to stem from the following commit: MDL-28019 Added plugin tinymce_managefiles 9e88661e58208848e5c7c0eab64223725be14269 Found using git bisect ; I've confirmed that disabling that plugin allows the HTML editor in essays to work again.
            Hide
            timhunt Tim Hunt added a comment -

            Ah! Great work. Thanks.

            Show
            timhunt Tim Hunt added a comment - Ah! Great work. Thanks.
            Hide
            timhunt Tim Hunt added a comment -

            Adding folks from MDL-28019 as watchers here.

            Show
            timhunt Tim Hunt added a comment - Adding folks from MDL-28019 as watchers here.
            Hide
            ktemkin Kyle Temkin added a comment -

            After taking a look at the relevant code in a JS debugger, I now have a pretty good idea what the cause is. When an editor is created using use_editor, any relevant file area information is specified as part of the file-picker options via an optional third argument, $fpoptions. This array argument can be used to independently configure the link button, the moodleimage plugin, and moodlemedia plugins.

            The managefiles plugin assumes that options for the moodleimage plugin will be present whenever any file-picker options are specified:

            if (!managefiles.itemid && M.editor_tinymce.filepicker_options && M.editor_tinymce.filepicker_options[ed.id]) {

            Instead; it should explicitly check to see if image options were specified, thus checking the full path to the data they want to access.

            if (!managefiles.itemid && M.editor_tinymce.filepicker_options && M.editor_tinymce.filepicker_options[ed.id] && M.editor_tinymce.filepicker_options[ed.id].image) {

            (This also seems like it might be an indicator of a potential problem with the managefiles plugin. It looks like it assumes that there's only ever one itemid per editor-- but, if I'm understanding the code correctly, one can actually specify separate areas for the editor itself/links, the media plugin, and the images plugin, though all three are set to the same itemid when using Moodleforms.)

            I've created a git branch with the simple fix above; I'll look and see if/how I can unit test this. If the fix is that simple, I'd gladly assign the issue to myself.

            Show
            ktemkin Kyle Temkin added a comment - After taking a look at the relevant code in a JS debugger, I now have a pretty good idea what the cause is. When an editor is created using use_editor , any relevant file area information is specified as part of the file-picker options via an optional third argument, $fpoptions . This array argument can be used to independently configure the link button, the moodleimage plugin, and moodlemedia plugins. The managefiles plugin assumes that options for the moodleimage plugin will be present whenever any file-picker options are specified: if (!managefiles.itemid && M.editor_tinymce.filepicker_options && M.editor_tinymce.filepicker_options[ed.id]) { Instead; it should explicitly check to see if image options were specified, thus checking the full path to the data they want to access. if (!managefiles.itemid && M.editor_tinymce.filepicker_options && M.editor_tinymce.filepicker_options[ed.id] && M.editor_tinymce.filepicker_options[ed.id].image) { (This also seems like it might be an indicator of a potential problem with the managefiles plugin. It looks like it assumes that there's only ever one itemid per editor-- but, if I'm understanding the code correctly, one can actually specify separate areas for the editor itself/links, the media plugin, and the images plugin, though all three are set to the same itemid when using Moodleforms.) I've created a git branch with the simple fix above; I'll look and see if/how I can unit test this. If the fix is that simple, I'd gladly assign the issue to myself.
            Hide
            ktemkin Kyle Temkin added a comment -

            It looks like the unit testing for javascript (see MDL-35595) is not as complete as I had thought. I suppose manual testing will have to suffice, for now.

            Hopefully I am not stepping on any toes by assigning this issue to myself and submitting my simple solution for peer review.

            Show
            ktemkin Kyle Temkin added a comment - It looks like the unit testing for javascript (see MDL-35595 ) is not as complete as I had thought. I suppose manual testing will have to suffice, for now. Hopefully I am not stepping on any toes by assigning this issue to myself and submitting my simple solution for peer review.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks for fixing it.

            Each editor should only have one file area. Think about how the data gets handled when the form is submitted. If the code allows them to be different, that is something of a bug, or at least it creates the potential for bugs.

            Your patch is fine, except that line of code is now very long. It might be better to line-wrap it.

            Also, the testing instructions should also test a normal Moodle form (e.g. create course) to ensure that you have not broken standard HTML editors.

            If you fix those details, I (or someone else) can submit for integration.

            Show
            timhunt Tim Hunt added a comment - Thanks for fixing it. Each editor should only have one file area. Think about how the data gets handled when the form is submitted. If the code allows them to be different, that is something of a bug, or at least it creates the potential for bugs. Your patch is fine, except that line of code is now very long. It might be better to line-wrap it. Also, the testing instructions should also test a normal Moodle form (e.g. create course) to ensure that you have not broken standard HTML editors. If you fix those details, I (or someone else) can submit for integration.
            Hide
            ktemkin Kyle Temkin added a comment -

            (In my haste to get this fixed, I forgot to break up that long line. Fixed.)

            Show
            ktemkin Kyle Temkin added a comment - (In my haste to get this fixed, I forgot to break up that long line. Fixed.)
            Hide
            timhunt Tim Hunt added a comment -

            Long line not broken up yet. Did you forget to do git push -f?

            Show
            timhunt Tim Hunt added a comment - Long line not broken up yet. Did you forget to do git push -f?
            Hide
            timhunt Tim Hunt added a comment -

            Oops! you efficiently handled checking normal forms still worked when creating the question. Sorry. I missed that.

            One other thing to add to the testing instructions: HTML editor admin settings, e.g. in clean theme settings. (Note that they don't involve files, I think.)

            Show
            timhunt Tim Hunt added a comment - Oops! you efficiently handled checking normal forms still worked when creating the question. Sorry. I missed that. One other thing to add to the testing instructions: HTML editor admin settings, e.g. in clean theme settings. (Note that they don't involve files, I think.)
            Hide
            ktemkin Kyle Temkin added a comment -

            I did push it; though I think GitHub lags a little. It shows up for me; but I re-forced the push anyway.

            The testing instructions do include a standard Moodle form (the edit questions form). Did you want me to add another?

            Show
            ktemkin Kyle Temkin added a comment - I did push it; though I think GitHub lags a little. It shows up for me; but I re-forced the push anyway. The testing instructions do include a standard Moodle form (the edit questions form). Did you want me to add another?
            Hide
            timhunt Tim Hunt added a comment -

            Hang on. When the student is answering the essay, the manage files button should appear.

            Show
            timhunt Tim Hunt added a comment - Hang on. When the student is answering the essay, the manage files button should appear.
            Hide
            ktemkin Kyle Temkin added a comment -

            Whoops! You're right; there was an error in my instructions. I just adjusted the instructions to match my actual intentions (changed step #2 to HTML editor, and added steps 8/9).

            The instructions should now be coherent.

            Show
            ktemkin Kyle Temkin added a comment - Whoops! You're right; there was an error in my instructions. I just adjusted the instructions to match my actual intentions (changed step #2 to HTML editor, and added steps 8/9). The instructions should now be coherent.
            Hide
            marina Marina Glancy added a comment -

            Thanks a lot for fixing it guys. Can I suggest to also check M.editor_tinymce.filepicker_options[ed.id].image.itemid
            I assumed everybody uses filepicker in textarea in a standard way - when filepicker_options are defined they are defined for images, media and links as well (otherwise why define filepicker?).

            But this case shows me that it is not so. Maybe there is some other module that has defined M.editor_tinymce.filepicker_options[ed.id].image but without the field itemid? (Most likely would break filepicker though).

            Show
            marina Marina Glancy added a comment - Thanks a lot for fixing it guys. Can I suggest to also check M.editor_tinymce.filepicker_options [ed.id] .image.itemid I assumed everybody uses filepicker in textarea in a standard way - when filepicker_options are defined they are defined for images, media and links as well (otherwise why define filepicker?). But this case shows me that it is not so. Maybe there is some other module that has defined M.editor_tinymce.filepicker_options [ed.id] .image but without the field itemid? (Most likely would break filepicker though).
            Hide
            timhunt Tim Hunt added a comment -

            Why are we passing in three separate itemids to the same editor? There can only be one file itemid (draftareaid) for the whole editor. How hard is it to change the code so all plugins use M.editor_tinymce.filepicker_options[ed.id].itemid? Or should we not risk doing that change here?

            Show
            timhunt Tim Hunt added a comment - Why are we passing in three separate itemids to the same editor? There can only be one file itemid (draftareaid) for the whole editor. How hard is it to change the code so all plugins use M.editor_tinymce.filepicker_options [ed.id] .itemid? Or should we not risk doing that change here?
            Hide
            timhunt Tim Hunt added a comment -

            I am taking in unilateral decision (since all the integrators and Marina are asleep) to submit this quick fix for integration this week, so we can carry on with MDL-39980 and MDL-39756.

            I have created MDL-41046 to sort this out properly when we are not in a rush to fix a regression.

            Show
            timhunt Tim Hunt added a comment - I am taking in unilateral decision (since all the integrators and Marina are asleep) to submit this quick fix for integration this week, so we can carry on with MDL-39980 and MDL-39756 . I have created MDL-41046 to sort this out properly when we are not in a rush to fix a regression.
            Hide
            marina Marina Glancy added a comment -

            Thanks, it was integrated in master. I added commit fixing trailing whitespaces and adding a check for non-empty itemid.

            Show
            marina Marina Glancy added a comment - Thanks, it was integrated in master. I added commit fixing trailing whitespaces and adding a check for non-empty itemid.
            Hide
            phalacee Jason Fowler added a comment -

            Thanks Kyle, works fine!

            Show
            phalacee Jason Fowler added a comment - Thanks Kyle, works fine!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better!

            "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar."
            ~ Professor Farnsworth

            Show
            samhemelryk Sam Hemelryk added a comment - Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better! "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar." ~ Professor Farnsworth

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13