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

no export / import from xml for specific feedback or hint files

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.1.2
    • Fix Version/s: 2.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1) Ideally: create one of each type of question, putting a different image into the HTML of each field that uses the HTML editor.

      (You may feel that this is overkill. An alternative would be to do this with one match, multichoice and numerical question.)

      2) Export the questions as Moodle XML format.

      3) Import into a new course.

      4) Verify the images were correctly transferred.

      5) Run all the unit tests in /question.

      Show
      1) Ideally: create one of each type of question, putting a different image into the HTML of each field that uses the HTML editor. (You may feel that this is overkill. An alternative would be to do this with one match, multichoice and numerical question.) 2) Export the questions as Moodle XML format. 3) Import into a new course. 4) Verify the images were correctly transferred. 5) Run all the unit tests in /question.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      The Moodle xml format import / export code is not importing / exporting files for correct feedback / partially correct / incorrect or for hints.

        Gliffy Diagrams

          Issue Links

            Activity

            jamiesensei Jamie Pratt created issue -
            jamiesensei Jamie Pratt made changes -
            Field Original Value New Value
            Link This issue will be resolved by MDL-29059 [ MDL-29059 ]
            Hide
            timhunt Tim Hunt added a comment -

            What makes you think that? Looking at the code, the code to handle files in hints and combined feedback is there, and I am pretty sure I remember testing it.

            Show
            timhunt Tim Hunt added a comment - What makes you think that? Looking at the code, the code to handle files in hints and combined feedback is there, and I am pretty sure I remember testing it.
            Hide
            jamiesensei Jamie Pratt added a comment -

            Hi,

            During testing the ddimagetoimage question type I exported a file with images embedded in all html editors. When I imported the question I found that the images in the three specific feedback elements and the hints did not display, instead there was a broken link.

            Looking at the code I don't think there is code there to export files embedded in these html editors.

            I do see the code for import is there.

            Jamie

            Show
            jamiesensei Jamie Pratt added a comment - Hi, During testing the ddimagetoimage question type I exported a file with images embedded in all html editors. When I imported the question I found that the images in the three specific feedback elements and the hints did not display, instead there was a broken link. Looking at the code I don't think there is code there to export files embedded in these html editors. I do see the code for import is there. Jamie
            Hide
            timhunt Tim Hunt added a comment -

            OK. Obviously I did not test this properly.

            Show
            timhunt Tim Hunt added a comment - OK. Obviously I did not test this properly.
            timhunt Tim Hunt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Priority Minor [ 4 ] Critical [ 2 ]
            Labels triaged
            Affects Version/s 2.1.1 [ 10750 ]
            Affects Version/s 2.0.4 [ 10652 ]
            Affects Version/s 2.2 [ 10656 ]
            Hide
            timhunt Tim Hunt added a comment -

            Jamie, I think this does it. Sadly, it cannot work without an API change to pass through some more information (contextid and questionid) so I think this has to be HEAD only.

            Please could you take a look and see if you think this works.

            Show
            timhunt Tim Hunt added a comment - Jamie, I think this does it. Sadly, it cannot work without an API change to pass through some more information (contextid and questionid) so I think this has to be HEAD only. Please could you take a look and see if you think this works.
            timhunt Tim Hunt made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            Pull Master Diff URL https://github.com/timhunt/moodle/compare/MDL-29739...MDL-29058
            Pull Master Branch MDL-29058
            Pull from Repository git://github.com/timhunt/moodle.git
            Peer reviewer jamiesensei
            Hide
            jamiesensei Jamie Pratt added a comment -

            Looks good Tim. I tested export and import with image files in the html editor and it seems to work great now.

            Show
            jamiesensei Jamie Pratt added a comment - Looks good Tim. I tested export and import with image files in the html editor and it seems to work great now.
            timhunt Tim Hunt made changes -
            Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Jamie. Submitting for integration.

            Dear integrators. I am just submitting this for 2.2, since it requires a small API change. We could, however, consider adding this to 2.1. That would fix core qtypes.

            It would meed that third-party qtypes generated Missing function argument warnings on export, but the export would still work other wise (well third-party qtypes would still fail to export files, since the missing contextid would be treated as 0) but they would be no more broken than now, and third-party qtype authors would be able to fix the export of their questions, which is not possible now. If we do this, we would need to update question/type/upgrade.txt.

            Note too that this fix follows on from the changes in MDL-29739, so please integrate that other fix first.

            Show
            timhunt Tim Hunt added a comment - Thanks Jamie. Submitting for integration. Dear integrators. I am just submitting this for 2.2, since it requires a small API change. We could, however, consider adding this to 2.1. That would fix core qtypes. It would meed that third-party qtypes generated Missing function argument warnings on export, but the export would still work other wise (well third-party qtypes would still fail to export files, since the missing contextid would be treated as 0) but they would be no more broken than now, and third-party qtype authors would be able to fix the export of their questions, which is not possible now. If we do this, we would need to update question/type/upgrade.txt. Note too that this fix follows on from the changes in MDL-29739 , so please integrate that other fix first.
            timhunt Tim Hunt made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Fix Version/s 2.2 [ 10656 ]
            Fix Version/s STABLE backlog [ 10463 ]
            Testing Instructions 1) Ideally: create one of each type of question, putting a different image into the HTML of each field that uses the HTML editor.

            (You may feel that this is overkill. An alternative would be to do this with one match, multichoice and numerical question.)

            2) Export the questions as Moodle XML format.

            3) Import into a new course.

            4) Verify the images were correctly transferred.

            5) Run all the unit tests in /question.
            Hide
            jamiesensei Jamie Pratt added a comment -

            Just a note to say that exports will fail if the user has display error messages on and the notices are displayed. This breaks the download as then headers cannot be output after the notices.

            Show
            jamiesensei Jamie Pratt added a comment - Just a note to say that exports will fail if the user has display error messages on and the notices are displayed. This breaks the download as then headers cannot be output after the notices.
            Hide
            timhunt Tim Hunt added a comment -

            Oh bum! I had forgotten that stupid change the the Moodle 2.x files API forced on us.

            Show
            timhunt Tim Hunt added a comment - Oh bum! I had forgotten that stupid change the the Moodle 2.x files API forced on us.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            nebgor Aparup Banerjee made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator nebgor
            Hide
            nebgor Aparup Banerjee added a comment -

            This has been integrated into master only.

            Tim,
            I too think that adding it to 2.1 would be a good idea too for the overall improvement if not for the fact that it doesn't really fix but enables a fix (but really that is IMHO). I can't assume this is desired by everyone (zero votes too ).

            I've stuck with putting this into master only, those who really need to fix (therefore break it first) can always pull this patch over to their 21 or upgrade or

            If anyone still feels strongly about backporting this we can always raise another issue as needed.

            Show
            nebgor Aparup Banerjee added a comment - This has been integrated into master only. Tim, I too think that adding it to 2.1 would be a good idea too for the overall improvement if not for the fact that it doesn't really fix but enables a fix (but really that is IMHO). I can't assume this is desired by everyone (zero votes too ). I've stuck with putting this into master only, those who really need to fix (therefore break it first) can always pull this patch over to their 21 or upgrade or If anyone still feels strongly about backporting this we can always raise another issue as needed.
            nebgor Aparup Banerjee made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.1.2 [ 10851 ]
            Affects Version/s 2.0.5 [ 10950 ]
            Affects Version/s 2.0.4 [ 10652 ]
            Affects Version/s 2.1.1 [ 10750 ]
            ankit_frenz Ankit Agarwal made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester ankit_frenz
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            all things working fine!
            Test passed
            Thanks!

            Show
            ankit_frenz Ankit Agarwal added a comment - all things working fine! Test passed Thanks!
            ankit_frenz Ankit Agarwal made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            timhunt Tim Hunt added a comment -

            The problem is that this patch makes significant API changes, which will break all third-party question types. We can't do that on a stable branch.

            Show
            timhunt Tim Hunt added a comment - The problem is that this patch makes significant API changes, which will break all third-party question types. We can't do that on a stable branch.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 15/Nov/11

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/Dec/11