Moodle

Thanks for providing all the details, however, I would be surprised if the shared category or the fact that the question has been previewed makes a difference. (Please correctly me if I am wrong.)

I'll look into this when I get back from my Christmas holiday.

When I look at this issue I find the following line, line 115 in the file question/type/multianswer/questiontype.php

$wrapped->category = $question->category . ',1'; // save_question strips this extra bit off again.

Perhaps the creator of this line intended it as a flag that would be stripped by one of the explode statements in the save_question function(s). That doesn't happen, but it does break the sql update query. Remove the line and the site works.

I believe it was just a rogue error that nobody has yet fixed upstream.

Thank you for the extra details. That is probably close to the cause of the problem, but it is not necessarily that simple.

When editing questions, in some places where data is passed from the editing form to/from a question, $question->category needs to be of the form $categoryid,$contextid. Therefore we need some more careful analysis to determine whether this line is correct or not.

The place to start the analysis is CVS history. Yes: this line was last changed in revision 1.40 of the file, which has comment:

"Related to MDL-10916 - saving Cloze questions in Moodle 1.9 generates Notices because of changes to questionstupe_base::save_question."

Hmm. Looking at default_questiontype::save_question in question/type/questiontype.php I think we should only be doing this when saving an existing wrapped question, rather than creating a new one, so this code needs to be

if ($wrapped->id) { $wrapped->category = $question->category } else { $wrapped->category = $question->category . ',1'; // save_question strips this extra bit off again. }

But I don't have time to test that properly.

Tim,
Before changing code look at the following test.

I create the role Shared question creator (add, edit all, move all, use all ).
I assigned this Shared question creator role to moodle user.
I create a question i.e the question up here in the course (SECOND) and move it (using the move all) to category Default for Divers.
I was able to edit and preview.
So everything is working fine and the bug is not in the questiontype.php

You can try on a 1.9.2+
http://132.208.141.198/moodle_19/
user:moodle pw:moodle

Further testing shows that when I edit the question (name = MDL-12719) in category Default for Divers I only have access to this category. If i create a new question I have access to all categories (SECOND course and Default for Divers).

If I save as new the original question(name = MDL-12719) as new name(test MDL-12719 saved as new in Defautl for Divers) in category Default for Divers everything is fine.
And if I move this question to Default for SECOND, everything is fine.

When editing I have the same problem even as administrator, there is just the root category ( Default for SECOND) and its next categories that are available.
It seems to be a general feature (for other questiontypes as numerical)...

When you ADD you have access to every categories that are allowed to you but not when you EDIT .

in question.php
line 24 $movecontext = optional_param('movecontext', 0, PARAM_BOOL);//switch to make question
//uneditable - form is displayed to edit category only
line 79
if ($id) {
$canview = question_has_capability_on($question, 'view');
if ($movecontext){ $question->formoptions->canedit = false; $question->formoptions->canmove = (question_has_capability_on($question, 'move') && $contexts->have_cap('moodle/question:add')); $question->formoptions->cansaveasnew = false; $question->formoptions->repeatelements = false; $question->formoptions->movecontext = true; $formeditable = true; question_require_capability_on($question, 'view'); } else {
$question->formoptions->canedit = question_has_capability_on($question, 'edit');
$question->formoptions->canmove = (question_has_capability_on($question, 'move') && $addpermission);
$question->formoptions->cansaveasnew = (($canview ||question_has_capability_on($question, 'edit')) && $addpermission);
$question->formoptions->repeatelements = ($question->formoptions->canedit || $question->formoptions->cansaveasnew);
$formeditable = $question->formoptions->canedit || $question->formoptions->cansaveasnew || $question->formoptions->canmove;
$question->formoptions->movecontext = false;
if (!$formeditable){ question_require_capability_on($question, 'view'); }
}

} else { // creating a new question
require_capability('moodle/question:add', $categorycontext);
$formeditable = true;
$question->formoptions->repeatelements = true;
$question->formoptions->movecontext = false;
}
in edit_question_from.php
line 70 if (!isset($this->question->id)){ //adding question $mform->addElement('questioncategory', 'category', get_string('category', 'quiz'), array('contexts' => $this->contexts->having_cap('moodle/question:add'))); } elseif (!($this->question->formoptions->canmove || $this->question->formoptions->cansaveasnew)){ //editing question with no permission to move from category. $mform->addElement('questioncategory', 'category', get_string('category', 'quiz'), array('contexts' => array($this->categorycontext))); } elseif ($this->question->formoptions->movecontext){ //moving question to another context. $mform->addElement('questioncategory', 'categorymoveto', get_string('category', 'quiz'), array('contexts' => $this->contexts->having_cap('moodle/question:add'))); } else {
//editing question with permission to move from category or save as new q
$currentgrp = array();
$currentgrp[0] =& $mform->createElement('questioncategory', 'category', get_string('categorycurrent', 'question'),
array('contexts' => array($this->categorycontext)));
if ($this->question->formoptions->canedit || $this->question->formoptions->cansaveasnew){ //not move only form $currentgrp[1] =& $mform->createElement('checkbox', 'usecurrentcat', '', get_string('categorycurrentuse', 'question')); $mform->setDefault('usecurrentcat', 1); }
If can save as new and move to category I think we should defined context as precedently i.e.
array('contexts' => $this->contexts->having_cap('moodle/question:add'))
Unless we want to define "move from category" differently for categories that are in the context as sub-categories than those that are in the contexts to which the user has been given access by the Shared question creator...

These issues could be similar and the problem could come from how the database works well with the category expressed as category, another parameter.

I think this patch questionediting.patch.txt fixes this bug, and MDL-18174, MDL-15774 and MDL-17105.

However, this code is clearly quite fragile, judging by the number of bug reports we have had relating to it in the past, so I would really appreciate some help testing it.

Basically, what I have done is move the code that deals with the combingation of usecurrentcat, categorymoveto and category into question.php, so the save_question method only has to worry about saving the question into the category it is told to.

I agree that this appears to me the right step even if my programming knowledge is not yours...
However there is some ambiguity about set ot empty or = 0 for $question_id

"Whether we are saving a new question or updating an existing one can be
+ * determined by testing whether $question->id is set. If it is, we are updating.
"
then the code
if (!empty($question->id)
which will work

From PHP docs
empty()Returns FALSE if var has a non-empty and non-zero value.

The following things are considered to be empty:

• "" (an empty string)
• 0 (0 as an integer)
• "0" (0 as a string)
• NULL
• FALSE
• array() (an empty array)
• var $var; (a variable declared, but without a value in a class)

However
a better comment will be
"Whether we are saving a new question or updating an existing one can be
+ * determined by testing whether !empty($question->id) returns true . If it is, we are updating."
or something similar.

I will do some tests in the next days (or nights) .

You are right about the comment. I will improve it.

Tested on 1.9 following the issue description. It works. Thanks everybody.