Moodle
  1. Moodle
  2. MDL-28686

QE2 upgrade fails on manually graded questions that have been deleted

    Details

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

      Migration-related bug; testing is complicated. Try the following:

      • Make a clean Moodle 1.9 installation.
      • Create a course and an empty quiz.
      • Create (at least) one essay question in the main category for the quiz, and one in a subcategory.
      • Add to the quiz:
        • One essay question from the main category
        • One random question taking essay questions from the subcategory
      • Log in as a student and take the quiz. Submit answers to the essay questions. Finish the attempt.
      • Log in as a teacher and grade the essay questions.
      • Now delete the essay questions (best done manually in the DB, from mdl_questions).
      • Run the upgrade routine to MOODLE_21_STABLE.

      Verify that during this upgrade, the QE 2 upgrader does not display error messages. Also, verify the output of the migration; the deleted questions should be displayed as "This question is missing. Unable to display anything." but not lead to fatal errors. (Use latest build; note dependency on MDL-28687.)

      Show
      Migration-related bug; testing is complicated. Try the following: Make a clean Moodle 1.9 installation. Create a course and an empty quiz. Create (at least) one essay question in the main category for the quiz, and one in a subcategory. Add to the quiz: One essay question from the main category One random question taking essay questions from the subcategory Log in as a student and take the quiz. Submit answers to the essay questions. Finish the attempt. Log in as a teacher and grade the essay questions. Now delete the essay questions (best done manually in the DB, from mdl_questions). Run the upgrade routine to MOODLE_21_STABLE. Verify that during this upgrade, the QE 2 upgrader does not display error messages. Also, verify the output of the migration; the deleted questions should be displayed as "This question is missing. Unable to display anything." but not lead to fatal errors. (Use latest build; note dependency on MDL-28687 .)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      18473

      Description

      The following error message occurred during my QE2 upgrade (1.9 -> 2.1):

      Coding error detected, it must be fixed by a programmer: Unexpected event 7 in state 46454 in question session 17385.

      Situation in the data: The quiz at hand is composed of random questions; one of the "real questions" behind them was manually graded. Students made attempts and the teacher graded these. However, at a later point, the teacher must have deleted the question. Therefore, question states with event=7 (submission for manual grading) remain in the DB.

      In the code, the problem is that question_engine_attempt_upgrader::get_converter_class_name() uses the quiz's preferred behaviour in the case of a deleted question. This behaviour converter may not be able to deal with event 7, though. (For me, this was qbehaviour_deferredfeedback_converter.)

      Proposed fix (tested against my data): Scan the question_states table for any event=7 entries for the question. If any are present, assume that the deleted question was manually graded.

      Code change to follow; remark: I consider it a safe assumption that $question->id is an integer.

        Activity

        Hide
        Tim Hunt added a comment -

        Yes, $question->id is an integer.

        By the way, have you explored local/qeupgradehelper/extracttestcase.php ?

        That makes it quite easy to create unit tests from garbage data in your database, which makes fixes like these much easier and safer to implement.

        Show
        Tim Hunt added a comment - Yes, $question->id is an integer. By the way, have you explored local/qeupgradehelper/extracttestcase.php ? That makes it quite easy to create unit tests from garbage data in your database, which makes fixes like these much easier and safer to implement.
        Hide
        Henning Bostelmann added a comment -

        Thanks for pointing me at the "extract test case" feature - I hadn't tried that. I'm now done with all my "strange" cases (hopefully), but if you need the specific attempt data for testing purposes, I can provide it. Anyway, the QE upgrade helper (even with its other features) was extremely useful for me.

        Show
        Henning Bostelmann added a comment - Thanks for pointing me at the "extract test case" feature - I hadn't tried that. I'm now done with all my "strange" cases (hopefully), but if you need the specific attempt data for testing purposes, I can provide it. Anyway, the QE upgrade helper (even with its other features) was extremely useful for me.
        Hide
        Tim Hunt added a comment -

        This will work, but it is not great.

        1. It sucks to have to do an extra DB query here. Can't we just pass the array of $states to this method, and then loop through it?

        2. Even if we decide to stick with a DB query, it should be record_exists, not count_records, for efficiency. We don't care exactly how many records there are, just whether there are any or not.

        Show
        Tim Hunt added a comment - This will work, but it is not great. 1. It sucks to have to do an extra DB query here. Can't we just pass the array of $states to this method, and then loop through it? 2. Even if we decide to stick with a DB query, it should be record_exists, not count_records, for efficiency. We don't care exactly how many records there are, just whether there are any or not.
        Hide
        Henning Bostelmann added a comment -

        Point taken about record_exists - I wasn't aware that this method existed. Commit amended.

        Regarding the DB query, actually using the query seems to be more accurate: If the question has an event=7 state in any attempt (not just the current one) it must have been manually graded. Anyway, efficiency should not be an issue here. These are spurious cases.

        Show
        Henning Bostelmann added a comment - Point taken about record_exists - I wasn't aware that this method existed. Commit amended. Regarding the DB query, actually using the query seems to be more accurate: If the question has an event=7 state in any attempt (not just the current one) it must have been manually graded. Anyway, efficiency should not be an issue here. These are spurious cases.
        Hide
        Tim Hunt added a comment -

        Ah, good point about checking all attempts. And thank you for making a revised patch. I'm submitting this for integration now.

        Show
        Tim Hunt added a comment - Ah, good point about checking all attempts. And thank you for making a revised patch. I'm submitting this for integration now.
        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
        Eloy Lafuente (stronk7) added a comment - - edited

        Sorry but I've to fail this (all minor details, but):

        1) put the global at the beginning of the method.
        2) SQL keywords (OR, LIKE, AND...) must be uppercase
        3) For everything not being a constant use database placeholders and params array (question marks or named, surely named for similarity with the rest of SQL code in that file. Tip: For the LIKE part, $DB->sql_like() can be handy.

        I'll be happy to integrate this next week. Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - - edited Sorry but I've to fail this (all minor details, but): 1) put the global at the beginning of the method. 2) SQL keywords (OR, LIKE, AND...) must be uppercase 3) For everything not being a constant use database placeholders and params array (question marks or named, surely named for similarity with the rest of SQL code in that file. Tip: For the LIKE part, $DB->sql_like() can be handy. I'll be happy to integrate this next week. Thanks!
        Hide
        Henning Bostelmann added a comment -

        Changes amended and rebased.

        If these are required standards, would it make sense to add them to http://docs.moodle.org/dev/Coding_style ?

        Show
        Henning Bostelmann added a comment - Changes amended and rebased. If these are required standards, would it make sense to add them to http://docs.moodle.org/dev/Coding_style ?
        Hide
        Tim Hunt added a comment -

        In terms of making this easier to test, the best thing would be to use the question engine upgrade helper to extract a unit test. I can help you with that if you need more detailed instructions.

        Show
        Tim Hunt added a comment - In terms of making this easier to test, the best thing would be to use the question engine upgrade helper to extract a unit test. I can help you with that if you need more detailed instructions.
        Hide
        Henning Bostelmann added a comment -

        I agree, but I'm not convinced whether it works in this particular case (I didn't look at the unit test code in detail). The problem is that, in my data, the affected questions are "random questions". The QE Upgrades produces something for these, but it doesn't seem to cover the details of the underlying "actual" questions. The underlying "actual" question is the one that was deleted.

        I'm attaching the output anyway. Note that some PHP warnings are included (for clarity). The deleted question had id=1054.

        Show
        Henning Bostelmann added a comment - I agree, but I'm not convinced whether it works in this particular case (I didn't look at the unit test code in detail). The problem is that, in my data, the affected questions are "random questions". The QE Upgrades produces something for these, but it doesn't seem to cover the details of the underlying "actual" questions. The underlying "actual" question is the one that was deleted. I'm attaching the output anyway. Note that some PHP warnings are included (for clarity). The deleted question had id=1054.
        Hide
        Tim Hunt added a comment -

        Sorry for the delay. I just looked at it again, and the revised patch looks good. Suitable for integration next week.

        Henning, if you can rebase your branches once more before integration, that would be good.

        In terms of testing, I think it is enough to test a basic upgrade from 2.0 -> 2.1 with some quiz attempts. That is enough to verify that this does not cause any regressions, which is the main thing. I think we can trust Henning that this change makes things better on his system.

        Show
        Tim Hunt added a comment - Sorry for the delay. I just looked at it again, and the revised patch looks good. Suitable for integration next week. Henning, if you can rebase your branches once more before integration, that would be good. In terms of testing, I think it is enough to test a basic upgrade from 2.0 -> 2.1 with some quiz attempts. That is enough to verify that this does not cause any regressions, which is the main thing. I think we can trust Henning that this change makes things better on his system.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, many thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, many thanks!
        Hide
        Sam Hemelryk added a comment -

        Pass! thanks guys. Nice test.

        Show
        Sam Hemelryk added a comment - Pass! thanks guys. Nice test.
        Hide
        Aparup Banerjee added a comment -

        fixes have been rolled merrily up the stream! Thanks everybody!

        Show
        Aparup Banerjee added a comment - fixes have been rolled merrily up the stream! Thanks everybody!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: