Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Questions
    • Database:
      Any
    • Testing Instructions:
      Hide

      Create questions of type multiplechoice and matching, with images embedded in the Combined feedback fields.

      Try moving the questions to a question category in a different context; and backing up and restoring the coures; and make sure the images are not lost.

      Try deleting the question, and make sure the right rows are deleted from the files table.

      Show
      Create questions of type multiplechoice and matching, with images embedded in the Combined feedback fields. Try moving the questions to a question category in a different context; and backing up and restoring the coures; and make sure the images are not lost. Try deleting the question, and make sure the right rows are deleted from the files table.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      While reimplementing ddmatch for 2.1 I found that qtype_match->delete_files delete area files for 'qtype_multichoice'.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tim Hunt added a comment -

            Thank you very much Artem for noticing this very nasty bug. I can't believe we did not pick this up when testing 2.1.

            Show
            Tim Hunt added a comment - Thank you very much Artem for noticing this very nasty bug. I can't believe we did not pick this up when testing 2.1.
            Hide
            Artem Andreev added a comment -

            Thanks for quick fix

            Show
            Artem Andreev added a comment - Thanks for quick fix
            Hide
            Sam Hemelryk added a comment -

            Thanks Tim, this has been integrated now

            Show
            Sam Hemelryk added a comment - Thanks Tim, this has been integrated now
            Hide
            Aparup Banerjee added a comment - - edited

            Tim,

            I've attached a screenshot of a problem i've found.

            I've moved the questions about, and performed backup and restored them to a different course ('TC' in the screenshot), while editing the question the pics are broken.

            The pics weren't broken (preview attempt) in the original backup source after moving them about.

            cheers,
            Aparup

            Show
            Aparup Banerjee added a comment - - edited Tim, I've attached a screenshot of a problem i've found. I've moved the questions about, and performed backup and restored them to a different course ('TC' in the screenshot), while editing the question the pics are broken. The pics weren't broken (preview attempt) in the original backup source after moving them about. cheers, Aparup
            Hide
            Aparup Banerjee added a comment -

            Helen: this test may need to be added to QA tests.

            Show
            Aparup Banerjee added a comment - Helen: this test may need to be added to QA tests.
            Hide
            Tim Hunt added a comment -

            Apu, please can you attach your backup file.

            I don't think this needs to block the weekly release. Although the new code may have bugs, it is still less buggy than the old code. We can release this this week, and then open a new bug for the thing that is still broken, if necessary.

            Show
            Tim Hunt added a comment - Apu, please can you attach your backup file. I don't think this needs to block the weekly release. Although the new code may have bugs, it is still less buggy than the old code. We can release this this week, and then open a new bug for the thing that is still broken, if necessary.
            Hide
            Artem Andreev added a comment - - edited

            Seems as backup_qtype_match_plugin->get_qtype_fileareas return only 'subquestion' and don't return
            'correctfeedback' => 'question_match',
            'partiallycorrectfeedback' => 'question_match',
            'incorrectfeedback' => 'question_match'

            Show
            Artem Andreev added a comment - - edited Seems as backup_qtype_match_plugin->get_qtype_fileareas return only 'subquestion' and don't return 'correctfeedback' => 'question_match', 'partiallycorrectfeedback' => 'question_match', 'incorrectfeedback' => 'question_match'
            Hide
            Tim Hunt added a comment -

            No. The files for the three combined feedback fields belong to the 'question' component because they are shared between many question types. Therefore they are added to the backup in a generic place: https://github.com/timhunt/moodle/compare/MOODLE_21_STABLE...MDL-28618_21#L0R2640

            Show
            Tim Hunt added a comment - No. The files for the three combined feedback fields belong to the 'question' component because they are shared between many question types. Therefore they are added to the backup in a generic place: https://github.com/timhunt/moodle/compare/MOODLE_21_STABLE...MDL-28618_21#L0R2640
            Hide
            Artem Andreev added a comment -

            Hmm, some core question types return such feedbacks from get_qtype_fileareas() function.
            https://github.com/moodle/moodle/blob/master/question/type/multichoice/backup/moodle2/backup_qtype_multichoice_plugin.class.php#L79
            https://github.com/moodle/moodle/blob/master/question/type/calculated/backup/moodle2/backup_qtype_calculated_plugin.class.php#L101
            And I thought that it is specific to question type has it such feedbacks or not. I was wrong?

            Show
            Artem Andreev added a comment - Hmm, some core question types return such feedbacks from get_qtype_fileareas() function. https://github.com/moodle/moodle/blob/master/question/type/multichoice/backup/moodle2/backup_qtype_multichoice_plugin.class.php#L79 https://github.com/moodle/moodle/blob/master/question/type/calculated/backup/moodle2/backup_qtype_calculated_plugin.class.php#L101 And I thought that it is specific to question type has it such feedbacks or not. I was wrong?
            Hide
            Tim Hunt added a comment -

            That is before you apply my suggested patch, isn't it.

            Show
            Tim Hunt added a comment - That is before you apply my suggested patch, isn't it.
            Hide
            Aparup Banerjee added a comment -

            Tim, i've just attached it the backup.

            agreed, the fix does improve things.

            Show
            Aparup Banerjee added a comment - Tim, i've just attached it the backup. agreed, the fix does improve things.
            Hide
            Tim Hunt added a comment -

            I suspect the problem is not unrelated to the message "The questions category "Default for Miscellaneous", originally at system/course category context in backup file, will be created at course context by restore"

            We are into very dodgy code there. I'm not suprised it is buggy.

            Would someone like to re-test this with questions that are not shared between courses?

            Show
            Tim Hunt added a comment - I suspect the problem is not unrelated to the message "The questions category "Default for Miscellaneous", originally at system/course category context in backup file, will be created at course context by restore" We are into very dodgy code there. I'm not suprised it is buggy. Would someone like to re-test this with questions that are not shared between courses?
            Hide
            Tim Hunt added a comment -

            Actually, no. When I look at the restored questions, it all seems fine to me.

            Nice baby

            Can someone independent confirm that restoring Apu's file works for them, and then this can be Closed as tested.

            Show
            Tim Hunt added a comment - Actually, no. When I look at the restored questions, it all seems fine to me. Nice baby Can someone independent confirm that restoring Apu's file works for them, and then this can be Closed as tested.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I've tried the restore into one course already having quizzes and so on:

            • 3 categories and 3 questions restored:
            • "Default for test222", at activity level, 0 questions.
            • "Default for t2", at course level, 1 question ("matching").
            • "Default for Miscellaneous", at course level, 2 questions ("multi choice" and "pond jumper"). This was the bank originally at category level.
            • Possible bug: The course where I've restored the activity backup already had one "Default for Miscellaneous" bank (from a previous restore). After restoring Aparup's backup, I've ended with TWO banks with the same name. Not sure if we should try to reuse the existing one, sounds better here. For your consideration.
            • Editing the 3 questions:
            • In the "matching" one, I get "powered by moodle" logo (@ correct response feedback), Aparups's monster (@ partially correct) and a reduced image of some fishes (incorrect response).
            • In the "multi choice" one, I get the "powered by moodle" logo in all (3) the combined feedback editors.
            • In the "pond jumper" one, I don't get any image (nor missing image).
            • Doing a new attempt: I complete it and then review it. The feedbacks with images are shown ok.

            And that's all. The only point I've found is the duplicated "Default for Miscellaneous" bank. But I think it's unrelated to this.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - I've tried the restore into one course already having quizzes and so on: 3 categories and 3 questions restored: "Default for test222", at activity level, 0 questions. "Default for t2", at course level, 1 question ("matching"). "Default for Miscellaneous", at course level, 2 questions ("multi choice" and "pond jumper"). This was the bank originally at category level. Possible bug: The course where I've restored the activity backup already had one "Default for Miscellaneous" bank (from a previous restore). After restoring Aparup's backup, I've ended with TWO banks with the same name. Not sure if we should try to reuse the existing one, sounds better here. For your consideration. Editing the 3 questions: In the "matching" one, I get "powered by moodle" logo (@ correct response feedback), Aparups's monster (@ partially correct) and a reduced image of some fishes (incorrect response). In the "multi choice" one, I get the "powered by moodle" logo in all (3) the combined feedback editors. In the "pond jumper" one, I don't get any image (nor missing image). Doing a new attempt: I complete it and then review it. The feedbacks with images are shown ok. And that's all. The only point I've found is the duplicated "Default for Miscellaneous" bank. But I think it's unrelated to this. Ciao
            Hide
            Tim Hunt added a comment -

            There has never been a limit on identical category names in the past, and no one has ever complained. I think we should leave it until a user complains.

            Show
            Tim Hunt added a comment - There has never been a limit on identical category names in the past, and no one has ever complained. I think we should leave it until a user complains.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Passing as requested. Thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Passing as requested. Thanks!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Sent upstream and closing, many thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Sent upstream and closing, many thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: