Moodle

Can't display quiz attempt when question has been deleted

Details

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

    1. Create a quiz containing at least two questions, and attempt at as a student.
    2. Go into the database, and delete one of the questions used. (That is, delete the row from the mdl_question table. Ideally also delete the linked data in other tables.)
    3. Review the quiz attempt using that question. Of course, you cannot expect to see the deleted questions, but as much as possible of the remaining information should still be visible and usable.

    (A less destructive way to do step 2. is just to edit the id in the mdl_question table. E.g. change id 123 to id 100123. Then you can easily put it back once you have finished testing.)

    Show
    1. Create a quiz containing at least two questions, and attempt at as a student. 2. Go into the database, and delete one of the questions used. (That is, delete the row from the mdl_question table. Ideally also delete the linked data in other tables.) 3. Review the quiz attempt using that question. Of course, you cannot expect to see the deleted questions, but as much as possible of the remaining information should still be visible and usable. (A less destructive way to do step 2. is just to edit the id in the mdl_question table. E.g. change id 123 to id 100123. Then you can easily put it back once you have finished testing.)
  • Affected Branches:
    MOODLE_21_STABLE, MOODLE_22_STABLE
  • Fixed Branches:
    MOODLE_21_STABLE
  • Pull from Repository:
  • Pull Master Branch:
  • Pull Master Diff URL:

Description

Sorry, I need to file another question-related problem. Not sure whether this is only related to data migration.

When a quiz attempt contains a deleted question, viewing the quiz attempt (mod/quiz/review.php) fails with an error message as below. The remainder of the page is not shown.

Category ID is invalid

More information about this error
Stack trace:

  • line 429 of /lib/setuplib.php: moodle_exception thrown
  • line 1349 of /lib/questionlib.php: call to print_error()
  • line 696 of /mod/quiz/attemptlib.php: call to question_has_capability_on()
  • line 951 of /mod/quiz/attemptlib.php: call to quiz_attempt->get_display_options_with_edit_link()
  • line 182 of /mod/quiz/renderer.php: call to quiz_attempt->render_question()
  • line 54 of /mod/quiz/renderer.php: call to mod_quiz_renderer->questions()
  • line 259 of /mod/quiz/review.php: call to mod_quiz_renderer->review_page()

This comes about as follows. In qtype_missingtype::make_deleted_instance(), the category id of the "fake" question is set to NULL. However, in lib/questionlib.php, function question_has_capability_on(), the question is tested on having a valid category. None is found, so an error is encountered.

So far, I was able to reproduce this with migrated data only. Is Moodle 2.1 expected to deal with "deleted questions" or is this a problem of the migration process?

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Tim Hunt added a comment -

It is probably a migration only issues - or at least a 'result of an earlier bug'-only issue.

I suggest we add an if (!emtpy($question->categoryid) somewhere. If this is a fake placeholder for a deleted question, there is no point offering an edit link.

Show
Tim Hunt added a comment - It is probably a migration only issues - or at least a 'result of an earlier bug'-only issue. I suggest we add an if (!emtpy($question->categoryid) somewhere. If this is a fake placeholder for a deleted question, there is no point offering an edit link.
Hide
Permalink
Henning Bostelmann added a comment -

This modification fixes it for me. Users will not have any capabilities on deleted questions, which seems consistent.

Show
Henning Bostelmann added a comment - This modification fixes it for me. Users will not have any capabilities on deleted questions, which seems consistent.
Hide
Permalink
Tim Hunt added a comment -

Thanks Rossi for starting the review of this. I will take it from here.

Show
Tim Hunt added a comment - Thanks Rossi for starting the review of this. I will take it from here.
Hide
Permalink
Rossiani Wijaya added a comment -

No worries Tim.

All yours.

Show
Rossiani Wijaya added a comment - No worries Tim. All yours.
Hide
Permalink
Rossiani Wijaya added a comment -

Hi Henning,

Thanks for providing the patch to address the issue.

Could you also provide some testing instructions to test this issue?

Show
Rossiani Wijaya added a comment - Hi Henning, Thanks for providing the patch to address the issue. Could you also provide some testing instructions to test this issue?
Hide
Permalink
Tim Hunt added a comment -

Two problems with the patch:

1. You get simpler code by inserting the new code after the existing if (!is_object($question)) { block.

2. In general, don't pollute the code with references to bug numbers. All that information is automatically tracked by git. On the other hand, the situation that makes this new code necessary is quite obscure, so a comment that explains why it is needed is a good idea.

I am about to submit a revised fix for integration.

Show
Tim Hunt added a comment - Two problems with the patch: 1. You get simpler code by inserting the new code after the existing if (!is_object($question)) { block. 2. In general, don't pollute the code with references to bug numbers. All that information is automatically tracked by git. On the other hand, the situation that makes this new code necessary is quite obscure, so a comment that explains why it is needed is a good idea. I am about to submit a revised fix for integration.
Hide
Permalink
Tim Hunt added a comment -

Henning, if you could test my revised fix, that would be helpful. Thanks.

Show
Tim Hunt added a comment - Henning, if you could test my revised fix, that would be helpful. Thanks.
Hide
Permalink
Henning Bostelmann added a comment -

Thanks Tim. I have now tested this with my "real life" data on the MOODLE_21_STABLE branch (or rather your branch MDL-28687_21). It fixes the problem for me.

Show
Henning Bostelmann added a comment - Thanks Tim. I have now tested this with my "real life" data on the MOODLE_21_STABLE branch (or rather your branch MDL-28687 _21). It fixes the problem for me.
Hide
Permalink
Tim Hunt added a comment -

Great! Thanks for the confirmation. And I am impressed by the speed!

Show
Tim Hunt added a comment - Great! Thanks for the confirmation. And I am impressed by the speed!
Hide
Permalink
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
Permalink
Aparup Banerjee added a comment - - edited

just checking: is this to be fixed in 2.0.x as well?

Show
Aparup Banerjee added a comment - - edited just checking: is this to be fixed in 2.0.x as well?
Hide
Permalink
Henning Bostelmann added a comment -

No, it's related to the new Question Engine only, which was introduced in 2.1.

Show
Henning Bostelmann added a comment - No, it's related to the new Question Engine only, which was introduced in 2.1.
Hide
Permalink
Aparup Banerjee added a comment -

Thanks! This has been integrated!

Show
Aparup Banerjee added a comment - Thanks! This has been integrated!
Hide
Permalink
Rossiani Wijaya added a comment -

works great.

Test passed

Show
Rossiani Wijaya added a comment - works great. Test passed
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

Show
Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: