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 Bug
    • Status: Closed
    • Priority: Blocker 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:
    • Rank:
      46272

      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.

        Issue Links

          Activity

          Hide
          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
          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
          Tim Hunt added a comment -

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

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

          Adding those involved in MDL-32785 as watchers.

          Show
          Tim Hunt added a comment - Adding those involved in MDL-32785 as watchers.
          Hide
          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
          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
          Michael de Raadt added a comment -

          I've asked Adrian to peer review this.

          Show
          Michael de Raadt added a comment - I've asked Adrian to peer review this.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          Y E S !

          Closing as fixed, many thanks!

          Show
          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: