Create function to repeat answers in question_edit_form and convert relevant qtypes to use it

Tim, this is a main patch which modifies all related qtypes except calculated (since calculated needs some additional work). You may want also add qtype_missingtype.php to language pack if you are feel puristic about strings in missing type.

Would you like to apply calculated changes too (I may add them to main patch, or create additional), or you want to leave it to Pierre? Pierre seems to basically agreed with the change, but doesn't have time right now to work with them.

Is this patch in need of 2.0 version? Forms doesn't seems to change much in there (except files handling, but it isn't affected).

I don't know when I will get time to look at this, but hopefully soon.

Having looked this patch, it just does not feel right.

I'm sure you are right. This needs to be refactored, to it is easier to inherit from. Just not like this.

And I realise that I can't just say "Don't do it that way" without saying what I think a better way is. But I am struggling to come up with something that feels right.

I think we should leave matching question type out of it for now. It is too different.

What do you think of the attached patch (repeated_answers.patch.txt)? I think it is a better approach.

"Having looked this patch, it just does not feel right. " - yes, I feel uneasy about that stuff myself.
"What do you think of the attached patch (repeated_answers.patch.txt)? I think it is a better approach. "
Hmm, approach seems good; only some details not so:
1) (most important one) you leave answerinstruct aside scope of you patch; however this is one of the main reason I started this: ability to inherit form definition from shortanswer, but replace answerinstruct with another one (and adding new options fields before calling parent). It is important for inheritability. I guess add_per_answer_fields should contain answerinstruct code, so any child can replace answerinstruct without replacing add_per_answer_fields call.
2) I taked some pain to preserve original formatting; you take less - most important case is numerical, it used a shorter string for an answer than shortanswer (and others); you patch will make it longer - i'm not bothered much about this personally, but don't know was this intentional or omitted side-effect
3) missingtype use same pattern as multichoice, but doesn't included in you patch - is this intentional, or you just want patch smaller as example?
4) "I think we should leave matching question type out of it for now. It is too different." - I think match can benefit from add_per_answer_fields, while completely rewriting get_per_answer_fields; this'll require an additional argument for add_per_answer fileds, something like $answersoptionname='answers', which in match will be set to 'subquestions'.

What you opinion on points above?

I may rewrite you patch knowing you decision on points above, if you so desire.

Thank you for your comments.

I was mainly trying to get a patch finished the showed the general idea of what I was suggesting so you could comment, rather that get everything perfect. (And I was trying to get it finished before going home on Friday evening )

1. I intentionally left answerinstruct outside add_per_answer_fields, so subclasses could reuse add_per_answer_fields with whatever instructions they liked. Is the point that you want to reuse definition_inner?

2. I overlooked this. But I think in question_edit_numerical_form:: get_per_answer_fields, we can do

$repeated[1]->setSize(10);

or whatever.

3. I overlooked this in my example.

4. OK, you are probably right about this.

If you could do another patch, that would be great. Thanks.

I'll try to find time to rewrite a patch in several days. Today I spent 9 hours with students, almost non-stop. 8-[

1. Yes, I want to reuse definition_inner. With answerinstruct code I could write something like this:
class question_edit_preg_form extends question_edit_shortanswer_form {

function definition_inner(&$mform)

And with current state of things I can't do that for no other reason than .... wrong answer instruction. :-/

I don't know how my implementation of the answerinstruct can restrict subclasses (while it certainly gives more freedom to sub-subclasses). It only say it must have specific string name (which is common for parent implementations of forms in Moodle), but it understood correctly if it's ommitted (this suits well all current forms). If some subclass want more freedom it can omit answerinstruct and place something another before call to add_per_answer_fields.

So I hope leaving answerinstruct part in patch is OK.

I think you can do more than you think. If you look at the definition of the $mform class, you will see that there are methods insertElementBefore and removeElement, so you could always write code like

function definition_inner(&$mform)

But if you want to handle this some other way, you are welcome to.

Tim, I'm writing next patch. Things to consider:

0. match, missingtype and calculated (with gracious permission from Pierre) added to the patch

1. I removed QUESTION_NUMANS_START from max in add_per_answer_fields, setting it as default value for $minoptions; also added $addoptions to overwrite QUESTION_NUMANS_ADD if needed. This most objectionable part of the thing was written due to Pirre objections ( http://tracker.moodle.org/browse/MDL-17851?focusedCommentId=63828&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_63828 ). Calculated form starts from 1 answer and add 2, and it has reasons to do it. To preserve functionality I edited multichoice (the only question using $minoptions) to pass there max (5, QUESTION_NUMANS_START).

2. there was a bug with setType function for subquestions/subanswers fields: it used subquestion instead of subquestions (and similar to subanswers). Hovewer, a type to subquestion must be considered carefully - ppl may want HTML there (and it is outputted as HTML now), but setType set it as PARAM_TEXT, so fixing a bug may leave ppl without a feature actually. Leaving actual type of subquestions field to you consideration.

3. name of the field int $question->options to count is returned from get_per_question_fields, so set_data/validate potentially can get it too....

Hmm, can't find a link to edit my comment - 2. in comment above deals with match qtype

I've also pass $repeatedoptions to get_per_answer_fields to allow to modify it - needed for calculated. I'm wondering, is using setType for repeated fields correct? Should it not be replaced with $repeatedoptions thing?

"I'm wondering, is using setType for repeated fields ".
I think you're wright , the setType does not repeat to all elements.
I will try to find where I discuss this...

MDL-12664

Pierre - setType probably should not work for array, there are $repeatedoptions to set type, defaults and so on for repeated elements (so MDL-12664 is probably not a bug) - but this is violated in several questions using repeat_elements. I'll fix all occurences of setType for repeated elements in a new patch.

P.S. In improving of calculated you may consider using Runkit_Sandbox class from runkit package (php 5 only). It can allow you to create sandboxes to execute not simple formulas, but an arbitrary php code supplied by user without any harm, which can make you possibilities almost limitless.

I have had to attach two patches so they could be applied right away because Pierre refused to merge changes answers->answer in 1.9 before Tim's decision (and Tim probably will want apply patches right after decision will be made), so the patches for 1.9 and 2.0 become different.
(Pierre, if Tim will apply patches above you should not merge MDL-18034 in 1.9)

Revised patch features:
1. calculated, match, missingtype, multichoice, numerical and shortanswer qtypes converted
2. fixed issues with setType use on repeated elements in several qtypes; not actually setting filters may be considered a bug (WARNING: Tim please consider subquestions type in match - in code it is setting in PARAM_TEXT, but this was'nt worked due to improper setType use and there was possibility to enter (and see results) HTML code there - and now it is fixed. Maybe PARAM_RAW will be a better thing)
3. I placed fraction (i.e. grade) before tolerance and other things in calculated to improve consistency with numerical; Pierre - it can be undone by replacing 3 to 2 in array_splice call on line 39
4. by the way I rewrite overall feedback fields in multichoice using foreach to get less code

I read your comments, but don't feel the need to respond to any of it. I just look forwards to seeing the patch.

How about Contrib question types?
Will they continue to work or will need immediate modifications as the main edit_question_form.php is changed?
Remember that when we do the main changes to the new edit_question_form.php there was a transition code that detect the new edit_questionform.php.

They will continue to work. But they will continue to have duplicated code that could be eliminated by calling the new methods.

Pierre, are you happy with the parts of this that affect the calculated question type? Are you happy for me to commit this?

I can live with this but need to test it.
However my current set-up does not use patch to change the code.
So if Oleg could attach here a zip containing at least the HEAD codes, that will be useful.

Pierre
"How about Contrib question types? Will they continue to work ..." oh, if they would not continue to work that would indeed be the first time in software development when adding new functions will break inherited classes. Even if you don't use patch to change files, patch files are very easy to read to see what will be changed in base and you classes. (And I never insists on compatibility break changes in stable versions, such things is madness).

"So if Oleg could attach here a zip containing at least the HEAD codes, that will be useful." - Oh, I forgot about that. HEAD zip will be available shortly. Pierre, actually patch tool is very easy to install and use (are you working on Windows?). I made it yesterday first time using instructions from Moodle Docs, and doesn't encounter any problems at all.

Tim - I may add Joseph as a watcher there so he may consider upgrading his question. I think contrib should done it in 2.0 only to have landmark for users with which versions of Moodle their code will work.

"think you can do more than you think. If you look at the definition of the $mform class, you will see that there are methods insertElementBefore and removeElement, so you could always write code like " - I've handle it even simpler with setText for answerinstruct; but encountered a strange bug with get_string returning an empty string instead of instructions...

Pierre - this archive (repeat_answers20.zip) contains whole type subdirectory of question directory of Moodle. It's for 2.0 version.

Looking at the code patch more closely, it seems to be transparent i.e. will give the same effect as the actual code.
However I prefer to leave the ordering of grade just below answer fromula text as it is actually, as it will be somewhat hidden if it is put after all the tolerance parameters.
So " replacing 3 to 2 in array_splice call on line 39 " as suggested by Oleg.

OK to commit.

P.S. The 1.9 calculated version has not been merged for answers to answer conversion.(MDL-18034)

Right, well tomorrow is testing day, so I will commit these on Wednesday. (If I remember )

Pierre - to have grade just below answer formula you need to preserve 3.

Oleg, thanks for the zips.
At first sight everything seems OK. including grade after formula.

Test (on calculated 2.0 i.e HEAD ) various parameters , adding answers, units.
Everything seems OK.
Good work Oleg

Sorry, I put the new files at the wrong place...
Things put at the rigth place
The display is OK but I need to update to last HEAD to conclude .

Hi, Tim
"Right, well tomorrow is testing day, so I will commit these on Wednesday" - please don't forget to decide type of "subquestions" fields in match question before committing. I leave it as it was intended in code PARAM_TEXT, but this may change current behavour. PARAM_RAW will probably better save current behavour. Don't know, why it was intendet to be PARAM_TEXT - ability to enter HTML there may be useful to someone.

if (isset($question->answer) && !isset($question->asnwers))

in the calculated question type is a bit of a hack, but I've seen worse. It can be fixed properly later.

tim@tim:~/Desktop$ diffstat crud.patch.txt
calculated/edit_calculated_form.php | 70 ++++++++++++++--------------------
calculated/questiontype.php | 8 ++-
edit_question_form.php | 50 ++++++++++++++++++++++++
match/edit_match_form.php | 34 ++++++----------
missingtype/edit_missingtype_form.php | 25 ------------
multichoice/edit_multichoice_form.php | 38 ++----------------
numerical/edit_numerical_form.php | 46 ++++++----------------
shortanswer/edit_shortanswer_form.php | 26 ------------
8 files changed, 122 insertions, 175 deletions

Anyway. that is more than 50 lines of code saved. I like.

Thanks Oleg and Pierre for your efforts. Checked in now.

(Note, I made some minor tweaks, including the matching PARAM_RAW thing. Also, the 2.0 patch is missing the changes in question/type/calculated/questiontype.php.)

2.0 patch is missing the changes in question/type/calculated/questiontype.php.because I have already CVS them (MDL-18034).
" if (isset($question->answer) && !isset($question->asnwers))

in the calculated question type is a bit of a hack, but I've seen worse. It can be fixed properly later. "
This is because $question->answers exist already at other places in the code and I just realize at looking at the code that this "hack" does not fix all occurences.
I will work on this today and fix potential problems.

The calculated code is built around the convention that the array is plural
i.e $answer = answers[i] ;
This convention is not used in the forms .
We create an 'answer' element although on the form it is part of an array as answer[0].
Ideally I should convert all answers to answer in calculated/questiontype, datasetitems_form and datasetdefinition_form and check in format files.

"Ideally I should convert" should be read as I will work on this today (tonight!) as all occurences of form->answers had not been corrected.

Tim, "Also, the 2.0 patch is missing the changes in question/type/calculated/questiontype.php" - those changes was checked in by Pierre (see commits on MDL-18034). Seeing diffs more closely it seems that Pierre just made this change in another place.

Also there is a typo in my patch in if (isset($question->answer) && !isset($question->asnwers)) {
(asnwers instead of answers), thought it not really hurting anything so it was not found during testing (malicious dynamic typing in PHP (( ).

Pierre - "Ideally I should convert all answers to answer in calculated/questiontype, datasetitems_form and datasetdefinition_form " - I did not found any $question->answers occurences in calculated/questiontype.php other than in save_question_options, and nothing at all in datasetitems_form. There is one relevant occurence in datasetdefinition_form on the line
foreach ($SESSION->datasetdependent->questionform->answers as $answer){
that probably need to be fixed.

The hard part is format files (and I don't know if there are other possible sources of $question objects).
Note that $question->options->answers should remain unchanged.

Sorry Pierre, I should have noticed that and tested more before committing. If you need any help from me tomorrow sorting this out, add a comment here and I will take a look.

Import formats is not broken - I've tested it, retest now - all works. It would need changes only in case you want to delete "hacked" code from calculated/questiontype.php
There is nothing else in questiontype.php that look related to this renaming.

The only things that really was broken is datasetdefinition_form.php, line
foreach ($SESSION->datasetdependent->questionform->answers as $answer){
should probably by fixed to
foreach ($SESSION->datasetdependent->questionform->answer as $answer){
Pierre, could you test this please?

Also there is a typo mentioned above, thought it not really break anything. Typo was in my patch, sorry.

Effectivley there is a typo
if (isset($question->answer) && !isset($question->answers))
that I will correct.
I have done other checking using print_r, the hack is working in editing a question because in type/questiontype.php
this is $ that is passed to save options
$result = $this->save_question_options($form);
and is calculated/questiontype the hack is to the $form
so that in abstractype.php
if (empty($form->id)) { // for a new question $form->id is empty
$question = parent::save_question($question, $form, $course);
//prepare the datasets using default $questionfromid
$this->preparedatasets($form);
$this->preparedatasets($form) has the two variables i.e

[answers] => Array
(
[0] =>

[1] =>
)

so the following code works.
There is no hurry to change all answers but this should be done.

Pierre, we probably must move discussion about answers->answer renaming in MDL-18034, adding Tim as a watcher there.
What do you think of foreach in datasetdefinition_form.php mentioned above? You are better know this code.

"foreach ($SESSION->datasetdependent->questionform->answers as $answer){ "
This come from an old code part that has not be cleaned.
Calculated needs some "spring" cleaning but there is no hurry .

Tim, I don't understand you changing
repeatedoptions['answer']['type'] = PARAM_NOTAGS;
to
$mform->setType('answer', PARAM_NOTAGS);

setType obviously not intended to work with arrays of repeated elements (see Pierre's MDL-12664 and our discussion with him above).

I don't understand either. It is a mistake. Which file is that?

Tim - "Which file is that? " it's edit_calculated_form.php Found this merging my files with HEAD.
Also questiontype.php of calculated question now contains two copies of "hacked" strings in save_question_options - one added by Pierre, another by you. Then one from me/you contains typo, but it better comply with 1.9 version. (Typo in 1.9 was corrected by Pierre).

Pierre - "Calculated needs some "spring" cleaning " - maybe it would be wise to create an meta-issue where you and others may write ideas about such cleaning. So they would not be forgotten.

Oleg and Tim ,
"two copies of "hacked" strings in save_question_options - one added by Pierre, another by you. Then one from me/you contains typo, but it better comply with 1.9 version. (Typo in 1.9 was corrected by Pierre)."
The "hacked" was corrected as you ask me in MDL-18034 before Tim do your patch.
I just corrected the typo in HEAD questiontype.php

Oleg,
"Calculated needs some "spring" cleaning " - maybe it would be wise to create an meta-issue where you and others may write ideas about such cleaning. So they would not be forgotten."
I am waiting to clean old code parts in HEAD until I have finish the simple calculated project as the two question types will use as much the same code as possible.
For older versions like 1.9, the old code does not give any errors so it is not worth the cleaning job as there was a somewhat important changes from 1.9 to HEAD in order to eliminate datasetdependent question type.

I have some problem on CVS download to the last HEAD as there was lot of code changes that Tortoise does not handle well and there are new requirements about PHP 5.2.... version that necessitate a new PHP installation.
So I cannot test correctly in HEAD until everything is fixed.
Sorry

As for the actual 1.24 HEAD version (after Tim CVS) of edit_calculated_form.php the code is OK.

There remain set-Type for unit as in numerical that perhaps should be changed...

$repeated[] =& $mform->createElement('text', 'unit', get_string('unit', 'quiz'));
$mform->setType('unit', PARAM_NOTAGS);

$repeated[] =& $mform->createElement('text', 'multiplier', get_string('multiplier', 'quiz'));
$mform->setType('multiplier', PARAM_NUMBER);

As I work with Tortoise , it CVS the actual code on my HEAD or 1.9 testing site so there is no patch created and I always check the changes before doing the CVS just in case the HEAD or 1.9 moodle have changed or if forgot some print_r or other tests....

Is there a way that you can set another sites (mirrors of actual HEAD or 1.9) to which you apply your patch and test on these sites that everything is OK?

Pierre, you can trick 2.0 into installing or upgrading by editing admin/environment.xml to say it requires 5.2.6 or whatever you have, instead of 5.2.8. Most things will work.

The best way to test what a CVS commit will do is to make a diff and look at that. There is no way to use a test server.

Pierre,
"As for the actual 1.24 HEAD version (after Tim CVS) of edit_calculated_form.php the code is OK. " - it is not, see get_per_answer_fields function, it contains
$mform->setType('answer', PARAM_NOTAGS);

"Is there a way that you can set another sites (mirrors of actual HEAD or 1.9) to which you apply your patch and test on these sites that everything is OK? " - you can easily set (any number of) testing site on you local machine (only localy available) using Denwer package if you work under Windows. You can create mirrors of both db and site by just copying files on you hard drive.

"The best way to test what a CVS commit will do is to make a diff and look at that. There is no way to use a test server. "
Tim, I don't understand this... I've tended to think that using a test server setted up on local computer (with code directly checked in from CVS, so that you can commit/create patch from it) is widespread practice. For those on Windows there are programs to get such setups with no experience of configuring Apache/MySQL/PHP at all.

Or I'm not understand at all about what you and Pierre are talking at all.

Oleg and Tim
Should I change in edit_calculated_form.php
$mform->setType('answer', PARAM_NOTAGS);
to
repeatedoptions['answer']['type'] = PARAM_NOTAGS;
or at least
$repeatedoptions['answer']['type'] = PARAM_NOTAGS;
Oleg, there was another typo problem on your patch.

This is why I ask YOU

Is there a way that YOU can set another sites (mirrors of actual HEAD or 1.9) to which you apply your patch and test on these sites that everything is OK?
I understand that Tim have such set-ups with for example various databases and or moodle versions.

I dont know about your set-up but these typos mean that either you were in a hurry or that insufficient tests were done.

This concerns me a lot if the patches are to be applied to a STABLE version like 1.9.

Although we should have the same high standards for HEAD, putting new code in HEAD give us more time to test.
I do personnaly think that sometimes developpers are putting on HEAD code that is either very experimental or not sufficiently tested.

Improving code effectiveness have not the same urgency as solving bugs.

But this is also what we call in french "La fougue de la jeunesse"

Tim, thanks for the tip about PHP.

One more comment
function get_per_answer_fields(&$mform, $label, $gradeoptions, &$repeatedoptions, &$answersoption) {
$repeated = parent::get_per_answer_fields(&$mform, $label, $gradeoptions, $repeatedoptions, $answersoption);
$mform->setType('answer', PARAM_NOTAGS);

was in the 2.0.zip file you put on MDL-18035 and that tested correctly before I decide to update to the last HEAD version with a lot of merging conflicts and PHP problems that I should solve this week.

"dont know about your set-up but these typos mean that either you were in a hurry or that insufficient tests were done. "

Pierre - I did have my test site right on my computer. The main cause of $question->asnwers typo unfound is malicious nature of PHP with it's dynamic typing - such things are much less obvious in languages with static typing (such as C++, Java). If you look at the code carefully, you'll see that this is a sort of typo that is very hard to notice while testing because it almost affects ... nothing (actually I found it during text searching). The only case you may notice it in testing is if there will be both $question->answer and $question->answers fields set with different values before call to save_question_options - and | personally don't know any circumstances under which it can happen. Did you know any such testing case? I spotted and eliminated some typos that really did have an effect while testing this patch.

What is another typo you talking about?

"This concerns me a lot if the patches are to be applied to a STABLE version like 1.9. Improving code effectiveness have not the same urgency as solving bugs. " - oh, improving code effectiveness was just the trick to get this into the core - this particular improvment interested me personally as it's opens a way to do some modifications (which should be our local modifications before 2.0 as we agreed with Tim - they are too dangerous to be included in stable version or breaking compatibility) which improve Moodle perfomance under some circumstances, which our teachers wants greatly. And 2.0 been very slowly in coming doesn't help. I still have to persuade our superiors to select Moodle, and such things helps a lot. Some people there come to really hate doing question editing in Moodle.

I did agree that this thing was somewhat adventurous at least and I will be more careful in the future.

"was in the 2.0.zip file " - oh , yes - now I understand. I found this thing after sending you zip and update patches there, but Tim probably used old versions. You can see this in change history. Those setTypes actually not very noticeable in testing too - they were in Moodle core from 1.8 and blissfully escape detecting or fixing before this (and this was not by my carelessnes really).

"I will be more careful in the future. "

I have put back on PHP 5.2.4 editing admin/environment.xml .
calculated HEAD seems to work correctly on preliminary tests.