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

javascript form validation library

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Can't think of a case where this case is reproducible. But make sure all the forms with required field get submitted.
      1. Login as student
      2. Edit profile (Settings -> Edit profile)
      3. Try updating profile with empty required fields.
      4. Form should not be submitted and display error message on fields.
      5. Try add valid values and forms should be submitted.

      Try perform similar testing on other forms with required fields and make sure form is not submitted with invalid input.

      Show
      Can't think of a case where this case is reproducible. But make sure all the forms with required field get submitted. 1. Login as student 2. Edit profile (Settings -> Edit profile) 3. Try updating profile with empty required fields. 4. Form should not be submitted and display error message on fields. 5. Try add valid values and forms should be submitted. Try perform similar testing on other forms with required fields and make sure form is not submitted with invalid input.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-27728

      Description

      Within lib/formslib.php on form submit, the code is intended to step through the form elements and validate the value of each element.

      Unfortunately, there are occasions when the element variable within the loop is undefined. This throws an exception. Since the form action has on exception return true, the form then gets submitted even though its values might not pass validation.

      This can be avoided as follows:

      Around line 1764 find

      function validate_' . $this->_formName . '_' . $escapedElementName . '(element) {

      Insert the following lines after it:

      if( undefined == element ){
           return false;
      }

      Then, around lines 1769 - 1774, find the range of lines beginning with

      var frm = element.parentNode;

      and ending with

      return qf_errorHandler(element, _qfMsg);

      Enclose this block in a condition like this:

          if( undefined != element.name ){
            var frm = element.parentNode;
            while (frm && frm.nodeName.toUpperCase() != "FORM") {
             frm = frm.parentNode;
            }
      ' . join("\n", $jsArr) . '
       
            return qf_errorHandler(element, _qfMsg);
          }

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this.

            I've put it on our backlog and we'll try to get to it as soon as we can.

            In the meantime adding more information, such as replication instructions, fix test instructions and a workaround, will help us and other users.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog and we'll try to get to it as soon as we can. In the meantime adding more information, such as replication instructions, fix test instructions and a workaround, will help us and other users.
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            Hello James,

            Thanks for finding this issue and resolving it.
            Can you please provide steps to reproduce this issue, as this issue should not even exist at first place.

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Hello James, Thanks for finding this issue and resolving it. Can you please provide steps to reproduce this issue, as this issue should not even exist at first place.
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            This case will never happen in moodle core, as script gets generated for valid element's.
            Providing this patch as "sanity check", to make sure we don't miss this case

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited This case will never happen in moodle core, as script gets generated for valid element's. Providing this patch as "sanity check", to make sure we don't miss this case
            Hide
            andyjdavis Andrew Davis added a comment - - edited

            The testing instructions are not currently adequate. Please provide instructions on how to get to a specific form that you know contains a required field. Just pick one The point of testing isn't to reproduce the bug. It's to check you didn't break anything while fixing the bug.

            Explain what should happen when you omit the required field Vs when you supply it. Example testing instructions are below

            1) go to form X by going to course administration and clicking Y
            2) the field 'blah' should be marked as required. Don't enter it. click submit.
            3) check you get an error saying that "blah is required".
            4) enter blah. click submit.
            5) check form is accepted.

            This is just a hastily thought out example and may not be quite right for this issue but basically if you have modified form validation write out test steps that show that bad data is still rejected and that good data will be accepted.

            Show
            andyjdavis Andrew Davis added a comment - - edited The testing instructions are not currently adequate. Please provide instructions on how to get to a specific form that you know contains a required field. Just pick one The point of testing isn't to reproduce the bug. It's to check you didn't break anything while fixing the bug. Explain what should happen when you omit the required field Vs when you supply it. Example testing instructions are below 1) go to form X by going to course administration and clicking Y 2) the field 'blah' should be marked as required. Don't enter it. click submit. 3) check you get an error saying that "blah is required". 4) enter blah. click submit. 5) check form is accepted. This is just a hastily thought out example and may not be quite right for this issue but basically if you have modified form validation write out test steps that show that bad data is still rejected and that good data will be accepted.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Andrew
            I have updated the test instructions, as suggested by you.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Andrew I have updated the test instructions, as suggested by you.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Raj,

            The patch is working great, except for validating textarea. Empty field in required textarea doesn't trigger any message to display.

            Another tiny issue is the spacing for the code.

            if ( undefined != element.name )

            And while you are there, you probably could also fix the tabbing spaces for that section (probably its a good idea to ask Sam for spacing issue).

            Regarding textarea issue, based on our conversation locally, there is no quick fix to address the issue. Therefore I created a separate issue to fix the textarea validation. Please refer to MDL-28674.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Raj, The patch is working great, except for validating textarea. Empty field in required textarea doesn't trigger any message to display. Another tiny issue is the spacing for the code. if ( undefined != element.name ) And while you are there, you probably could also fix the tabbing spaces for that section (probably its a good idea to ask Sam for spacing issue). Regarding textarea issue, based on our conversation locally, there is no quick fix to address the issue. Therefore I created a separate issue to fix the textarea validation. Please refer to MDL-28674 .
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for pointing that Rossie
            I just had a word with Sam about tabbing this section and at this point in time it's not important.

            I have modified the code to return true for undefined elements. As, in case of undefined elements JS should just let form to be submitted and let server handle validation.
            Checks will avoid browser to throw debug errors for undefined elements.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for pointing that Rossie I just had a word with Sam about tabbing this section and at this point in time it's not important. I have modified the code to return true for undefined elements. As, in case of undefined elements JS should just let form to be submitted and let server handle validation. Checks will avoid browser to throw debug errors for undefined elements.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Thanks for fixing this issue Raj.

            patches work great.

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks for fixing this issue Raj. patches work great.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated thanks!

            Please, test this into stable branch.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated thanks! Please, test this into stable branch.
            Hide
            salvetore Michael de Raadt added a comment -

            Works nicely. I was able to rename some form elements using Firebug the JS validation skipped them. On the lesson page editing form it passed problems through to the server also.

            Show
            salvetore Michael de Raadt added a comment - Works nicely. I was able to rename some form elements using Firebug the JS validation skipped them. On the lesson page editing form it passed problems through to the server also.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks!

            Closing. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks! Closing. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Oct/11