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:
    • Rank:
      18392

      Description

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

        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: