Moodle
  1. Moodle
  2. MDL-32217

numerical unitgradingtype parameter not correctly set on import

    Details

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

      1. Import the attached Moodle XML file.
      2. Verify that there are no errors or noticed during the import.
      3. Verify that the import was accurate. One way you can do that is to re-export the category of questions as Moodle XML, and diff the two files.

      Show
      1. Import the attached Moodle XML file. 2. Verify that there are no errors or noticed during the import. 3. Verify that the import was accurate. One way you can do that is to re-export the category of questions as Moodle XML, and diff the two files.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      38992

      Description

      the unitgradingtype parameter set to 0 on import as the related question->unitgradingtype is not used in function save_unit_options($question).
      see http://moodle.org/mod/forum/discuss.php?d=199258

        Activity

        Hide
        Pierre Pichet added a comment - - edited

        The function save_unit_options($question) part implied is the following
        the ++ shows the line to add

                $options->unitgradingtype = 0;
                if (isset($question->unitrole)) {
                    // Saving the editing form.
                    $options->showunits = $question->unitrole;
                    if ($question->unitrole == self::UNITGRADED) {
                        $options->unitgradingtype = $question->unitgradingtypes;
                        $options->showunits = $question->multichoicedisplay;
                    }
        
                } else if (isset($question->showunits)) {
                    // Updated import, e.g. Moodle XML.
                    $options->showunits = $question->showunits;
        ++          $options->unitgradingtype = $question->unitgradingtype;
                } else {
                    // Legacy import.
                    if ($defaultunit = $this->get_default_numerical_unit($question)) {
                        $options->showunits = self::UNITINPUT;
                    } else {
                        $options->showunits = self::UNITNONE;
                    }
                }
        

        Then I have to set the necessary tests.

        Show
        Pierre Pichet added a comment - - edited The function save_unit_options($question) part implied is the following the ++ shows the line to add $options->unitgradingtype = 0; if (isset($question->unitrole)) { // Saving the editing form. $options->showunits = $question->unitrole; if ($question->unitrole == self::UNITGRADED) { $options->unitgradingtype = $question->unitgradingtypes; $options->showunits = $question->multichoicedisplay; } } else if (isset($question->showunits)) { // Updated import, e.g. Moodle XML. $options->showunits = $question->showunits; ++ $options->unitgradingtype = $question->unitgradingtype; } else { // Legacy import. if ($defaultunit = $this->get_default_numerical_unit($question)) { $options->showunits = self::UNITINPUT; } else { $options->showunits = self::UNITNONE; } } Then I have to set the necessary tests.
        Hide
        Pierre Pichet added a comment -

        Doing the tests, I discover that the selection of unit with drop-down list is not detected correctly on check in adaptative mode.

        Show
        Pierre Pichet added a comment - Doing the tests, I discover that the selection of unit with drop-down list is not detected correctly on check in adaptative mode.
        Hide
        Pierre Pichet added a comment -

        I was using my french keyboard so the , was used for the numbers.
        I received the message that the unit was not selected.
        I finally understand that this message comes from numerical/question.php
        public function get_validation_error(array $response) {
        which code reproduce the steps of public function is_complete_response(array $response) {
        to select the correct message.
        This will work as long as the two functions receive the same data.
        In numerical this imply answer and unit, the call in numerical/renderer.php is
        $question->get_validation_error(array('answer' => $currentanswer)
        the $selectedunit was forgotten so I received the no unit selected message

                if ($qa->get_state() == question_state::$invalid) {
                    $result .= html_writer::nonempty_tag('div',
                            $question->get_validation_error(array('answer' => $currentanswer,'unit' => $selectedunit)), array('class' => 'validationerror'));
                }
        
        

        solve the problem.

        Should I set a separate bug as solving the MDL-32217 imply only one or 2 code lines ?
        Or create 2 bugs for master then merging them as only one for the other versions
        or ...

        Show
        Pierre Pichet added a comment - I was using my french keyboard so the , was used for the numbers. I received the message that the unit was not selected. I finally understand that this message comes from numerical/question.php public function get_validation_error(array $response) { which code reproduce the steps of public function is_complete_response(array $response) { to select the correct message. This will work as long as the two functions receive the same data. In numerical this imply answer and unit, the call in numerical/renderer.php is $question->get_validation_error(array('answer' => $currentanswer) the $selectedunit was forgotten so I received the no unit selected message if ($qa->get_state() == question_state::$invalid) { $result .= html_writer::nonempty_tag('div', $question->get_validation_error(array('answer' => $currentanswer,'unit' => $selectedunit)), array('class' => 'validationerror')); } solve the problem. Should I set a separate bug as solving the MDL-32217 imply only one or 2 code lines ? Or create 2 bugs for master then merging them as only one for the other versions or ...
        Hide
        Tim Hunt added a comment -

        It is better to proceed in small steps. If this issue can be fixed with a two-line change, then please submit a fix for this issue - that is progress - and then create a separate issue (or two) for any other changes that need to be made later.

        Show
        Tim Hunt added a comment - It is better to proceed in small steps. If this issue can be fixed with a two-line change, then please submit a fix for this issue - that is progress - and then create a separate issue (or two) for any other changes that need to be made later.
        Hide
        Pierre Pichet added a comment -

        The commit for master has been pushed on my github site.
        I realize that the procedures have changed since my last commits.
        here the link for the diff (master).
        https://github.com/ppichet/moodle/commit/2a453ebe758e7bdb0f5ed1fe22991357a9090b78
        from http://docs.moodle.org/dev/Process I should
        Use the workflow buttons in the tracker.
        You need to fill in the information about your public git repository and which branches the fixes are on.
        This would be a good time to fill in the testing instructions for how to verify your fix is correct. You may also wish to add a comment in the bug. (... or should you put that information in the commit comment?)

        However the worflow asks for all the versions this should be applied and my 2,1 STABLE amd 2.2 STABLE are not fully installed (should be tomorrow ???).

        I have to build some import files as this should be tested on numerical, calculated, and calculatedsimple.

        What do you suggest ?

        Show
        Pierre Pichet added a comment - The commit for master has been pushed on my github site. I realize that the procedures have changed since my last commits. here the link for the diff (master). https://github.com/ppichet/moodle/commit/2a453ebe758e7bdb0f5ed1fe22991357a9090b78 from http://docs.moodle.org/dev/Process I should Use the workflow buttons in the tracker. You need to fill in the information about your public git repository and which branches the fixes are on. This would be a good time to fill in the testing instructions for how to verify your fix is correct. You may also wish to add a comment in the bug. (... or should you put that information in the commit comment?) However the worflow asks for all the versions this should be applied and my 2,1 STABLE amd 2.2 STABLE are not fully installed (should be tomorrow ???). I have to build some import files as this should be tested on numerical, calculated, and calculatedsimple. What do you suggest ?
        Hide
        Tim Hunt added a comment -

        I suggest that you click the button to submit this for peer review, and just fill in the information about the master branch now. That way, I can review the change, and the way you have put it in git, and we can fix any problems - if there are any.

        Then, you can work on making the other branches tomorrow, and we can submit it for integration then.

        Does that sound like a plan?

        Show
        Tim Hunt added a comment - I suggest that you click the button to submit this for peer review, and just fill in the information about the master branch now. That way, I can review the change, and the way you have put it in git, and we can fix any problems - if there are any. Then, you can work on making the other branches tomorrow, and we can submit it for integration then. Does that sound like a plan?
        Hide
        Pierre Pichet added a comment -

        I updated and or create local MOODLE_21_STABLE and MOODLE_22_STABLE versions from which I create using cherry_picking to tranfer the quite simple changes.
        However in each of these versions(22 the 21) I just slightly modify the commit message creating another commit.
        I think I create a mess somehow ...

        I tested the three versions with one import sample which worked correctly in all cases.

        All the branches are in the site.(https://github.com/ppichet/moodle)

        I will add some other examples later.

        Show
        Pierre Pichet added a comment - I updated and or create local MOODLE_21_STABLE and MOODLE_22_STABLE versions from which I create using cherry_picking to tranfer the quite simple changes. However in each of these versions(22 the 21) I just slightly modify the commit message creating another commit. I think I create a mess somehow ... I tested the three versions with one import sample which worked correctly in all cases. All the branches are in the site.( https://github.com/ppichet/moodle ) I will add some other examples later.
        Hide
        Pierre Pichet added a comment -

        calculatdsimple moodle XML that should create a question with 2 units to select.

        Show
        Pierre Pichet added a comment - calculatdsimple moodle XML that should create a question with 2 units to select.
        Hide
        Pierre Pichet added a comment - - edited

        this sample was furnishd by Tim Lovett
        http://moodle.org/mod/forum/discuss.php?d=199258#p868748

        Show
        Pierre Pichet added a comment - - edited this sample was furnishd by Tim Lovett http://moodle.org/mod/forum/discuss.php?d=199258#p868748
        Hide
        Pierre Pichet added a comment -

        The code is Ok in all three versions (MDL-32217, MDL-32217_22 and MDL-32217_21).
        What should be the "correct" cherry-picking procedure to other versions once you have set the code in the master branch (i.e. MDL-32217) ?
        In understand that in all three versions I am ahead of the corresponding last week code.

        Show
        Pierre Pichet added a comment - The code is Ok in all three versions ( MDL-32217 , MDL-32217 _22 and MDL-32217 _21). What should be the "correct" cherry-picking procedure to other versions once you have set the code in the master branch (i.e. MDL-32217 ) ? In understand that in all three versions I am ahead of the corresponding last week code.
        Hide
        Pierre Pichet added a comment -

        I will try again to set correctly the cherry_picking.
        More news next week, have a good Easter weekend ...

        Show
        Pierre Pichet added a comment - I will try again to set correctly the cherry_picking. More news next week, have a good Easter weekend ...
        Hide
        Aparup Banerjee added a comment -

        The main moodle.git repository has just been updated (yesterday) 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
        Aparup Banerjee added a comment - The main moodle.git repository has just been updated (yesterday) 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 will do the rebase and I have found how to generate the compare view.
        like https://github.com/ppichet/moodle/compare/master...MDL-32217
        It seems that my cherry-picking was not so bad

        Show
        Pierre Pichet added a comment - I will do the rebase and I have found how to generate the compare view. like https://github.com/ppichet/moodle/compare/master...MDL-32217 It seems that my cherry-picking was not so bad
        Hide
        Dan Poltawski added a comment -

        Pierre: it looks like you sent the issue for integration review rather than peer review. I would like it if Tim was able to peer review this change before integration

        Tim: I thought i'd hold this in integration review rather than hold this issue up a week - if you are able to peer review it, that would be great.

        Thanks,
        Dan

        Show
        Dan Poltawski added a comment - Pierre: it looks like you sent the issue for integration review rather than peer review. I would like it if Tim was able to peer review this change before integration Tim: I thought i'd hold this in integration review rather than hold this issue up a week - if you are able to peer review it, that would be great. Thanks, Dan
        Hide
        Pierre Pichet added a comment -

        Sorry
        I misinterpreted the workflow as the steps are
        start development
        submit for integration
        close
        Peer review is another option that is not visually integrated in the workflow.

        Furthermore the http://docs.moodle.org/dev/Process as usual for docs need some reviewing.

        Anyway, there is no real problem that this very simple code modification is used as a lesson
        on how to work with the new multistep Process

        Show
        Pierre Pichet added a comment - Sorry I misinterpreted the workflow as the steps are start development submit for integration close Peer review is another option that is not visually integrated in the workflow. Furthermore the http://docs.moodle.org/dev/Process as usual for docs need some reviewing. Anyway, there is no real problem that this very simple code modification is used as a lesson on how to work with the new multistep Process
        Hide
        Tim Hunt added a comment -

        I have now reviewed this issue. Please integrate it.

        Pierre, thanks for the fix. Can I make a couple of suggestions?

        1. Looking at the code, I immediately had to ask myself a question: "What happens if $question->unitgradingtype is not set?" With a little thought, you can see that what happens is that in this case, the database default of 0 is used, which is UNITOPTIONAL. That seems OK. Actually, it seems very unlikely that $question->showunits will be set while $question->unitgradingtype will not.

        Anyway, the point I am trying to make is that the code would have been easier to understand if you had added

        } else {
        $question->unitgradingtype = qtype_numerical::UNITOPTIONAL;

        I just make this comment as something to think about next time you write code like this.

        2. Did you see this blog post I wrote: http://tjhunt.blogspot.co.uk/2012/03/fixing-bug-in-moodle-core-mechanics.html

        Show
        Tim Hunt added a comment - I have now reviewed this issue. Please integrate it. Pierre, thanks for the fix. Can I make a couple of suggestions? 1. Looking at the code, I immediately had to ask myself a question: "What happens if $question->unitgradingtype is not set?" With a little thought, you can see that what happens is that in this case, the database default of 0 is used, which is UNITOPTIONAL. That seems OK. Actually, it seems very unlikely that $question->showunits will be set while $question->unitgradingtype will not. Anyway, the point I am trying to make is that the code would have been easier to understand if you had added } else { $question->unitgradingtype = qtype_numerical::UNITOPTIONAL; I just make this comment as something to think about next time you write code like this. 2. Did you see this blog post I wrote: http://tjhunt.blogspot.co.uk/2012/03/fixing-bug-in-moodle-core-mechanics.html
        Hide
        Pierre Pichet added a comment -

        Thanks for the follow-up
        the $options->unitgradingtype is already set before the ifs.
        The 0 was used before the constnt was defined const UNITOPTIONAL = 0;

        $options->unitgradingtype = 0;
                if (isset($question->unitrole)) {
                    // Saving the editing form.
                    $options->showunits = $question->unitrole;
                    if ($question->unitrole == self::UNITGRADED) {
                        $options->unitgradingtype = $question->unitgradingtypes;
                        $options->showunits = $question->multichoicedisplay;
                    }
        
                } else if (isset($question->showunits)) {
                    // Updated import, e.g. Moodle XML.
                    $options->showunits = $question->showunits;
                    if (isset($question->unitgradingtype)) {
                        $options->unitgradingtype = $question->unitgradingtype;
                    }
                } else {
                    // Legacy import.
                    if ($defaultunit = $this->get_default_numerical_unit($question)) {
                        $options->showunits = self::UNITINPUT;
                    } else {
                        $options->showunits = self::UNITNONE;
                    }
                }
        

        Anyway, I register the point.

        Thanks for the blob, it would have been useful as it complements the Sam Hemelryk page that I used as reference until now...

        Show
        Pierre Pichet added a comment - Thanks for the follow-up the $options->unitgradingtype is already set before the ifs. The 0 was used before the constnt was defined const UNITOPTIONAL = 0; $options->unitgradingtype = 0; if (isset($question->unitrole)) { // Saving the editing form. $options->showunits = $question->unitrole; if ($question->unitrole == self::UNITGRADED) { $options->unitgradingtype = $question->unitgradingtypes; $options->showunits = $question->multichoicedisplay; } } else if (isset($question->showunits)) { // Updated import, e.g. Moodle XML. $options->showunits = $question->showunits; if (isset($question->unitgradingtype)) { $options->unitgradingtype = $question->unitgradingtype; } } else { // Legacy import. if ($defaultunit = $this->get_default_numerical_unit($question)) { $options->showunits = self::UNITINPUT; } else { $options->showunits = self::UNITNONE; } } Anyway, I register the point. Thanks for the blob, it would have been useful as it complements the Sam Hemelryk page that I used as reference until now...
        Hide
        Dan Poltawski added a comment -

        Thanks, this has been integrated now

        Show
        Dan Poltawski added a comment - Thanks, this has been integrated now
        Hide
        Andrew Davis added a comment -

        Worked as described in 2.2 and 2.1. In master the diff matched but, for some reason, export wouldn't give me a complete xml file. Kept getting clipped before the end of the file. What I could get matched perfectly and I dont think the export issue is related to this bug. Passing.

        Show
        Andrew Davis added a comment - Worked as described in 2.2 and 2.1. In master the diff matched but, for some reason, export wouldn't give me a complete xml file. Kept getting clipped before the end of the file. What I could get matched perfectly and I dont think the export issue is related to this bug. Passing.
        Hide
        Tim Hunt added a comment -

        I wonder if the Strict syntax changes are somehow breaking export in master? Oh well. Let us assume that it will be fixed before the end of QA testing.

        Show
        Tim Hunt added a comment - I wonder if the Strict syntax changes are somehow breaking export in master? Oh well. Let us assume that it will be fixed before the end of QA testing.
        Hide
        Pierre Pichet added a comment -

        I just try on last week master and the xml seems OK
        <?xml version="1.0" encoding="UTF-8"?>
        <quiz>
        <!-- question: 0 -->
        <question type="category">

        </question>

        <!-- question: 36 -->
        <question type="calculatedsimple">
        ...
        </answer>
        <unitgradingtype>1</unitgradingtype>
        <unitpenalty>0.1000000</unitpenalty>
        <showunits>2</showunits>
        <unitsleft>0</unitsleft>
        <units>
        <unit>
        <multiplier>1</multiplier>
        <unit_name>m2</unit_name>
        </unit>
        <unit>
        <multiplier>1000000</multiplier>
        <unit_name>mm2</unit_name>
        </unit>
        </units>
        ....
        </question>

        <!-- question: 37 -->
        <question type="calculatedsimple">
        ....
        </question>

        </quiz>

        Show
        Pierre Pichet added a comment - I just try on last week master and the xml seems OK <?xml version="1.0" encoding="UTF-8"?> <quiz> <!-- question: 0 --> <question type="category"> </question> <!-- question: 36 --> <question type="calculatedsimple"> ... </answer> <unitgradingtype>1</unitgradingtype> <unitpenalty>0.1000000</unitpenalty> <showunits>2</showunits> <unitsleft>0</unitsleft> <units> <unit> <multiplier>1</multiplier> <unit_name>m2</unit_name> </unit> <unit> <multiplier>1000000</multiplier> <unit_name>mm2</unit_name> </unit> </units> .... </question> <!-- question: 37 --> <question type="calculatedsimple"> .... </question> </quiz>
        Hide
        Dan Poltawski added a comment -

        Jolly good show!

        Your changes have made it into the Moodle release - its time to celebrate! I suggest a hot cup of English tea (with milk, no sugar) or a hoppy English ale.

        Tally-ho!

        Show
        Dan Poltawski added a comment - Jolly good show! Your changes have made it into the Moodle release - its time to celebrate! I suggest a hot cup of English tea (with milk, no sugar) or a hoppy English ale. Tally-ho!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: