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

Match question delete_files bug

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Questions
    • Labels:
    • 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

          Attachments

            Issue Links

              Activity

              Hide
              timhunt 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
              timhunt 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
              aav Artem Andreev added a comment -

              Thanks for quick fix

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

              Thanks Tim, this has been integrated now

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Tim, this has been integrated now
              Hide
              nebgor 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
              nebgor 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - Helen: this test may need to be added to QA tests.
              Hide
              timhunt 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
              timhunt 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
              aav 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
              aav 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
              timhunt 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
              timhunt 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
              aav 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
              aav 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
              timhunt Tim Hunt added a comment -

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

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

              Tim, i've just attached it the backup.

              agreed, the fix does improve things.

              Show
              nebgor Aparup Banerjee added a comment - Tim, i've just attached it the backup. agreed, the fix does improve things.
              Hide
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              stronk7 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
              stronk7 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
              timhunt 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
              timhunt 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Passing as requested. Thanks!

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

              Sent upstream and closing, many thanks!

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

              Removing qa_test_required label as this area is well covered by unit and acceptance tests.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Removing qa_test_required label as this area is well covered by unit and acceptance tests.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Oct/11