Uploaded image for project: '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, MOODLE_28_STABLE
    • Pull Master Branch:
      MDL-38555-master

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore 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
            salvetore 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
            phalacee 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
            phalacee 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
            kabalin Ruslan Kabalin added a comment -

            Thanks Jason, added testing instructions.

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

            Ready for integration.

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

            Adding Martin to decide if this improvement makes it in 2.5

            Show
            poltawski Dan Poltawski added a comment - Adding Martin to decide if this improvement makes it in 2.5
            Hide
            samhemelryk 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
            samhemelryk 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
            poltawski Dan Poltawski added a comment -

            Setting this to failed

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

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

            Show
            poltawski Dan Poltawski added a comment - (Adrian will add reprouction steps for the regession spotted on another issue)
            Hide
            samhemelryk 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
            samhemelryk 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
            abgreeve 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
            abgreeve 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            kabalin 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
            kabalin 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
            kabalin Ruslan Kabalin added a comment -

            Jason, can you please review it again.

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

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

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

            Thanks again Ruslan - this has once more been integrated.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks again Ruslan - this has once more been integrated.
            Hide
            poltawski 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
            poltawski 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
            kabalin Ruslan Kabalin added a comment -

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

            Show
            kabalin Ruslan Kabalin added a comment - You might need to clear JS cache. Will test it again in a moment.
            Hide
            kabalin 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
            kabalin 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
            kabalin Ruslan Kabalin added a comment -

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

            Show
            kabalin Ruslan Kabalin added a comment - We need to remove it from integration for another peer revision I suppose.
            Hide
            dmonllao 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
            dmonllao 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
            samhemelryk Sam Hemelryk added a comment -

            Reverted this commit and reopening.

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

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

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

            I worked recently on this, just need to run behat and will update the patch very soon.

            Show
            kabalin Ruslan Kabalin added a comment - - edited I worked recently on this, just need to run behat and will update the patch very soon.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Pushed a new patch. This one also respects M.core_formchangechecker which did not exist when I pushed the fix last time. The code has been refactored, some issues with treating form errors have been fixed.

            I run behat, and it gives me errors, but completely different one to those reported above. They are similar to this one e.g. this one:

            03. Link matching locator "'Course 1'" not found.
                In step `When I follow "Course 1"'.
                From scenario `Self-enrolment disabled'.
                Of feature `Users can auto-enrol themself in courses where self enrolment is allowed'. 
            

            The thing is that the step is done immediately after login, so there is no other form than login is used prior to error, I tried to replicate, but was not successful because login works correctly with this patch. No idea what causes this Behat issue, another error is similar to this one (also step after login), which is confusing because hundreds of other logins in behat scenarios do not cause error.

            Show
            kabalin Ruslan Kabalin added a comment - Pushed a new patch. This one also respects M.core_formchangechecker which did not exist when I pushed the fix last time. The code has been refactored, some issues with treating form errors have been fixed. I run behat, and it gives me errors, but completely different one to those reported above. They are similar to this one e.g. this one: 03. Link matching locator "'Course 1'" not found. In step `When I follow "Course 1"'. From scenario `Self-enrolment disabled'. Of feature `Users can auto-enrol themself in courses where self enrolment is allowed'. The thing is that the step is done immediately after login, so there is no other form than login is used prior to error, I tried to replicate, but was not successful because login works correctly with this patch. No idea what causes this Behat issue, another error is similar to this one (also step after login), which is confusing because hundreds of other logins in behat scenarios do not cause error.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-38555 Remote repository: https://github.com/lucisgit/moodle.git Remote branch MDL-38555 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3483 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3483/artifact/work/smurf.html
            Hide
            marina Marina Glancy added a comment -

            I added the AJAX&Javascript component to the issue so maybe Andrew Nicols or somebody else from frontend can review this issue. From my point of view the JS is not located in the proper place, should rather be lib/yui/src/xxx where we already have formautosubmit and formchangechecker.

            Show
            marina Marina Glancy added a comment - I added the AJAX&Javascript component to the issue so maybe Andrew Nicols or somebody else from frontend can review this issue. From my point of view the JS is not located in the proper place, should rather be lib/yui/src/xxx where we already have formautosubmit and formchangechecker.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Marina, in this case JS functionality is related to particular form element only, not to form in general. Currenly similar small JS bits related to other elements are located in the same directory (/lib/form/).

            Show
            kabalin Ruslan Kabalin added a comment - Marina, in this case JS functionality is related to particular form element only, not to form in general. Currenly similar small JS bits related to other elements are located in the same directory (/lib/form/).
            Hide
            dobedobedoh Andrew Nicols added a comment -

            [N] Syntax
            [Y] Whitespace
            [Y] Output
            [-] Language
            [-] Databases
            [N] Testing (instructions and automated tests)
            [Y] Security
            [-] Documentation
            [Y] Git
            [-] Third party code
            [N] Sanity check

            Syntax

            Syntax-wise this is mostly okay, but it doesn't comply with the JS Coding guidelines (http://docs.moodle.org/dev/Javascript/Coding_style). It should also be written in a proper, shifted YUI module.

            Other comments:

            • L4: It would be better to do this with event delegation rather than a Y.one().on(). Largely because there could be a time when the form is not actually displayed on the page (sometimes we generate forms but don't end up displaying them immediately) but the JS is generated.
            • L4: Actually, rather than event delegation, and listening to the click event, we should listen to the form submission instead.
            • L5: I'm not keen on using this global. Can this not use an attribute on the form instead?
            • L6-13: Incorrect comment style
            • L14-17: Naming - camelCase please.
            • L15-16: Not keen on using Y.one() like this
            • L22-27: Incorrect comment style
            • L28-30: It would be good to find out the real reason here rather than just working around it
            • L31-32: If this were using a form submission rather than button click listener, this wouldn't be necessary at all
            Testing

            I think the testing instructions could do with extending to cover other cases.

            Sanity Check

            As mentioned above, I think that the approach here is incorrect.
            This also needs to be a shifted YUI module, and could potentially be added to the standard rollup.

            Show
            dobedobedoh Andrew Nicols added a comment - [N] Syntax [Y] Whitespace [Y] Output [-] Language [-] Databases [N] Testing (instructions and automated tests) [Y] Security [-] Documentation [Y] Git [-] Third party code [N] Sanity check Syntax Syntax-wise this is mostly okay, but it doesn't comply with the JS Coding guidelines ( http://docs.moodle.org/dev/Javascript/Coding_style ). It should also be written in a proper, shifted YUI module. Other comments: L4: It would be better to do this with event delegation rather than a Y.one().on(). Largely because there could be a time when the form is not actually displayed on the page (sometimes we generate forms but don't end up displaying them immediately) but the JS is generated. L4: Actually, rather than event delegation, and listening to the click event, we should listen to the form submission instead. L5: I'm not keen on using this global. Can this not use an attribute on the form instead? L6-13: Incorrect comment style L14-17: Naming - camelCase please. L15-16: Not keen on using Y.one() like this L22-27: Incorrect comment style L28-30: It would be good to find out the real reason here rather than just working around it L31-32: If this were using a form submission rather than button click listener, this wouldn't be necessary at all Testing I think the testing instructions could do with extending to cover other cases. Sanity Check As mentioned above, I think that the approach here is incorrect. This also needs to be a shifted YUI module, and could potentially be added to the standard rollup.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Ughhh. Thanks, Andrew

            Show
            kabalin Ruslan Kabalin added a comment - Ughhh. Thanks, Andrew
            Hide
            moodle.com moodle.com added a comment -

            Ruslan: Are you still working on this?

            Show
            moodle.com moodle.com added a comment - Ruslan: Are you still working on this?
            Hide
            kabalin Ruslan Kabalin added a comment -

            Not at the moment, but it is on my list to look at soon.

            Show
            kabalin Ruslan Kabalin added a comment - Not at the moment, but it is on my list to look at soon.

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                12 Start watching this issue

                Dates

                • Created:
                  Updated: