Moodle

The todos in mod/quiz/addrandom.php:

//setting the second parameter of process_randomquestion_formdata to true causes it to redirect on success
//TODO: process if returns false?
quiz_process_randomquestion_formdata($qcobject,true, $cmid);

The correct way to handle the error condition if the function returns false is to call print_error with appropriate arguments.

And I think it is pretty bad that quiz_process_randomquestion_formdata sometimes redirects and sometimes does not. It would be better if it always returned, and the redirect, which is only needed by addrandom.php, was there.

Actually, I think you can skip logging when the add category form is displayed, but you should log when the category is actually added.

TODO in mod/quiz/edit.php

I think the unset is not necessary, so no worries here.

Not a TODO, as such, but also in mod/quiz/edit.php

// as soon as javascript toggling and saving the state to the server as an ajax call
// has been done, add $quiz_hide_javascript to $localjs

This is now done. Look at print_collapsible_region_start in lib/weblib.php. In particular the calls to user_preference_allow_ajax_update and get_user_preferences, and then the calls to set_user_preference from the collapsible_region code in lib/javascript-static.js

In mod/quiz/editlib.php

//TODO: Tim? remove showbreaks since it is no longer used
function quiz_print_question_list($quiz, $pageurl, $allowdelete=true,

Since this function is only called from one place, and it seems unlikely that people would be relying on it in customisations, it is better to remove the $showbreaks parameter.

(In functions that are used from a lot of places, so you can't remove the paramter, I normally just rename it to $notused, or $obsolete, or something.)

//TODO: if moodle used PHP exceptions, we would raise one here so that
//the main UI would know to display quis status "unfinished".

Well, in Moodle 2.0 you can do throw new moodle_exception(...); however, it should only be used for truly exceptional things, and a random question pointing to a category with no questions is does not seem that exceptional to me. We expect it to happen some times in normal operation.

//TODO: this should be a call to quiz_question_tostring, right?

Yes, I think it should, and if so, you can clean up some code earlier in the function.

//TODO: Tim? I left this out from the below get_record call:, , 'contextid' => $contextid

Correct.

//TODO: URGENT fix question preview for random questions
/*echo '<span class="questionpreview">'.
quiz_question_preview_button($quiz, $question).
'</span>';
*/

Can you put the preview icon back, and file a bug assigned to me saying fix preview of random questions. Thanks.

Hmm, those last ones come twice. It is a pity there is so much duplicated code between quiz_print_randomquestion and quiz_print_randomquestion_reordertool. Oh well, there is worse code duplication in Moodle.

"This is now done. Look at print_collapsible_region_start in lib/weblib.php. In particular the calls to user_preference_allow_ajax_update and get_user_preferences, and then the calls to set_user_preference from the collapsible_region code in lib/javascript-static.js"

I tried using the print_collapsible_region_start directly, but

• I have my doubts about the usability of having just a triangle after the text instead of a show/hide title.
• it is not an option to have the window visible always when the user has no javascript, so that would need to still be worked around
• when the question bank window is opened, quizwhenbankcollapsed class needs to be added to the div with id quizcontentsblock, to make the quiz contents expand to full width

The other option then, would be just to call JS set_user_preference() from the current implementation with PHP user_preference_allow_ajax_update() added.

This does not seem much, but it is still too much for me to concentrate at this point. Shall I create a ticket about ajaxifying it?

In addition, the use cases of the question bank window suggest that collapsing and showing the window is not a frequent task at all, and thus it should not matter much if it is as slow as a new page load.

The code that was related to this was cleaned out in revision 1.2 of /mod/quiz/edit.js. Look at revision 1.1 if you want to use it for this change.

Note that in 1.2 the dialogs are also broken, 1.3 had some debugging code, so the first revision to work after 1.1 is 1.4.

Sorry, my comment was not clear, and so you read more into it than I intended.

I thought from your comment that you wanted a JavaScript function like set_user_preference to just finish something off that was nearly working. However, now that I think about it more, I see that I did not understand the comment.

"Well, in Moodle 2.0 you can do throw new moodle_exception(...); however, it should only be used for truly exceptional things, and a random question pointing to a category with no questions is does not seem that exceptional to me. We expect it to happen some times in normal operation."

What I was taught about exceptions while studying Java was that they are for raising error situations that are not necessarily fatal, but also are not part of the core operation, so exceptions provide a secondary path for functions to bubble up something about the state of the application that is not the actual data to be returned from the function.

Still some commented code I can't resolve in quiz_print_question_list() at the start of /mod/quiz/editlib.php:185

/*if (!$quiz->questions) { echo "<p class=\"quizquestionlistcontrols\">"; print_string("noquestions", "quiz"); echo "</p>"; return 0; }*/

and :292

/* // Tim: is this a separate case from the above "no questions in quiz"? Should it still be done?:
if(!$quiz->questions){ $pagecount=1; echo '<div class="quizpage"><span class="pagetitle">Page '. $pagecount.'</span><div class="pagecontent">'; print_string("noquestions", "quiz"); quiz_print_pagecontrols($quiz, $pageurl, $pagecount); echo "</div></div>"; }*/

That is, if you can confirm the above to be unneeded, you can just delete them.

I don't see any need for making 'no questions in quiz' a special case any more. I think showing a single 'Empy page' is clear enough. (Otherwise, it is a bit weird when you have a quiz with two empty pages and then delete one.)

So I have taken out that special case. Can you review my changes and let me know what you think. Thanks.