Moodle

sending patch for review, Tim or Jamie, please could you have a look at the question related code? thanks

Hi Petr, some comments... just after reviewing the code:

1- I find the recursive deletion highly "dangerous". I hope it's protected behind at least 2 confirmation boxes.
2- In general... could we start using some sort of execution_result class or so in order to return functions execution results? I can imagine this functions (or similar) as potentially being used by web-services or so and separating process from output could be a good idea IMO.
3- There is one return false missing the corresponding notify (the first in category_delete_move() ).
4- Not knowing internals very well just want to notice that, when category_delete_move() is used, courses aren't deleted, so they can be using questions, so IMO questions should be moved to parent cat too, instead of deleted. For sure Tim or Jamie will confirm this. In the other side I think it's safe to delete questions when the category_delete_full() is used, because courses (potential uses) are deleted. Needs confirmation too.
5- Perhaps it would be interesting to leave some records about deletions in log table?

Ciao

It would be better to move questions to a parent category. Publishing questions in the course category means you are sharing them to be used amongst all courses in that category. Moving them up a category means the sharing becomes wider, would be good to notify the user that that is what you will do before asking them to confirm to continue.

1- adding one more confirmation
2- that would be very nice to have
3- fixing
4- questions are move to new caregory if courses not deleted
5- agree the course deletes should be logged

to Jamie:
the parent category is not used here anymore - they are either all moved to new selected category or deleted

All sounds good to me! +1

In patch 4, this is surely wrong:

function question_delete_course_category($category, $newcategory, $feedback=true) {
$context = get_context_instance(CONTEXT_COURSECAT, $category->id);
if (empty($newparentid)) {

I also don't think that you should be doing:

delete_records("question", "category", $category->id);

after

foreach ($questions as $question) {
delete_question($question->id);
}

There is a reason why the code

// Do not delete a question if it is used by an activity module
if (count(question_list_instances($questionid))) { return; }

exists in function delete_question.

If there are any questions left after the for loop, then you should set them to be hidden, and move them to the default site category, or something.

Tim:

sure silly bug caused by refactoring, should be
if (empty($newcategory->id)) { // means user requested full category delete including all activities, courses and categories

I do not understand why you would be moving the questions to anywhere when deleting everything in course category.

Becuase it is possible, in sites that have upgraded from an old version of Moodle, that a quiz in one course refers to a question belonging to another course. And because people care a lot about assessment data, we make sure that even if their database is screwed up in this way, we do not delete a question that is still needed (but we do mark it as hidden).

Thanks Tim, now I understand your reasons.
Could you please make some question_delete_course_category() that solves this problem when doing full delete?

I'll see what I can do. The bit of code from your patch is pretty close, I will probably build on that.

Thinking a bit more:
1/ why are the shared questions not in some category at the system level?
2/ I guess we should also add upgrade code that finds all orphaned categories (left join to context table returns null) and move them somewhere

+ * Category is about to be deleted,
+ * 1/ All question categories and their questions are deleted for this course category.
+ * 2/ All questions are moved to new category
At first sigth it is not clear if the following line move ALL questions to the new category.

return set_field('question_categories', 'contextid', $newcontext->id, 'contextid', $context->id);

Is the subquestions are also moved or not ?
see MDL-10899

Or perhaps you are not moving the questions but moving the category in another context?

However because of MDL-10899 if a cloze question has been moved to another course, the related subquestions where not moved, will be deleted and the cloze question is invalid.
MDL-10899 is a real bug...

An update (category_delete_5.patch) of Petr's latest patch, which deals with the question bank properly. If this gets checked in, MDL-14633 can be marked fixed.

There is a bug in my patch. Line 572 of questionlib.php should be

$questionids = get_records_select_menu('question', 'category = ' . $category->id, '', 'id,1');

Update patch. I have tested and it should work, once I have checked in the fix for MDL-14804, which will happen momentarily.

sending patch with delete confirmation option - it looks a bit weird, I need to ask MD before commit
thanks!

assigning to martin for UI changes review

I'll review this in 4 hours.

Yes, I see what you mean.

Some thoughts:

• Don't embed <br> in the new strings. It can be shorter anyway: "If you delete this category, you need to choose what to do with the courses and subcategories it contains."
• I got the confirm dialogs even when the category was empty (brand new)
• "Confirm category delete" menu is a bit odd (both as a UI thing and in it's wording, because it applies to more than categories), could go completely.
• "Delete" fieldset could be called "Contents of $categoryname"
• The second feedback screen has some issues:
  a) it shows empty table headers even where there are no questions involved at all
  b) Category in this case means question category (not course category). Needs to be a lot clearer, say "Question category"

committed into cvs, big thanks everybody!

Tested and approved!