Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36760

Changes to frozen elements in MDL-30845 broke form validation (was Unit handling for numerical type questions does not work)

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2.6, 2.3.3
    • Fix Version/s: 2.2.7, 2.3.4
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a numerical question that has the option to use units enabled.

      2. Given that we already missed one regression, consider whether we should do more of the testing that was outlined at http://tracker.moodle.org/browse/MDL-30845#bonfiremodule_heading. Indeed, we may even want to repeat some. For example, it claims there that question editing forms were tested, but that is where this regression was.

      Show
      1. Create a numerical question that has the option to use units enabled . 2. Given that we already missed one regression, consider whether we should do more of the testing that was outlined at http://tracker.moodle.org/browse/MDL-30845#bonfiremodule_heading . Indeed, we may even want to repeat some. For example, it claims there that question editing forms were tested, but that is where this regression was.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      If units are not used at all, a numerical question can be saved and will work. If Unit Handling is set to 'Units are optional...' or to 'The unit must be given, and will be graded.' the question cannot be saved. Unit 1 will insist 'You must enter a multiplier here.' but will not let you type in the input box. I get the same behavior at qa.moodle.net and on my own 2.4 beta site.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            alrachels AL Rachels added a comment -

            If units are not used at all, a numerical question can be saved and will work. If Unit Handling is set to 'Units are optional...' or to 'The unit must be given, and will be graded.' the question cannot be saved. Unit 1 will insist 'You must enter a multiplier here.' but will not let you type in the input box. I get the same behavior at qa.moodle.net and on my own 2.4 beta site.

            Show
            alrachels AL Rachels added a comment - If units are not used at all, a numerical question can be saved and will work. If Unit Handling is set to 'Units are optional...' or to 'The unit must be given, and will be graded.' the question cannot be saved. Unit 1 will insist 'You must enter a multiplier here.' but will not let you type in the input box. I get the same behavior at qa.moodle.net and on my own 2.4 beta site.
            Hide
            timhunt Tim Hunt added a comment -

            OK, this is serious, but I am on the trail of it.

            Show
            timhunt Tim Hunt added a comment - OK, this is serious, but I am on the trail of it.
            Hide
            timhunt Tim Hunt added a comment -

            Adding those involved in MDL-32785 as watchers.

            Show
            timhunt Tim Hunt added a comment - Adding those involved in MDL-32785 as watchers.
            Hide
            timhunt Tim Hunt added a comment -

            Right, I think this fix works, but please can someone peer-review. Then I will cherry-pick to stable branches.

            Show
            timhunt Tim Hunt added a comment - Right, I think this fix works, but please can someone peer-review. Then I will cherry-pick to stable branches.
            Hide
            salvetore Michael de Raadt added a comment -

            I've asked Adrian to peer review this.

            Show
            salvetore Michael de Raadt added a comment - I've asked Adrian to peer review this.
            Hide
            abgreeve Adrian Greeve added a comment -

            [N] Syntax
            [Y] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [Y] Testing
            [Y] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            Hello Tim,

            Your code makes sense. I take it that you aren't using MoodleQuickForm::exportValues as it is a private function. The duplication of the code seems fine to me. Maybe the integrators have a different idea about that.
            Only one small thing. I know that this is a direct copy of exportValues, but perhaps you could change the variable names to all lower case.

            Show
            abgreeve Adrian Greeve added a comment - [N] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [Y] Security [-] Documentation [Y] Git [Y] Sanity check Hello Tim, Your code makes sense. I take it that you aren't using MoodleQuickForm::exportValues as it is a private function. The duplication of the code seems fine to me. Maybe the integrators have a different idea about that. Only one small thing. I know that this is a direct copy of exportValues, but perhaps you could change the variable names to all lower case.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks for the review.

            The whole point of MDL-30845 was that we cannot use export value, because that will use the submitted data, even if the field is meant to be frozen.

            I thought about changing the variable names, but in the end made the decision that it was better to match the coding style of the code I was copying, and the other nearby functions in formslib.

            I will cherry-pick and submit for integration shortly.

            Show
            timhunt Tim Hunt added a comment - Thanks for the review. The whole point of MDL-30845 was that we cannot use export value, because that will use the submitted data, even if the field is meant to be frozen. I thought about changing the variable names, but in the end made the decision that it was better to match the coding style of the code I was copying, and the other nearby functions in formslib. I will cherry-pick and submit for integration shortly.
            Hide
            timhunt Tim Hunt added a comment -

            Note that the symptom of the regression was only present in 2.4. I still think it is worth applying the formslib fix to 2.2 and 2.3.

            Show
            timhunt Tim Hunt added a comment - Note that the symptom of the regression was only present in 2.4. I still think it is worth applying the formslib fix to 2.2 and 2.3.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Tim, I've integrated this now.

            I agree about the approach and that it would be best backported as well.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Tim, I've integrated this now. I agree about the approach and that it would be best backported as well. Many thanks Sam
            Hide
            andyjdavis Andrew Davis added a comment -

            I have just tested and passed MDLQA-4840. I'm not sure if that qualifies this issue for an instant pass or if further testing here is required/justified.

            Show
            andyjdavis Andrew Davis added a comment - I have just tested and passed MDLQA-4840 . I'm not sure if that qualifies this issue for an instant pass or if further testing here is required/justified.
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success!

            I tested this on master, 2.3 and 2.2.

            I was able to create a numerical question with units. They validated and were able to be answered by a student.

            Show
            salvetore Michael de Raadt added a comment - Test result: Success! I tested this on master, 2.3 and 2.2. I was able to create a numerical question with units. They validated and were able to be answered by a student.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Y E S !

            Closing as fixed, many thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jan/13