|
Petr Skoda made changes - 16/Apr/08 06:05 AM
Petr Skoda made changes - 16/Apr/08 06:05 AM
Petr Skoda made changes - 16/Apr/08 06:12 AM
Petr Skoda made changes - 16/Apr/08 06:12 AM
Petr Skoda made changes - 17/Apr/08 05:49 AM
[
Permalink
| « Hide
]
Petr Skoda added a comment - 29/Apr/08 05:52 AM
sending patch for review, Tim or Jamie, please could you have a look at the question related code? thanks
Petr Skoda made changes - 29/Apr/08 05:52 AM
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. 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: All sounds good to me! +1
In patch 4, this is surely wrong:
function question_delete_course_category($category, $newcategory, $feedback=true) { I also don't think that you should be doing: delete_records("question", "category", $category->id); after foreach ($questions as $question) { There is a reason why the code // Do not delete a question if it is used by an activity module 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 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? 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 ? Or perhaps you are not moving the questions but moving the category in another context?
Tim Hunt made changes - 13/May/08 12:21 AM
Tim Hunt made changes - 13/May/08 01:21 AM
sending patch with delete confirmation option - it looks a bit weird, I need to ask MD before commit
thanks!
Petr Skoda made changes - 13/May/08 03:45 AM
assigning to martin for UI changes review
Petr Skoda made changes - 13/May/08 07:20 AM
I'll review this in 4 hours.
Yes, I see what you mean.
Some thoughts:
Martin Dougiamas made changes - 13/May/08 11:47 PM
Petr Skoda committed 9 files to 'Moodle CVS' on branch 'MOODLE_19_STABLE' - 14/May/08 05:51 AM
Petr Skoda committed 9 files to 'Moodle CVS' - 14/May/08 05:52 AM
Petr Skoda committed 2 files to 'Moodle CVS' on branch 'MOODLE_19_STABLE' - 14/May/08 06:03 AM
Petr Skoda committed 2 files to 'Moodle CVS' - 14/May/08 06:04 AM
committed into cvs, big thanks everybody!
Petr Skoda made changes - 14/May/08 06:04 AM
martignoni committed 3 files to 'Lang CVS' - 14/May/08 05:47 PM
martignoni committed 1 file to 'Lang CVS' - 15/May/08 01:06 AM
Mitsuhiro Yoshida committed 3 files to 'Lang CVS' - 18/May/08 02:17 AM
Mitsuhiro Yoshida committed 1 file to 'Lang CVS' - 18/May/08 02:26 AM
Nicolas Connault made changes - 10/Jun/08 03:35 PM
Nicolas Connault made changes - 17/Jun/08 02:00 PM
Anthony Borrow made changes - 09/Jul/08 10:10 AM
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||