Moodle
  1. Moodle
  2. MDL-29058

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

    Details

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

      Description

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

        Issue Links

          Activity

          Jamie Pratt created issue -
          Jamie Pratt made changes -
          Field Original Value New Value
          Link This issue will be resolved by MDL-29059 [ MDL-29059 ]
          Hide
          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
          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
          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
          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
          Tim Hunt added a comment -

          OK. Obviously I did not test this properly.

          Show
          Tim Hunt added a comment - OK. Obviously I did not test this properly.
          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
          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
          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.
          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
          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
          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.
          Tim Hunt made changes -
          Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
          Hide
          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
          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.
          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
          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
          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
          Tim Hunt added a comment -

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

          Show
          Tim Hunt added a comment - Oh bum! I had forgotten that stupid change the the Moodle 2.x files API forced on us.
          Hide
          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
          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
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Aparup Banerjee made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator nebgor
          Hide
          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
          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.
          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 Agarwal made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Tester ankit_frenz
          Hide
          Ankit Agarwal added a comment -

          all things working fine!
          Test passed
          Thanks!

          Show
          Ankit Agarwal added a comment - all things working fine! Test passed Thanks!
          Ankit Agarwal made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao
          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: