Moodle
  1. Moodle
  2. MDL-32220

Moodle XML question import sometimes fails to import files

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.5, 2.2.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Ideally, we need to test a lot of different combinations here.

      1A. Create some questions in a question category belonging to a course, with images embedded in various bits of the question text.

      1B. Create some questions in a question category belonging to a quiz, with images embedded in various bits of the question text.

      2A. Export the questions selecting both "Write category to file" and "Write context to file".

      2B. Export the questions selecting Just "Write category to file".

      2C. Export the questions selecting neither "Write category to file" nor "Write context to file".

      3A. Go to the question import page, and set the Import category to a course-level category.

      3B. Go to the question import page, and set the Import category to a quiz-level category.

      4A. Turn on both "Get category from file" and "Get context from file".

      4B. Turn on just "Get category from file"

      4C. Turn on neither "Get category from file" nor "Get context from file".

      5. Import the file you you exported in 2. above. Verify that the questions are imported into a category that is added in the right place in the question bank, and that the images in the questions are still there.

      In total, this is 2x3x2x3 = 36 possible combinations. I do not seriously expect you to test them all, just test a few different combinations, that between them cover each option at least once.

      Show
      Ideally, we need to test a lot of different combinations here. 1A. Create some questions in a question category belonging to a course, with images embedded in various bits of the question text. 1B. Create some questions in a question category belonging to a quiz, with images embedded in various bits of the question text. 2A. Export the questions selecting both "Write category to file" and "Write context to file". 2B. Export the questions selecting Just "Write category to file". 2C. Export the questions selecting neither "Write category to file" nor "Write context to file". 3A. Go to the question import page, and set the Import category to a course-level category. 3B. Go to the question import page, and set the Import category to a quiz-level category. 4A. Turn on both "Get category from file" and "Get context from file". 4B. Turn on just "Get category from file" 4C. Turn on neither "Get category from file" nor "Get context from file". 5. Import the file you you exported in 2. above. Verify that the questions are imported into a category that is added in the right place in the question bank, and that the images in the questions are still there. In total, this is 2x3x2x3 = 36 possible combinations. I do not seriously expect you to test them all, just test a few different combinations, that between them cover each option at least once.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      38996

      Description

      This bug only happens in the following situation:

      1. The questions were exported from the MODULE-level question bank for a particular quiz.

      2. The questions are being imported into the MODULE-level of the question bank in another (or the same) quiz.

      3. In the Import from, Import category is set to one of the COURSE-level question categories.

      4. The two options "Get category from file" and "Get context from file" are both turned on.

      (Congratulations to Phil Butcher of the OU for actually finding this bug!)

        Activity

        Hide
        Tim Hunt added a comment -

        This file should work for re-producing this issue. It was exported from a quiz-level question category.

        Show
        Tim Hunt added a comment - This file should work for re-producing this issue. It was exported from a quiz-level question category.
        Hide
        Tim Hunt added a comment -

        OK, submitting this for integration. It is a horrible change to make on the stable branches, but this is causing pain for users at the OU. Losing images from questions when you import and export is really quite bad. So, I hope it is OK to integrate this.

        Show
        Tim Hunt added a comment - OK, submitting this for integration. It is a horrible change to make on the stable branches, but this is causing pain for users at the OU. Losing images from questions when you import and export is really quite bad. So, I hope it is OK to integrate this.
        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
        Hide
        Dan Poltawski added a comment -

        Waiting to receive more views about whether we accept this or not.

        Note that the only format in /plugins/ has the removed argument as an optional param: http://moodle.org/plugins/view.php?plugin=qformat_hotpot and doens't use it.

        Show
        Dan Poltawski added a comment - Waiting to receive more views about whether we accept this or not. Note that the only format in /plugins/ has the removed argument as an optional param: http://moodle.org/plugins/view.php?plugin=qformat_hotpot and doens't use it.
        Hide
        Tim Hunt added a comment -

        In that case, can't we just ask Gordon to update his code, and problem solved.

        Actually, in PHP if you do

        
        <?php
        error_reporting(E_ALL);
        ini_set('display_errors', 1);
        class base {
            public function fn($a) {
                echo 'Hello ', $a;
            }
        }
        class sub {
            public function fn($a, $b) {
                echo 'Goodbye ', $a;
            }
        }
        $c = new base();
        $d = new sub();
        $c->fn('world');
        $d->fn('world');
        

        Then it works, you just get "Warning: Missing argument 2 for sub::fn()". So, we are not actually breaking stuff that works (if people have debugging off on their live systems).

        Show
        Tim Hunt added a comment - In that case, can't we just ask Gordon to update his code, and problem solved. Actually, in PHP if you do <?php error_reporting(E_ALL); ini_set('display_errors', 1); class base { public function fn($a) { echo 'Hello ', $a; } } class sub { public function fn($a, $b) { echo 'Goodbye ', $a; } } $c = new base(); $d = new sub(); $c->fn('world'); $d->fn('world'); Then it works, you just get "Warning: Missing argument 2 for sub::fn()". So, we are not actually breaking stuff that works (if people have debugging off on their live systems).
        Hide
        Dan Poltawski added a comment -

        Thanks Tim, got another + so this has been integrated now.

        Show
        Dan Poltawski added a comment - Thanks Tim, got another + so this has been integrated now.
        Hide
        Ankit Agarwal added a comment - - edited

        Hi Tim,
        Doesn't the changes needs to be ported to blackboard format?

        I am getting following errors in the export page

        Strict Standards: Declaration of qformat_blackboard::readquestions() should be compatible with that of qformat_default::readquestions() in /var/www/int/master/moodle/question/format/blackboard/format.php on line 402 Strict Standards: Declaration of qformat_blackboard_six::readquestions() should be compatible with that of qformat_default::readquestions() in /var/www/int/master/moodle/question/format/blackboard_six/format.php on line 949 
        

        Thanks

        Show
        Ankit Agarwal added a comment - - edited Hi Tim, Doesn't the changes needs to be ported to blackboard format? I am getting following errors in the export page Strict Standards: Declaration of qformat_blackboard::readquestions() should be compatible with that of qformat_default::readquestions() in /var/www/int/master/moodle/question/format/blackboard/format.php on line 402 Strict Standards: Declaration of qformat_blackboard_six::readquestions() should be compatible with that of qformat_default::readquestions() in /var/www/int/master/moodle/question/format/blackboard_six/format.php on line 949 Thanks
        Hide
        Tim Hunt added a comment -

        Well, the blackboard_six format is somewhat broken anyway, and people are working on fixing it. Therefore, if everything else is OK, I think we should pass this issue, and file a separate issue for this strict syntax problem which I will fix next week.

        Note that strict-syntax messages are about the least serious type of message you can get. They should, of course, be fixed, but nothing is likely to be broken because of one.

        (I will also note that the code does not follow the coding style - space between readquestions and ( - which is probably why my search did not find this one. Sorry.)

        Show
        Tim Hunt added a comment - Well, the blackboard_six format is somewhat broken anyway, and people are working on fixing it. Therefore, if everything else is OK, I think we should pass this issue, and file a separate issue for this strict syntax problem which I will fix next week. Note that strict-syntax messages are about the least serious type of message you can get. They should, of course, be fixed, but nothing is likely to be broken because of one. (I will also note that the code does not follow the coding style - space between readquestions and ( - which is probably why my search did not find this one. Sorry.)
        Hide
        Dan Poltawski added a comment -

        Given that the param is not being used - I propose to remove the unused argument and send it through testing again.

        (Also annoyed I didn't spot this)

        Show
        Dan Poltawski added a comment - Given that the param is not being used - I propose to remove the unused argument and send it through testing again. (Also annoyed I didn't spot this)
        Hide
        Ankit Agarwal added a comment -

        Great than...I will restart testing once Dan makes that commit.
        Thanks

        Show
        Ankit Agarwal added a comment - Great than...I will restart testing once Dan makes that commit. Thanks
        Hide
        Tim Hunt added a comment -

        Dan, if you are happy to fix this now, that would be great. Thanks.

        Show
        Tim Hunt added a comment - Dan, if you are happy to fix this now, that would be great. Thanks.
        Hide
        Dan Poltawski added a comment -

        I have pushed a fix for this (it affected master only).

        Please could you do test again, thanks for spotting that Ankit!

        Show
        Dan Poltawski added a comment - I have pushed a fix for this (it affected master only). Please could you do test again, thanks for spotting that Ankit!
        Hide
        Ankit Agarwal added a comment -

        tested this on master...works as expected..
        Will test on the other two branches tomorrow
        Thanks

        Show
        Ankit Agarwal added a comment - tested this on master...works as expected.. Will test on the other two branches tomorrow Thanks
        Hide
        Tim Hunt added a comment -

        Thanks for all your efforts Ankit.

        Show
        Tim Hunt added a comment - Thanks for all your efforts Ankit.
        Hide
        Ankit Agarwal added a comment -

        This works fine. I randomly tried some configurations and all seems to be working fine.

        In one instance I encountered the following error

        Notice: Undefined index: format in /var/www/int/22/moodle/question/import_form.php on line 125 Notice: Undefined index: format in /var/www/int/22/moodle/question/import_form.php on line 127
        
        question/formatnotfound
        
        More information about this error
        Stack trace:
        
            line 127 of /question/import_form.php: moodle_exception thrown
            line 148 of /question/import_form.php: call to question_import_form->validate_uploaded_file()
            line 514 of /lib/formslib.php: call to question_import_form->validation()
            line 463 of /lib/formslib.php: call to moodleform->validate_defined_fields()
            line 560 of /lib/formslib.php: call to moodleform->is_validated()
            line 77 of /question/import.php: call to moodleform->get_data()
        

        I am not sure what exactly was the configuration that I used, so cannot replicate it. But it doesnt look related to this fix anyway. So passing this issue.
        Thanks

        Show
        Ankit Agarwal added a comment - This works fine. I randomly tried some configurations and all seems to be working fine. In one instance I encountered the following error Notice: Undefined index: format in /var/www/int/22/moodle/question/import_form.php on line 125 Notice: Undefined index: format in /var/www/int/22/moodle/question/import_form.php on line 127 question/formatnotfound More information about this error Stack trace: line 127 of /question/import_form.php: moodle_exception thrown line 148 of /question/import_form.php: call to question_import_form->validate_uploaded_file() line 514 of /lib/formslib.php: call to question_import_form->validation() line 463 of /lib/formslib.php: call to moodleform->validate_defined_fields() line 560 of /lib/formslib.php: call to moodleform->is_validated() line 77 of /question/import.php: call to moodleform->get_data() I am not sure what exactly was the configuration that I used, so cannot replicate it. But it doesnt look related to this fix anyway. So passing this issue. Thanks
        Hide
        Tim Hunt added a comment -

        That is an unrelated issue. Looking at the code, I would guess that you can get that if you try to submit the form without selecting the import format, and for some reason, the normal client-side required field validation is not working. I guess the validate_uploaded_file method should test that $data['format'] is set before using it. Oh well.

        Thanks for your testing.

        Show
        Tim Hunt added a comment - That is an unrelated issue. Looking at the code, I would guess that you can get that if you try to submit the form without selecting the import format, and for some reason, the normal client-side required field validation is not working. I guess the validate_uploaded_file method should test that $data ['format'] is set before using it. Oh well. Thanks for your testing.
        Hide
        Aparup Banerjee added a comment -

        The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

        Closing, have a good weekend!

        Show
        Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: