Moodle
  1. Moodle
  2. MDL-38555

Forms do not prevent same data submission multiple times.

    Details

    • Testing Instructions:
      Hide
      1. Open forum, select "Add a new topic" or "Reply" to existing one.
      2. Fill-in required fields.
      3. Try to click on Submit as many times as you can (do a double-click at least).
      4. Observe that after the first click the button becomes disabled and just one post appear as a result of submission.

      Testing fields error case:

      1. Open forum, select "Add a new topic" or "Reply" to existing one.
      2. Leave one of required fields empty.
      3. Submit the form and ensure that submit button does not become disabled.
      Show
      Open forum, select "Add a new topic" or "Reply" to existing one. Fill-in required fields. Try to click on Submit as many times as you can (do a double-click at least). Observe that after the first click the button becomes disabled and just one post appear as a result of submission. Testing fields error case: Open forum, select "Add a new topic" or "Reply" to existing one. Leave one of required fields empty. Submit the form and ensure that submit button does not become disabled.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-38555-master
    • Rank:
      48573

      Description

      Users reported that the forum post they submit appear twice or more times in some cases. Deeper investigation revealed that this happen if user double-clicks submit button or press submit again before the page has re-loaded after initial submission.

      It is easy to replicate:

      • Open forum, select "Add a new topic" or "Reply" to existing one
      • Fill-in required fields
      • Try to click on Submit as many times as you can (do a double-click at least).
      • Observe the result of submission, you should see the copy of your post published several times.

      The suggested solution disables submit button after the first click preventing multiple submissions, but it is slightly hacky as button should not be disabled if the from contains validation errors and JS validation code is output inline using getValidationScript() method. Better ideas are welcome.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I thought there was a pre-existing issue along the same lines, but I could not find it.

          Show
          Michael de Raadt added a comment - I thought there was a pre-existing issue along the same lines, but I could not find it.
          Hide
          Jason Fowler added a comment -

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

          Just need some testing instructions, then this will be good to go for integration.

          Show
          Jason Fowler added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Just need some testing instructions, then this will be good to go for integration.
          Hide
          Ruslan Kabalin added a comment -

          Thanks Jason, added testing instructions.

          Show
          Ruslan Kabalin added a comment - Thanks Jason, added testing instructions.
          Hide
          Ruslan Kabalin added a comment -

          Ready for integration.

          Show
          Ruslan Kabalin added a comment - Ready for integration.
          Hide
          Dan Poltawski added a comment -

          Adding Martin to decide if this improvement makes it in 2.5

          Show
          Dan Poltawski added a comment - Adding Martin to decide if this improvement makes it in 2.5
          Hide
          Sam Hemelryk added a comment -

          Thanks Ruslan this has been integrated now.

          At first I was leaning towards sending this back in order to tidy a couple of things up however after looking at JS for formslib it is apparent that what you've done here is consistent with formslib and I think itd be a better idea to tidy the whole area up in a single sweep.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Ruslan this has been integrated now. At first I was leaning towards sending this back in order to tidy a couple of things up however after looking at JS for formslib it is apparent that what you've done here is consistent with formslib and I think itd be a better idea to tidy the whole area up in a single sweep. Many thanks Sam
          Hide
          Dan Poltawski added a comment -

          Setting this to failed

          Show
          Dan Poltawski added a comment - Setting this to failed
          Hide
          Dan Poltawski added a comment -

          (Adrian will add reprouction steps for the regession spotted on another issue)

          Show
          Dan Poltawski added a comment - (Adrian will add reprouction steps for the regession spotted on another issue)
          Hide
          Sam Hemelryk added a comment -

          Aha thanks Dan.

          Ruslan I've reverted the patch now and am reopening this so that it can be further investigated and the issue Adrian discovered resolved.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Aha thanks Dan. Ruslan I've reverted the patch now and am reopening this so that it can be further investigated and the issue Adrian discovered resolved. Many thanks Sam
          Hide
          Adrian Greeve added a comment -

          My testing of MDL-40013 found the following problem when creating a matching question.
          Replication steps

          1. Go to the question bank (Administration ► Front page settings ► Question bank ► Questions)
          2. Create a new question - Matching
          3. Fill in the question name, questions, and answers for the three blank questions.
          4. Click "Blanks for 3 more questions"

          Expected result: The page redirects back to the question edit page with three more blank question sections.
          Actual result: The page redirects back to the question bank main page.

          Show
          Adrian Greeve added a comment - My testing of MDL-40013 found the following problem when creating a matching question. Replication steps Go to the question bank (Administration ► Front page settings ► Question bank ► Questions) Create a new question - Matching Fill in the question name, questions, and answers for the three blank questions. Click "Blanks for 3 more questions" Expected result: The page redirects back to the question edit page with three more blank question sections. Actual result: The page redirects back to the question bank main page.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Ruslan Kabalin added a comment -

          I see what causes the problem. Added the check if submit contains onlick attribute. If it contains, we do not override it with our submit function and leave it as it is. Basically, if submit already has some onclick action, we do not disable the button then.

          Show
          Ruslan Kabalin added a comment - I see what causes the problem. Added the check if submit contains onlick attribute. If it contains, we do not override it with our submit function and leave it as it is. Basically, if submit already has some onclick action, we do not disable the button then.
          Hide
          Ruslan Kabalin added a comment -

          Jason, can you please review it again.

          Show
          Ruslan Kabalin added a comment - Jason, can you please review it again.
          Hide
          Jason Fowler added a comment -

          Looks like it will do exactly what you are trying to do Ruslan, pushing for integration now.

          Show
          Jason Fowler added a comment - Looks like it will do exactly what you are trying to do Ruslan, pushing for integration now.
          Hide
          Sam Hemelryk added a comment -

          Thanks again Ruslan - this has once more been integrated.

          Show
          Sam Hemelryk added a comment - Thanks again Ruslan - this has once more been integrated.
          Hide
          Dan Poltawski added a comment -

          Again this seems to be causing a regression:

          1. Edit an existing assignment
          2. Click 'save and display'
          3. EXPECTED: You sent to the assignment
          4. ACTUAL: You are returned to the course page.
          Show
          Dan Poltawski added a comment - Again this seems to be causing a regression: Edit an existing assignment Click 'save and display' EXPECTED: You sent to the assignment ACTUAL: You are returned to the course page.
          Hide
          Ruslan Kabalin added a comment -

          You might need to clear JS cache. Will test it again in a moment.

          Show
          Ruslan Kabalin added a comment - You might need to clear JS cache. Will test it again in a moment.
          Hide
          Ruslan Kabalin added a comment -

          I am terribly sorry for not testing properly last time. This time I have tested solution with all possible situations.

          The problem was that upon submission, the name of submit element is checked to determine which of submit buttons has been clicked. When I disable submit button to pevent multiple clicks, the name-value pair for submit button is not passed, resulting in default behaviour. I resolved that by adding a dummy hidden element that contains name-value pair of original submit button in case we disable the latter, thus the expected bahaviour will be obtained. Another problem that has appeared is that event was linked to form submission, thus all submit buttons have been disabled on submission, resulting in a conflict when above approach with dummy element was used. I have narrowed down event procesing to clicked button only, rather than form submission. Now it seems work as expected in many possible cases.

          Show
          Ruslan Kabalin added a comment - I am terribly sorry for not testing properly last time. This time I have tested solution with all possible situations. The problem was that upon submission, the name of submit element is checked to determine which of submit buttons has been clicked. When I disable submit button to pevent multiple clicks, the name-value pair for submit button is not passed, resulting in default behaviour. I resolved that by adding a dummy hidden element that contains name-value pair of original submit button in case we disable the latter, thus the expected bahaviour will be obtained. Another problem that has appeared is that event was linked to form submission, thus all submit buttons have been disabled on submission, resulting in a conflict when above approach with dummy element was used. I have narrowed down event procesing to clicked button only, rather than form submission. Now it seems work as expected in many possible cases.
          Hide
          Ruslan Kabalin added a comment -

          We need to remove it from integration for another peer revision I suppose.

          Show
          Ruslan Kabalin added a comment - We need to remove it from integration for another peer revision I suppose.
          Hide
          David Monllaó added a comment -

          Hi,

          Behat reported failures, they seems to be related with this issue because of the wrong redirection. The only one that I'm not sure if it is caused by this issue is the 2nd one.

          (::) failed steps (::)
          
          01. "empty.txt" text was not found in the page
              In step `Then I should see "empty.txt"'.                            # behat_general::assert_page_contains_text()
              From scenario `Add files recently uploaded'.                        # /home/davidm/Desktop/moodlecode/INTEGRATION/master/repository/recent/tests/behat/add_recent.feature:8
              Of feature `Recent files repository lists the recently used files'. # /home/davidm/Desktop/moodlecode/INTEGRATION/master/repository/recent/tests/behat/add_recent.feature
          
          02. "Group members" text was not found in the page
              In step `Then I should see "Group members"'.                                                         # behat_general::assert_page_contains_text()
              From scenario `Split automatically the course users in groups and add the groups to a new grouping'. # /home/davidm/Desktop/moodlecode/INTEGRATION/master/group/tests/behat/auto_creation.feature:8
              Of feature `Automatic creation of groups'.                                                           # /home/davidm/Desktop/moodlecode/INTEGRATION/master/group/tests/behat/auto_creation.feature
          
          03. "upload_users.csv" text was not found in the page
              In step `Then I should see "upload_users.csv"'.                                                      # behat_general::assert_page_contains_text()
              From scenario `Cancel a selected recent file from being added to a folder'.                          # /home/davidm/Desktop/moodlecode/INTEGRATION/master/repository/tests/behat/cancel_add_file.feature:8
              Of feature `A selected file can be cancelled'.                                                       # /home/davidm/Desktop/moodlecode/INTEGRATION/master/repository/tests/behat/cancel_add_file.feature
          
          Show
          David Monllaó added a comment - Hi, Behat reported failures, they seems to be related with this issue because of the wrong redirection. The only one that I'm not sure if it is caused by this issue is the 2nd one. (::) failed steps (::) 01. "empty.txt" text was not found in the page In step `Then I should see "empty.txt" '. # behat_general::assert_page_contains_text() From scenario `Add files recently uploaded'. # /home/davidm/Desktop/moodlecode/INTEGRATION/master/repository/recent/tests/behat/add_recent.feature:8 Of feature `Recent files repository lists the recently used files'. # /home/davidm/Desktop/moodlecode/INTEGRATION/master/repository/recent/tests/behat/add_recent.feature 02. "Group members" text was not found in the page In step `Then I should see "Group members" '. # behat_general::assert_page_contains_text() From scenario `Split automatically the course users in groups and add the groups to a new grouping'. # /home/davidm/Desktop/moodlecode/INTEGRATION/master/group/tests/behat/auto_creation.feature:8 Of feature `Automatic creation of groups'. # /home/davidm/Desktop/moodlecode/INTEGRATION/master/group/tests/behat/auto_creation.feature 03. "upload_users.csv" text was not found in the page In step `Then I should see "upload_users.csv" '. # behat_general::assert_page_contains_text() From scenario `Cancel a selected recent file from being added to a folder'. # /home/davidm/Desktop/moodlecode/INTEGRATION/master/repository/tests/behat/cancel_add_file.feature:8 Of feature `A selected file can be cancelled'. # /home/davidm/Desktop/moodlecode/INTEGRATION/master/repository/tests/behat/cancel_add_file.feature
          Hide
          Sam Hemelryk added a comment -

          Reverted this commit and reopening.

          Show
          Sam Hemelryk added a comment - Reverted this commit and reopening.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            People

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

              Dates

              • Created:
                Updated: