Moodle

Question Engine delete questions in use by modules other than QUIZ

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Won't Fix
  • Affects Version/s: 1.9.6
  • Fix Version/s: None
  • Component/s: Questions
  • Labels:
    None
  • Difficulty:
    Easy
  • Affected Branches:
    MOODLE_19_STABLE

Description

When deleting questions Moodle tries to detect used ones but only query a table from the QUIZ module. This breaks the plug-ability of this part of Moodle.

Problematic code (editlib.php):

if (record_exists('quiz_question_instances', 'question', $key)) { $questionnames .= '* '; $inuse = true; }

We propose this set of changes to solve the issue:

Change above lines to be:

$inuse = question_is_question_in_use($key);

And define the new function:

function question_is_question_in_use($questionid)
{
global $CFG;
require_once($CFG->libdir . '/ddllib.php');
$modules = get_records("modules");
// Scan all modules looking for a table named modulename_question_instances
$question_in_use = false;
foreach ($modules as $module)
{
$table = new XMLDBTable($module->name.'_question_instances');
if (table_exists($table))
{
if (record_exists($module->name.'_question_instances', 'question', $questionid))

{ return true; }

}
}
return false;
}

This way default functionality remains the same and new modules can register the use of question instances by defining a table named "mdlprefix_modulename_question_instances" (like QUIZ does).

The same piece of code is reused in the function question_showbank_actions:

if (record_exists('quiz_question_instances', 'question', $questionid)) {
if (!set_field('question', 'hidden', 1, 'id', $questionid)) { question_require_capability_on($questionid, 'edit'); error('Was not able to hide question'); }
} else { delete_question($questionid); }

We propose to change it to:

question_delete_or_hide($questionid);

Being this function:

function question_delete_or_hide($questionid)
{
$question_in_use = question_is_question_in_use($questionid);

// If the question is instanced in any module do not delete it, just hide it.
if ($question_in_use == true)
{
if (!set_field('question', 'hidden', 1, 'id', $questionid))

{ question_require_capability_on($questionid, 'edit'); error('Was not able to hide question'); }

}
else

{ delete_question($questionid); }

}

This approach has been tested and we don't see any adverse side-effects.

Activity

Hide
Petr Škoda (skodak) added a comment -

I do not think this would be correct approach in 2.0, instead we have new plugin_supports(), you would get list of all plugins and then query them all to see if they use.

Also please note that this would not work for all plugins/modules because there is a limit on table name length - "_question_instances" suffix is definitely too long.

Show
Petr Škoda (skodak) added a comment - I do not think this would be correct approach in 2.0, instead we have new plugin_supports(), you would get list of all plugins and then query them all to see if they use. Also please note that this would not work for all plugins/modules because there is a limit on table name length - "_question_instances" suffix is definitely too long.
Hide
Petr Škoda (skodak) added a comment -

hmm, thinking a bit more about the proposed patch the use of DDL layer outside of upgrade/install does not look correct.

Show
Petr Škoda (skodak) added a comment - hmm, thinking a bit more about the proposed patch the use of DDL layer outside of upgrade/install does not look correct.
Hide
Juan Pablo de Castro added a comment -

The patch proposal was directly based on the current implementation for quiz standard module. It 's obvious that there are a design flaw in this part of Moodle.

There are a better way to solve this issue for the 1.9.x family?

PD: regarding to the table name length: what is the recommended limit? I've noticed that a lot of tables in Moodle are as big as mdl_quiz_question_instances.

Show
Juan Pablo de Castro added a comment - The patch proposal was directly based on the current implementation for quiz standard module. It 's obvious that there are a design flaw in this part of Moodle. There are a better way to solve this issue for the 1.9.x family? PD: regarding to the table name length: what is the recommended limit? I've noticed that a lot of tables in Moodle are as big as mdl_quiz_question_instances.
Hide
Tim Hunt added a comment -

The above is wrong. There is already a function question_in_use in questionlib.php, that calls any functions like quiz_question_in_use - the function can then do whatever test is necessary. We should not be assuming table names.

The implementation may be dodgy in Moodle 1.9, but I think it is good in 2.0.

Following Petr's plugin cleanup, it might be possible to clean up the code further in 2.0, anyway, the suggested change here is definitely not right.

Show
Tim Hunt added a comment - The above is wrong. There is already a function question_in_use in questionlib.php, that calls any functions like quiz_question_in_use - the function can then do whatever test is necessary. We should not be assuming table names. The implementation may be dodgy in Moodle 1.9, but I think it is good in 2.0. Following Petr's plugin cleanup, it might be possible to clean up the code further in 2.0, anyway, the suggested change here is definitely not right.
Hide
Juan Pablo de Castro added a comment -

I apologize for filling this issue. Now I see the function question_list_instances($questionid) that is the way to do this.

Thank you very much for your help.

Show
Juan Pablo de Castro added a comment - I apologize for filling this issue. Now I see the function question_list_instances($questionid) that is the way to do this. Thank you very much for your help.
Hide
Michael de Raadt added a comment -

Thanks for reporting this issue.

We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

Michael d;

lqjjLKA0p6

Show
Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; lqjjLKA0p6
Hide
Michael de Raadt added a comment -

I'm closing this issue as it appears to have become inactive and is probably not relevant to a current supported version. If you are encountering this problem or one similar, please launch a new issue.

Show
Michael de Raadt added a comment - I'm closing this issue as it appears to have become inactive and is probably not relevant to a current supported version. If you are encountering this problem or one similar, please launch a new issue.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: