Moodle

The references in question bank for paging (page 1,2 ,3 NEXT, SHOW ALL) don't have a return url so the link does not work due to the filtering of valid addresses.
also
Notice: Undefined variable: paging in C:\moodle\moodle\site\moodle_head\question\editlib.php on line 1303
which invalidates the related code

if ($paging) { $paging = substr($paging, 0, strrpos($paging, '</div>')); $paging .= "<br />$showall</div>"; } else { $paging = "<div class='paging'>$showall</div>"; }
echo $paging;

I set as critical as there are other links also to question/edit.hp that do not contain a valid returnurl
as on the upper link "Edit questions" when you are editng a question.

Sam, saw your recent commit for this bug. I just wanted to point out that, in a bit of code like

+    $url = new moodle_url($CFG->wwwroot.'/mod/quiz/review.php', array('attempt'=>$attemptid));
+    if ($page !== 0) {
+        $url->param('page', $page);
+    }
+    if ($showall !== 0) {
+        $url->param('showall', $showall);
+    }
+    $PAGE->set_url($url);
+
     $attemptobj = new quiz_attempt($attemptid);

it might be easier to set $PAGE->url after you have created $attemptobj, because there is a $attemptobj->get_review_url() method.

Of course, that returns one string, not a moodle_url or a string and array, so perhaps you should leave it and I will sort things out later.

Anyway, thanks for working on this.

Now extends settings navigation, perhaps if you get time and feel the desire Tim you could find a cool way to extend the main navigation.

Reopening this, since it seems like the most natural bug to use for improving the quiz navigation.

Hi Tim,

Have attached a patch that I believe meets the goals of http://docs.moodle.org/en/Development:How_the_quiz_navigation_should_work_in_Moodle_2.0

If you could find a couple of minutes to check it out for me that would be most appreciated.

Cheers
Sam

Lots of great work there, but ...

+    public function show_only_fake_blocks($setting=true) {
+        $this->fakeblocksonly = true;
 }

Should use $setting, not true, and seems to have a whitespace problem with the }

moodle/lib/navigationlib.php @@ -2685,23 +2692,10 @@
Should not require_once($CFG->dirroot.'/question/editlib.php'); surely it only needs questionlib.php. If that is not the case, fix it so that it does only need questionlib.php

question_extend_settings_navigation copies-and-pastes way too much code from somewhere. In particular, using optional param there is just evil. I think that question_extend_settings_navigation should only take a $context or $contextid parameter, in addition to $questionbank.

I also think that $questionbank is a bad name for this argument. It is a navigation node, not a question bank.

$PAGE->set_pagelayout('admin'); in mod/quiz/edit.php must be wrong. (It may possibly be that admin layout is badly named, or it may be this line that needs to change.)

$PAGE->set_title($pagetitle.": ".$quiz->name); in mod/quiz/edit.php. Why are we concatenating strings like this?

Don't require questionlib.php at the top of mod/quiz/lib.php. That includes all sorts of crap, and so hurts performance. Do it in quiz_extend_settings_navigation

Hmm. That makes me wonder whether question_extend_settings_navigation should be in questionlib.php, or somewhere else like navigationlib.php

$PAGE->set_pagelayout('standard'); in question/category.php (and a couple of other places). Is that really necessary? Why isn't that the default?

+        $filename = $CFG->dirroot."/mod/$cm->modname/tabs.php";
+        if (file_exists($filename)) {
+            include($filename);
     }

(in various questionbank scripts) Why did you keep that bit? I vote for just getting rid of it.

(So far, I have just reviewed the code. I have not yet applied the patch and tested it. Having found the above issues, I will wait for a second patch before testing.)

Thank you very much for doing this.

Hi Tim,

Thanks for looking at that patch for me, the following changes have been made in the patch that I have attached:

1. Have changed $this->fakeblocksonly = true; to use $setting. The whitespace issue is a flaw in netbeans ability to produce a good quality patch. When it gets commit, it gets commit fine, but for some reason the patch tool exports whitespace issues like that.
2. editlib.php is required because of the question_edit_contexts class. After a quick check turns out it can be moved from question/editlib.php to lib/questionlib.php without requiring any changes to require calls, which is what I have done.
3. optional params removed from question_extend_settings_navigation, renamed questionbank to questionnode and now takes a context rather than cm or course.
4. $PAGE->set_title($pagetitle.": ".$quiz->name); ... because we can ... Haha nah, it was a foobar anyway as I wanted to set that as the header not the title. I did a grep on the quiz module and saw there were only two pages that were setting heading, overridedelete was using a topical string and view.php was using the course fullname. In the interests of standardisation (several other modules use course fullname everywhere) I have changed edit.php to the course fullname and set a heading on all applicable pages in the quiz module. Let me know if you would like something different to be used.
5. Moved the requiring of questionlib within mod/quiz/lib.php into the quiz_extend_settings_navigation function.
6. The inclusion of a tabs.php file was left in for scripts that were already calling it purely for backwards compatibility encase anyone out there has successfully integrated a module with the questionbank. I've now removed it as I wasn't that partial to it anyway let me know if you think it is worth keeping in regards to backwards compatibility.

The following points are open for discussion at this point and have not been acted upon yet:

1. $PAGE->set_pagelayout('admin'); is required for lack of a better option, side-post blocks clutter the interface something terrible and the admin layout is the only layout that has side-pre but not side-post regions. I'm open to suggestions for this one? I don't want to create a layout just for the quiz module, perhaps if there is a smart generic name that won't be abused elsewhere (I immediately thought form though most forms are fine with blocks on both sides).
2. I've left question_extend_settings_navigation within questionlib for the time being, to me that feels correct as it is where I would expect to find it... but then I work with this on nearly a daily basis. If you do decide you would like it moved let me know.
3. The default pagelayout is `base` hence the calls to set the pagelayout to standard. I forget why base is the default and not standard. Petr had a good reason for it at the time as to why pages should not have blocks by default but I can't remember the details. Perhaps this could be raised again at a dev meeting.

Let me know your thoughts

Cheers
Sam

Ohhh I keep forgetting to mention, the ordering that you requested for the quiz settings block hasn't been achieved. Currently there is no way to insert nodes above default nodes that the navigation takes the liberty of adding.
I am thinking that such a task wouldn't be too big but I would like to tackle it as a separate tracker issue after this has been commit.

Cheers
Sam

On the discussion points:

1. Thanks for explaining. I think we should try to come up with a better name.

2. I bow to your greater wisdom on this. Thanks for thinking about it.

3. I would like to see the arguments on this. It does not make sense to me. Perhaps it would be interesting to do some grepping and do a frequency analysis of what is passed to $PAGE->set_pagelayout(). That might answer the question of what the best default is.

4. (the separate comment) You can argue it both ways. It is good leave the standard links in a consistent order. It is also good to group the links for each activity as logically as possible. The one that seems most desirable to me is having settings overrides next to settings. However, if it is impossible, it is impossible, and that is OK for now.

I'll try to review the new patch later today.

Removing the top level tabs is okay since they are not tabbed navigation anyway.

However, "edit" and "order and paging" are rapid switching between alternative views of the same information object (Nielsen*) - the UI is modal, and modes are classically really difficult for users unless they are made visible. Tabs are good at making them visible. Removing them and placing them as links in a list just really really breaks the UI even more than the space taken by the blocks on the left will.

See also http://moodle.org/mod/forum/discuss.php?d=149375#p658375 .

Please, please leave the two tabs above the UI, unless there is a real justification for removing them that is based on user goals. Is there?

• * http://www.useit.com/alertbox/991114.html

If you wish, I can look for more justification in literature for why tabs as a UI element works so well for modal display of the same data. Anything to not have half the Quiz UI redesign work flushed down the drain.

Good point. I was not happy with having edit questions and order and paging as sub-links in the settings block. Keeping two tabs at the top of the edit page, with a user preference so that the mode you last used is the default, seems better to me.

Sam, is it OK to make that change?

Hm, in fact yeah, now that the two tabs are no longer subtabs, it seems less of a problem if the tabs are persistent, since at least there is no consistency problem within the same set of tabs.

OK. I think this is a big step forwards, and after correcting the minor points below (and the two tabs on the edit page, if possible) we can commit this, to get another round of comments.

(Really picky coding style point. I think the rules are space round operators like $x == $y.)

Should question_extend_settings_navigation create the question node, as well as its children? And should the node itself be a link to save you expanding that bit of the tree in the common case?

I think Overrides should be Settings overrides.

Question Bank management should be just Question bank. Similarly, I think the top-level Questions should become Question Bank on the course page, and should be a link.

I think that during a preview attempt/review, the settings navigation should always be visible. And, the only realistic way to do that is to show blocks in this case. (In other words, the Show blocks during attempt: No only applies to non-preview attempts.)

Thanks again.

Hi guys,

I'm resolving this issue now, I have made the requested changes as follows and have commit the patch, CVS update and you will see all of the changes live.

Changes made before commit:

• Corrected the couple of places where spacing wasn't correct
• question_extend_settings_navigation now created the branch node which is now labelled `Question bank` (It is also linked)
• Overrides now Settings overrides
• Blocks are not shown only for actual attempts.
• The edit page has two tabs now as requested.

Cheers
Sam

Done

Yay! Thanks Sam.

Thanks Sam!

I am pretty happy. The persistence of tabs apparently didn't make it though?

A minor sidenote: "Editing quiz" sounds to me like it's a quiz, a property of which is editing. Could you shed light on why not "Edit quiz"?

Oh, I really grave to work on the layout of especially the order and paging tab, it's horrendous and that's my fault. Now with less room the repaginate clashes with "The basic ideas of quiz-making" badly.

Also, after having sat on the question for over a year, I am apparently less in love with my brainchild now and start to agree that the dialog for adding random questions probably should have choosing from existing question categories as the default option. /me bangs head on wall