Moodle
  1. Moodle
  2. MDL-32181

calculatedmulti editform does not validate correctly multianswers option

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      the bug does not allow to create multianswers version of calculatedmulti

      1. load the two questions: one response, multiresponse moodle xml examples
      2. use preview to check that they are working correctly
      3. edit each of them and vary the grade so that it
        • does not add to exactly 100% for multianswers one ( either lower of higher )
        • there is not 100% grade option
        • add negative grade for bad responses
        • use save as a copy button to test the error mesages so that you do not destroy the original
        • save as a new question by modifying the mulianwers to a one answer and vice-versa
      4. as side tests notice any problem with using datasets
      5. take note that in the answers you should use {= {x}*3+...} syntax (no space in the formula)
        #* if {x}

        ==3 than {=

        {x}*2} will show as 6 with the formating set
        #* if {x}

        ==3 than

        {x}

        *2 will show as 3*2

        • test that the copies preview as they should
      Show
      the bug does not allow to create multianswers version of calculatedmulti load the two questions: one response, multiresponse moodle xml examples use preview to check that they are working correctly edit each of them and vary the grade so that it does not add to exactly 100% for multianswers one ( either lower of higher ) there is not 100% grade option add negative grade for bad responses use save as a copy button to test the error mesages so that you do not destroy the original save as a new question by modifying the mulianwers to a one answer and vice-versa as side tests notice any problem with using datasets take note that in the answers you should use {= {x}*3+...} syntax (no space in the formula) #* if {x} ==3 than {= {x}*2} will show as 6 with the formating set #* if {x} ==3 than {x} *2 will show as 3*2 test that the copies preview as they should
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      38930

      Description

      the message is one answer should be graded with 100%

        Activity

        Hide
        Pierre Pichet added a comment -

        I located the error in maxgrade testing that should not be there...line 340 of edit..form
        so let's do the git

        Show
        Pierre Pichet added a comment - I located the error in maxgrade testing that should not be there...line 340 of edit..form so let's do the git
        Hide
        Tim Hunt added a comment -

        Pierre, do you need any help getting this in to git?

        Show
        Tim Hunt added a comment - Pierre, do you need any help getting this in to git?
        Hide
        Pierre Pichet added a comment -

        Thanks,
        Lets say that I finally will be able to git today ....or Let's dance

        However, is there a simpletest that can be set or
        just give detailed instructions when creating and editing a question ?

        Show
        Pierre Pichet added a comment - Thanks, Lets say that I finally will be able to git today ....or Let's dance However, is there a simpletest that can be set or just give detailed instructions when creating and editing a question ?
        Hide
        Tim Hunt added a comment -

        It is quite hard to unit test the editing forms, so don't worry about simpletest. Just write some step-by-step testing instructions in the Testing instructions field of this bug.

        Show
        Tim Hunt added a comment - It is quite hard to unit test the editing forms, so don't worry about simpletest. Just write some step-by-step testing instructions in the Testing instructions field of this bug.
        Hide
        Pierre Pichet added a comment -

        Given all the complexities of such questiontypes, my first reflex now is to set
        valid simpletests and or instructions before loading github with the code solution...

        Even old teachers have something to learn

        Show
        Pierre Pichet added a comment - Given all the complexities of such questiontypes, my first reflex now is to set valid simpletests and or instructions before loading github with the code solution... Even old teachers have something to learn
        Hide
        Pierre Pichet added a comment -

        There are also some validation code from edit_calculated_form.php that need to be removed.

                    if ($answercount == 0) {
                        $errors['answer[0]'] = get_string('atleastoneanswer', 'qtype_calculated');
                    }
                    if ($maxgrade == false) {
                        $errors['fraction[0]'] = get_string('fractionsnomax', 'question');
                    }
        

        So I am building more complete testing instructions.

        Show
        Pierre Pichet added a comment - There are also some validation code from edit_calculated_form.php that need to be removed. if ($answercount == 0) { $errors['answer[0]'] = get_string('atleastoneanswer', 'qtype_calculated'); } if ($maxgrade == false) { $errors['fraction[0]'] = get_string('fractionsnomax', 'question'); } So I am building more complete testing instructions.
        Hide
        Pierre Pichet added a comment -

        The actual validation code does not handle all the more specific cases of regular multiple choice.
        So I am merging the regular code with the supplementary handling of calculated parameters.

        However as we are handling numerical data 0 could be a valid choice.
        So the use of empty() in the following lines from multiplechoice is not the valid test to do as empty(0) will test as true.

                foreach ($answers as $key => $answer) {
                    //check no of choices
                    $trimmedanswer = trim($answer['text']);
                    $fraction = (float) $data['fraction'][$key];
                    if (empty($trimmedanswer) && empty($fraction)) {
                        continue;
                    }
                    if (empty($trimmedanswer)) {
                        $errors['fraction['.$key.']'] = get_string('errgradesetanswerblank', 'qtype_multichoice');
                    }
        

        In the actual 2,2 multiplechoice, the text editor is mandatory for the answer (images are allowed).
        So if the user type 0 the editor will add <p>0</p> that will not test as empty().

        As long as the images are not implemented in calculatedmulti, we need to add != '0' to the empty() test.

        Show
        Pierre Pichet added a comment - The actual validation code does not handle all the more specific cases of regular multiple choice. So I am merging the regular code with the supplementary handling of calculated parameters. However as we are handling numerical data 0 could be a valid choice. So the use of empty() in the following lines from multiplechoice is not the valid test to do as empty(0) will test as true. foreach ($answers as $key => $answer) { //check no of choices $trimmedanswer = trim($answer['text']); $fraction = (float) $data['fraction'][$key]; if (empty($trimmedanswer) && empty($fraction)) { continue; } if (empty($trimmedanswer)) { $errors['fraction['.$key.']'] = get_string('errgradesetanswerblank', 'qtype_multichoice'); } In the actual 2,2 multiplechoice, the text editor is mandatory for the answer (images are allowed). So if the user type 0 the editor will add <p>0</p> that will not test as empty(). As long as the images are not implemented in calculatedmulti, we need to add != '0' to the empty() test.
        Hide
        Pierre Pichet added a comment -

        I am rebuilding the answer validation procedure trying to find the "good" mix between a valid numerical part if there is one and a non empty answer.
        As we are in multiplechoice even in a calculated one, one valid answer can be "none of these answers"

        I will first submit a solution for the "master" version so that you can review it before working the other versions.

        I think we should define a simpletest of the validation process(or function) to cover a legitimate part of all the combinations that can arise in the real world i.e.
        for answer empty, valid formula
        for grade none, total or max
        minimum number of valid answers (answer valid but grade can be none)

        If we had one such simpletest, this bug will not be a valid one

        Show
        Pierre Pichet added a comment - I am rebuilding the answer validation procedure trying to find the "good" mix between a valid numerical part if there is one and a non empty answer. As we are in multiplechoice even in a calculated one, one valid answer can be "none of these answers" I will first submit a solution for the "master" version so that you can review it before working the other versions. I think we should define a simpletest of the validation process(or function) to cover a legitimate part of all the combinations that can arise in the real world i.e. for answer empty, valid formula for grade none, total or max minimum number of valid answers (answer valid but grade can be none) If we had one such simpletest, this bug will not be a valid one
        Hide
        Tim Hunt added a comment -

        Hmm. Thinking about it, it should be possible to unit-test the form validation. After all, the validation() method basically takes one array as input ($data) and returns a different array as output ($errors). That is quite testable.

        Show
        Tim Hunt added a comment - Hmm. Thinking about it, it should be possible to unit-test the form validation. After all, the validation() method basically takes one array as input ($data) and returns a different array as output ($errors). That is quite testable.
        Hide
        Pierre Pichet added a comment -

        As MDL-32217 seems to be solved with just one code line (http://moodle.org/mod/forum/discuss.php?d=199258),
        I will do it and come back to this one.

        Show
        Pierre Pichet added a comment - As MDL-32217 seems to be solved with just one code line ( http://moodle.org/mod/forum/discuss.php?d=199258 ), I will do it and come back to this one.
        Hide
        Pierre Pichet added a comment -

        Tim,
        As this bug does not allowed multianswers option, I push the master on github and will give tomorrow complete testing instructions. We will build a simpletest later.
        The code pass your codechecker test OK
        If my testing is OK, I will build the other versions.
        I understand that you are on MDL-3030 ...

        Show
        Pierre Pichet added a comment - Tim, As this bug does not allowed multianswers option, I push the master on github and will give tomorrow complete testing instructions. We will build a simpletest later. The code pass your codechecker test OK If my testing is OK, I will build the other versions. I understand that you are on MDL-3030 ...
        Hide
        Tim Hunt added a comment -

        I just corrected some of the git information. What you had done was perfectly clear to let me find the code, but is not quite what is expected.

        For "Pull from Repository", it should be something like git://github.com/ppichet/moodle.git. That is, in the github interface, find the bit where it says SSH / HTTP / Git Read-Only, and copy and paste the Git Read-Only address.

        For Pull ... branch, just give the branch name (like MDL-32181), not a full URL.

        On the other hand, you just taught me something. I did not know that URLs like https://github.com/ppichet/moodle/compare/MDL-32181 worked. I though you have to do something like https://github.com/ppichet/moodle/compare/master...MDL-32181. That is really good to know. Thank you.

        Now I will look at the actual patch.

        Show
        Tim Hunt added a comment - I just corrected some of the git information. What you had done was perfectly clear to let me find the code, but is not quite what is expected. For "Pull from Repository", it should be something like git://github.com/ppichet/moodle.git. That is, in the github interface, find the bit where it says SSH / HTTP / Git Read-Only, and copy and paste the Git Read-Only address. For Pull ... branch, just give the branch name (like MDL-32181 ), not a full URL. On the other hand, you just taught me something. I did not know that URLs like https://github.com/ppichet/moodle/compare/MDL-32181 worked. I though you have to do something like https://github.com/ppichet/moodle/compare/master...MDL-32181 . That is really good to know. Thank you. Now I will look at the actual patch.
        Hide
        Tim Hunt added a comment -

        Oh and I just learned another useful thing thanks to Google. If you change the compare URL to https://github.com/ppichet/moodle/compare/MDL-32181?w=1 then it does no highlight lines where the only change is in the amount of indent, which makes this patch easier to understand.

        Show
        Tim Hunt added a comment - Oh and I just learned another useful thing thanks to Google. If you change the compare URL to https://github.com/ppichet/moodle/compare/MDL-32181?w=1 then it does no highlight lines where the only change is in the amount of indent, which makes this patch easier to understand.
        Hide
        Pierre Pichet added a comment -

        However the w=1 option does not align the text correcty.
        I had to go back to the full text to be sure that the spacing was as Ok as your codechecker tells me

        Show
        Pierre Pichet added a comment - However the w=1 option does not align the text correcty. I had to go back to the full text to be sure that the spacing was as Ok as your codechecker tells me
        Hide
        Tim Hunt added a comment -

        That looks good to me. Please complete the testing instructions and other branches, then we can get this integrated. Thank you for sorting this out.

        Show
        Tim Hunt added a comment - That looks good to me. Please complete the testing instructions and other branches, then we can get this integrated. Thank you for sorting this out.
        Hide
        Pierre Pichet added a comment -

        Tim,
        I just initially test the validation logic.
        However on completing the question and preview, there is another bug about the datasets that appears at least on master.
        So we stop this and I come back later once the other bug is cleared ...

        Show
        Pierre Pichet added a comment - Tim, I just initially test the validation logic. However on completing the question and preview, there is another bug about the datasets that appears at least on master. So we stop this and I come back later once the other bug is cleared ...
        Hide
        Tim Hunt added a comment -

        OK. Thanks for letting me know.

        Show
        Tim Hunt added a comment - OK. Thanks for letting me know.
        Hide
        Pierre Pichet added a comment -

        first problem located
        $variant has been forgotten in the call to qtype_calculated_question_helper::start_attempt() in
        qtype_calculatedmulti_multi_question
        public function start_attempt(question_attempt_step $step, $variant) {
        qtype_calculated_question_helper::start_attempt($this, $step);
        testing continue

        Show
        Pierre Pichet added a comment - first problem located $variant has been forgotten in the call to qtype_calculated_question_helper::start_attempt() in qtype_calculatedmulti_multi_question public function start_attempt(question_attempt_step $step, $variant) { qtype_calculated_question_helper::start_attempt($this, $step); testing continue
        Hide
        Pierre Pichet added a comment -

        One problem on master is related to MY implemntation of images in multichoice MDL-24594.
        On the editform the answer values are not set correctly.
        I should then go back to include images in calculatedmulti (MDL-26510) or patch temporarily master.
        There is also a problem in the validation for single answer in the actual proposal.
        more later

        Show
        Pierre Pichet added a comment - One problem on master is related to MY implemntation of images in multichoice MDL-24594 . On the editform the answer values are not set correctly. I should then go back to include images in calculatedmulti ( MDL-26510 ) or patch temporarily master. There is also a problem in the validation for single answer in the actual proposal. more later
        Hide
        Pierre Pichet added a comment -

        I have what seems to be a working version for 2,2
        I push on github this version MDL-32181_22 and I will add here an export file containing a simple and multi answers calculatedmulti question.
        Somehow, I mix old style validation with the new one, so i put it back to the actual multiplechoice syntax.

        So we have something that should work on 2,2 but we cannot put it directly on master because of a another bug.

        I let you read again closely the code, its enough for me for today

        Show
        Pierre Pichet added a comment - I have what seems to be a working version for 2,2 I push on github this version MDL-32181 _22 and I will add here an export file containing a simple and multi answers calculatedmulti question. Somehow, I mix old style validation with the new one, so i put it back to the actual multiplechoice syntax. So we have something that should work on 2,2 but we cannot put it directly on master because of a another bug. I let you read again closely the code, its enough for me for today
        Hide
        Pierre Pichet added a comment -

        Finally I found how to get it working in actual master.
        more later...

        Show
        Pierre Pichet added a comment - Finally I found how to get it working in actual master. more later...
        Hide
        Pierre Pichet added a comment -

        Tim
        Things are Ok for the three versions that have been updated on github.

        Will complete testing instructions tomorrow using mainly the attach examples that have loaded correctly on master, MOODLE_21_STABLE and MOODLE_22_STABLE

        I am figuring a simpletest of the validation function of the edit_form. The calculatedmulti could be the more complex case as there is a need to validate the multiplechoice AND the calculated. You put in a $data array and have an $error array as output. In most case the error array is a given string.

        Show
        Pierre Pichet added a comment - Tim Things are Ok for the three versions that have been updated on github. Will complete testing instructions tomorrow using mainly the attach examples that have loaded correctly on master, MOODLE_21_STABLE and MOODLE_22_STABLE I am figuring a simpletest of the validation function of the edit_form. The calculatedmulti could be the more complex case as there is a need to validate the multiplechoice AND the calculated. You put in a $data array and have an $error array as output. In most case the error array is a given string.
        Hide
        Pierre Pichet added a comment -

        How could we use the validation() function of the edit_calculatedmulti_form class without creating the form?

        Most code is static except for lines like $this->qtypeobj->

        //verifying for errors in

        {=...}

        in question text;
        $qtext = '';
        $qtextremaining = $data['questiontext']['text'];
        $possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']);
        foreach ($possibledatasets as $name => $value) {
        $qtextremaining = str_replace('

        {'.$name.'}

        ', '1', $qtextremaining);
        }

        Show
        Pierre Pichet added a comment - How could we use the validation() function of the edit_calculatedmulti_form class without creating the form? Most code is static except for lines like $this->qtypeobj-> //verifying for errors in {=...} in question text; $qtext = ''; $qtextremaining = $data ['questiontext'] ['text'] ; $possibledatasets = $this->qtypeobj->find_dataset_names($data ['questiontext'] ['text'] ); foreach ($possibledatasets as $name => $value) { $qtextremaining = str_replace(' {'.$name.'} ', '1', $qtextremaining); }
        Hide
        Tim Hunt added a comment -

        You just need to create an instance of the form in the test. You need to create enough of the right data structures to call the constructor. The only example I can see is qtype_numeric, but the tests there are rather simple.

        Note, however, that we are changing from simpletest to PHPunit, which is a similar system, but slightly different. Therefore, you should look at question/type/numerical/tests/form_test.php as an example.

        A more complex example is https://github.com/sangwinc/moodle-qtype_stack/blob/master/simpletest/testeditform.php (Note that this is still using the old simpletest.)

        Show
        Tim Hunt added a comment - You just need to create an instance of the form in the test. You need to create enough of the right data structures to call the constructor. The only example I can see is qtype_numeric, but the tests there are rather simple. Note, however, that we are changing from simpletest to PHPunit, which is a similar system, but slightly different. Therefore, you should look at question/type/numerical/tests/form_test.php as an example. A more complex example is https://github.com/sangwinc/moodle-qtype_stack/blob/master/simpletest/testeditform.php (Note that this is still using the old simpletest.)
        Hide
        Tim Hunt added a comment -

        Documentation about PHPUnit in Moodle is at http://docs.moodle.org/dev/PHPUnit. Unfortunately, getting it installed and working the first time is a real pain, but once you have done that, you are OK.

        Show
        Tim Hunt added a comment - Documentation about PHPUnit in Moodle is at http://docs.moodle.org/dev/PHPUnit . Unfortunately, getting it installed and working the first time is a real pain, but once you have done that, you are OK.
        Hide
        Pierre Pichet added a comment -

        I have something that is working in simpletest.
        The errors from validation can be multiple as
        Array
        (
        [answer[0]] => This type of question requires at least 2 choices
        [answer[1]] => This type of question requires at least 2 choices
        [answer[2]] => There should be at least one wild card in answer formula or question text
        [answer[3]] => There should be at least one wild card in answer formula or question text
        [answer[4]] => There should be at least one wild card in answer formula or question text
        [fraction[0]] => One of the choices should be 100%, so that it is
        possible to get a full grade for this question.
        )
        I solve this by using more than one test.
        $this->assertEqual($errors['answer[0]'],
        get_string('notenoughanswers', 'qtype_multichoice', 2));
        $this->assertEqual($errors['answer[1]'],
        get_string('notenoughanswers', 'qtype_multichoice', 2));
        etc.

        Is it the best way to do this ...

        Show
        Pierre Pichet added a comment - I have something that is working in simpletest. The errors from validation can be multiple as Array ( [answer [0] ] => This type of question requires at least 2 choices [answer [1] ] => This type of question requires at least 2 choices [answer [2] ] => There should be at least one wild card in answer formula or question text [answer [3] ] => There should be at least one wild card in answer formula or question text [answer [4] ] => There should be at least one wild card in answer formula or question text [fraction [0] ] => One of the choices should be 100%, so that it is possible to get a full grade for this question. ) I solve this by using more than one test. $this->assertEqual($errors['answer [0] '], get_string('notenoughanswers', 'qtype_multichoice', 2)); $this->assertEqual($errors['answer [1] '], get_string('notenoughanswers', 'qtype_multichoice', 2)); etc. Is it the best way to do this ...
        Hide
        Tim Hunt added a comment -

        That looks good.

        Show
        Tim Hunt added a comment - That looks good.
        Hide
        Pierre Pichet added a comment -

        Everything is set for testing.
        I add a line (Strict Standard : new StdClass()) in master
        The simpletest and PHPUnit will for editform be worked as a separate task as this implies also the other calculateds.

        Show
        Pierre Pichet added a comment - Everything is set for testing. I add a line (Strict Standard : new StdClass()) in master The simpletest and PHPUnit will for editform be worked as a separate task as this implies also the other calculateds.
        Hide
        Tim Hunt added a comment -

        Thanks Pierre. Submitting for integration now.

        Show
        Tim Hunt added a comment - Thanks Pierre. Submitting for integration now.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Pierre Pichet added a comment -

        I was waiting for the update
        I will include in the master version some Strict Standard corrections observed in the mean time before doing the rebase.

        Show
        Pierre Pichet added a comment - I was waiting for the update I will include in the master version some Strict Standard corrections observed in the mean time before doing the rebase.
        Hide
        Pierre Pichet added a comment -

        Done

        Show
        Pierre Pichet added a comment - Done
        Hide
        Dan Poltawski added a comment -

        Whilst reviewing this I noticed that you are using a qtype_multichoice string in the calcualtedmulti plugin:
        get_string('errgradesetanswerblank', 'qtype_multichoice')

        But I see that the plugin does also depend on the qtype_multichoice plugin being installed so I suppose this is OK, just unusual to see strings shared like that.

        Show
        Dan Poltawski added a comment - Whilst reviewing this I noticed that you are using a qtype_multichoice string in the calcualtedmulti plugin: get_string('errgradesetanswerblank', 'qtype_multichoice') But I see that the plugin does also depend on the qtype_multichoice plugin being installed so I suppose this is OK, just unusual to see strings shared like that.
        Hide
        Dan Poltawski added a comment -

        Thanks Pierre i've integrated this now.

        Just a note that I cherry-picked onto 22_STABLE rather than pulled because your branch seemed to have multiple merge commits where were not necessary. This might've been from doing a pull/merge rather than a rebase

        Show
        Dan Poltawski added a comment - Thanks Pierre i've integrated this now. Just a note that I cherry-picked onto 22_STABLE rather than pulled because your branch seemed to have multiple merge commits where were not necessary. This might've been from doing a pull/merge rather than a rebase
        Hide
        Tim Hunt added a comment -

        Dan, calculatedmulti shares a lot of code with multiplechoice. This is a good thing from a maintenance point of view, and it is one of the reasons we allowed plugins to depend on other plugins. So, sharing language strings is the least of the sharing required, but if you remember our discussions with Koen, anything that reduces the translators' work-load is good.

        Show
        Tim Hunt added a comment - Dan, calculatedmulti shares a lot of code with multiplechoice. This is a good thing from a maintenance point of view, and it is one of the reasons we allowed plugins to depend on other plugins. So, sharing language strings is the least of the sharing required, but if you remember our discussions with Koen, anything that reduces the translators' work-load is good.
        Hide
        Dan Poltawski added a comment -

        I wasn't disagreeing with any of that, just discovering

        Show
        Dan Poltawski added a comment - I wasn't disagreeing with any of that, just discovering
        Hide
        Rajesh Taneja added a comment -

        Thanks for fixing this Pierre
        Works as expected.

        Show
        Rajesh Taneja added a comment - Thanks for fixing this Pierre Works as expected.
        Hide
        Eloy Lafuente (stronk7) added a comment -
        UPDATE tracker_issues
           SET status = 'Closed',
              comment = 'Thanks!'
        WHEN participants = 'Did a gorgeous work'
        

        This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

        Show
        Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: