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

"Submit in Groups" Assignment setting allows a student in a group to submit an empty file picker that can override another student's file submission with an empty submission.

    Details

    • Testing Instructions:
      Hide

      NB: On master both sets of tests need to be carried out. On 31 and 30 only the test involving the two students and a teacher is needed.

      These tests need to be carried out using two browsers (or with something like incognito mode):

      Test group submissions

      1. Create a course and enroll two students and a teacher, I'll call them S1 and S2
      2. Browse to "Course administration" > "Users" > "Groups"
      3. Press the "Create group" button
      4. Give the group a name and save
      5. Add S1 and S2 to the group
      6. Back on the groups page, navigate to the "Groupings" tab
      7. Press "Create grouping"
      8. Give the grouping a name and save
      9. You should be back at the groupings page, under the "Edit" column for the new grouping click the "people" icon ("Show groups in grouping"
      10. Add the previously created group to the grouping
      11. Create an assignment for the course:
        • Enable online text and file submission
        • Under group submission settings, set "Students submit in groups" to "Yes"
        • Under group submission settings, set "Grouping for student groups" to the grouping created earlier
      12. Log in as S1, and in a different browser log in as S2
      13. As S1, browse to the assignment and press "Add submission"
      14. As S2 do the same thing
      15. Back as S1 submit something in either of the submission areas (or both, if you feel like it)
      16. Save
      17. As S2 try to submit something
      18. Verify an error explaining that someone else modified the submission in the meantime is displayed
      19. Browse to the assignment submission page again, verify you see the modifications from the other student
      20. Modify the submission again and save. Ensure there are no errors.
      21. Play around with students clobbering each other's changes and make sure nothing goes wrong

      Test individual submissions

      1. Create an a new assignment in the course (just use the default settings. No grouping stuff is needed)
      2. Log in as the same student in two different browsers
      3. In both browsers, navigate to the new assignment and press "Add submission"
      4. In one of the browsers, add a submission and save
      5. In the other browser, attempt to submit something
      6. Verify you get an error explaining that you have existing submission data
      7. Navigate to the submission page again and verify you can edit the submission without problems
      8. Play around with attempting to clobber your own changes, make sure you can't

      The following tests just need one student and one browser:

      Test empty submissions

      1. Browse to the assignment submission page
      2. Clear both the file and online text area
      3. Attempt to submit the assignment
      4. Verify an error message explaining that the submission was empty is displayed
      5. As admin, check the submissions for the assignment and ensure that the submission is not empty
      6. As the student, go back to the assignment and submit something in the online text area but make sure the files area is empty
      7. Submit the assignment and as the admin verify the submission appears as it was submitted (i.e., not file, and some text)
      8. As the student edit the submission again. This time remove the text and add a file
      9. Submit the assignment, and ass the admin verify the submission appears as it was submitted (i.e., a file and no text)
      10. Finally, as the student, edit the submission and add something for both the text and file
      11. Submit, and as admin verify the submission appears as it was submitted (i.e., file and text)
      Show
      NB: On master both sets of tests need to be carried out. On 31 and 30 only the test involving the two students and a teacher is needed. These tests need to be carried out using two browsers (or with something like incognito mode): Test group submissions Create a course and enroll two students and a teacher, I'll call them S1 and S2 Browse to "Course administration" > "Users" > "Groups" Press the "Create group" button Give the group a name and save Add S1 and S2 to the group Back on the groups page, navigate to the "Groupings" tab Press "Create grouping" Give the grouping a name and save You should be back at the groupings page, under the "Edit" column for the new grouping click the "people" icon ("Show groups in grouping" Add the previously created group to the grouping Create an assignment for the course: Enable online text and file submission Under group submission settings, set "Students submit in groups" to "Yes" Under group submission settings, set "Grouping for student groups" to the grouping created earlier Log in as S1, and in a different browser log in as S2 As S1, browse to the assignment and press "Add submission" As S2 do the same thing Back as S1 submit something in either of the submission areas (or both, if you feel like it) Save As S2 try to submit something Verify an error explaining that someone else modified the submission in the meantime is displayed Browse to the assignment submission page again, verify you see the modifications from the other student Modify the submission again and save. Ensure there are no errors. Play around with students clobbering each other's changes and make sure nothing goes wrong Test individual submissions Create an a new assignment in the course (just use the default settings. No grouping stuff is needed) Log in as the same student in two different browsers In both browsers, navigate to the new assignment and press "Add submission" In one of the browsers, add a submission and save In the other browser, attempt to submit something Verify you get an error explaining that you have existing submission data Navigate to the submission page again and verify you can edit the submission without problems Play around with attempting to clobber your own changes, make sure you can't The following tests just need one student and one browser: Test empty submissions Browse to the assignment submission page Clear both the file and online text area Attempt to submit the assignment Verify an error message explaining that the submission was empty is displayed As admin, check the submissions for the assignment and ensure that the submission is not empty As the student, go back to the assignment and submit something in the online text area but make sure the files area is empty Submit the assignment and as the admin verify the submission appears as it was submitted (i.e., not file, and some text) As the student edit the submission again. This time remove the text and add a file Submit the assignment, and ass the admin verify the submission appears as it was submitted (i.e., a file and no text) Finally, as the student, edit the submission and add something for both the text and file Submit, and as admin verify the submission appears as it was submitted (i.e., file and text)
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_28_STABLE, MOODLE_30_STABLE
    • Fixed Branches:
      MOODLE_30_STABLE, MOODLE_31_STABLE
    • Pull 3.0 Branch:
    • Pull 3.1 Branch:
    • Pull Master Branch:
      MDL-41945-master
    • Sprint:
      3.2 Sprint 2
    • Sprint:
      3.2 Sprint 2

      Description

      Instructors at our school have discovered this when students are working on Group assignments in class. Multiple students try to access the assignment, and have unknowingly erased other students' file submissions. The "Nothing was submitted" alert that pops up when a student tries save an empty file picker misleads a student into thinking they have not inadvertently overwritten another student's file submission with an empty slot. Trying to submit an empty file picker should do nothing, not actively overwrite previous submissions.

      Reproduction Steps:

      1.Create an assignment in a course with at least 2 students in a group & grouping.
      2.Change the settings so that "Students submit in groups" is enabled, and that "Grouping for student groups" is associated with grouping containing the two students.
      3.On two additional separate browsers, sign in as the two students and navigate to the assignment submission screen for each student on each browser by clicking "Add Submission", so that each is on the "File submissions" drop-box page.
      4.On Student 1's browser page, have Student 1 upload a file into the box and Save Changes. Confirm that the "Submission status" screen loads showing the submitted file.
      5.On Student 2's browser page, have Student 2 click the Save Changes button with no files entered. Confirm that a "Nothing was submitted" alert appears.
      6.Have Student 2 click "Cancel." Confirm that the "Submission status" screen loads showing no submitted file.
      7.Have Student 1 click the breadcrumb with the Assignment activity name to reload the submission page. Confirm that the "Submission status" screen loads showing no submitted file.
      8.As the teacher, navigate to the assignment Grading screen. Confirm that both students are flagged as "submitted," with no files present.

      This is especially problematic if "Require students click submit button" is enabled for the assignment without "all students must submit" enabled. In that case, if Student 1 uploads and Submits a file, and Student 2 saves the empty file picker, this locks all the students in the group from being able to update the now-empty file submission slot.

      This looks like it is related to MDL-36908, MDL-35963, and possibly MDL-39569

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              sama Sam Anderson added a comment -

              This bug is still present on 2.8, and it has tripped up at least one of our instructors. Any sense on when it will see progress?

              Show
              sama Sam Anderson added a comment - This bug is still present on 2.8, and it has tripped up at least one of our instructors. Any sense on when it will see progress?
              Hide
              mcka0074 Mark McKay added a comment -

              Marina Glancy has this been triaged?

              Show
              mcka0074 Mark McKay added a comment - Marina Glancy has this been triaged?
              Hide
              marina Marina Glancy added a comment -

              Hi Mark, Damyon Wiese is the component lead for the Assignment component. I'm sure he will get to this issue soon, he's very busy at the moment afaik

              Show
              marina Marina Glancy added a comment - Hi Mark, Damyon Wiese is the component lead for the Assignment component. I'm sure he will get to this issue soon, he's very busy at the moment afaik
              Hide
              damyon Damyon Wiese added a comment -

              Thanks for reporting this. Moodle file areas are not really good at handling 2 people editing them at the same time - this will be the same anywhere in Moodle - it's just more noticable here. We could add a check that the submission was modified while you had the form open, and reject the submission from the second student.

              I'll triage this as Major because of the dataloss - but I imagine it happens rarely.

              Show
              damyon Damyon Wiese added a comment - Thanks for reporting this. Moodle file areas are not really good at handling 2 people editing them at the same time - this will be the same anywhere in Moodle - it's just more noticable here. We could add a check that the submission was modified while you had the form open, and reject the submission from the second student. I'll triage this as Major because of the dataloss - but I imagine it happens rarely.
              Hide
              pavel.m.sokolov Pavel Sokolov added a comment -

              We faced a similar problem, but the difference is a student's intention. When working in groups, after all team members submitted for grading, some last student with dishonest behaviour can lately replace originally submitted files to a faulty one. Say, to a file containing plagiarism. Then he/she presses 'submit for grading', and teacher effectively thinks that it was a shared work of all team members, giving them Fail grade due to plagiarism. Even if a dialogue starts and everyone claims it was a bad move of some student, there is still a room for students to cheat and submit some 'junk' file when deadline approaches, expecting to replace it with a new one later (arguing that some student has replaced the original file with no acknowledgment of team members).

              What do you think? Maybe some similar issue exists (couldn't find any).

              Show
              pavel.m.sokolov Pavel Sokolov added a comment - We faced a similar problem, but the difference is a student's intention. When working in groups, after all team members submitted for grading, some last student with dishonest behaviour can lately replace originally submitted files to a faulty one. Say, to a file containing plagiarism. Then he/she presses 'submit for grading', and teacher effectively thinks that it was a shared work of all team members, giving them Fail grade due to plagiarism. Even if a dialogue starts and everyone claims it was a bad move of some student, there is still a room for students to cheat and submit some 'junk' file when deadline approaches, expecting to replace it with a new one later (arguing that some student has replaced the original file with no acknowledgment of team members). What do you think? Maybe some similar issue exists (couldn't find any).
              Hide
              cameron1729 Cameron Ball added a comment -

              There were two problems I identified here...

              1. Users can clobber each other's changes

              To mitigate this, I added a simple modification time check to alert users when someone else has made changes to the submission.

              2. The current way we check if the submission is empty looks like this:

              mod/assign/locallib.php

                      $pluginerror = false;
                      foreach ($this->submissionplugins as $plugin) {
                          if ($plugin->is_enabled() && $plugin->is_visible()) {
                              if (!$plugin->save($submission, $data)) {
                                  $notices[] = $plugin->get_error();
                                  $pluginerror = true;
                              }
                          }
                      }
                      $allempty = $this->submission_empty($submission);
                      if ($pluginerror || $allempty) {
                          if ($allempty) {
                              $notices[] = get_string('submissionempty', 'mod_assign');
                          }
                          return false;
                      }
              

              The problem with this is that we save the submission plugin stuff before the submission_empty check. Unfortunately we can't just move that check, because submission_empty relies on data in the database. This means users will get a message saying that their submission can't be empty, but the submission will have saved. This leads to problems like this

              So what I've done is add a new method. submission_is_empty to assign_submission_plugin which just returns false. Then the submission plugins have to override it with their own logic to determine if they are empty or not. Then we display the empty submission notification based on whether or not the plugins report the submission is empty. I decided to have it return false in the base implementation so that any existing plugins will continue to behave in the existing (broken) way.

              Show
              cameron1729 Cameron Ball added a comment - There were two problems I identified here... 1. Users can clobber each other's changes To mitigate this, I added a simple modification time check to alert users when someone else has made changes to the submission. 2. The current way we check if the submission is empty looks like this: mod/assign/locallib.php $pluginerror = false; foreach ( $this ->submissionplugins as $plugin ) { if ( $plugin ->is_enabled() && $plugin ->is_visible()) { if (! $plugin ->save( $submission , $data )) { $notices [] = $plugin ->get_error(); $pluginerror = true; } } } $allempty = $this ->submission_empty( $submission ); if ( $pluginerror || $allempty ) { if ( $allempty ) { $notices [] = get_string( 'submissionempty' , 'mod_assign' ); } return false; } The problem with this is that we save the submission plugin stuff before the submission_empty check. Unfortunately we can't just move that check, because submission_empty relies on data in the database. This means users will get a message saying that their submission can't be empty, but the submission will have saved. This leads to problems like this So what I've done is add a new method. submission_is_empty to assign_submission_plugin which just returns false. Then the submission plugins have to override it with their own logic to determine if they are empty or not. Then we display the empty submission notification based on whether or not the plugins report the submission is empty. I decided to have it return false in the base implementation so that any existing plugins will continue to behave in the existing (broken) way.
              Hide
              cameron1729 Cameron Ball added a comment -

              Sending for peer review.

              Show
              cameron1729 Cameron Ball added a comment - Sending for peer review.
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git master (0 errors / 0 warnings) [branch: MDL-41945-master | CI Job ] More information about this report
              Hide
              abgreeve Adrian Greeve added a comment -

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

              Hello Cameron,

              This all looks good to me. Is there any chance that you could provide some unit tests for the new functions that you have created?

              Cheers,

              Show
              abgreeve Adrian Greeve added a comment - [Y] Syntax [Y] Whitespace [Y] Output [Y] Language [Y] Databases [*] Testing (instructions and automated tests) [-] Security [-] Documentation [Y] Git [Y] Third party code [Y] Sanity check [-] Icons Hello Cameron, This all looks good to me. Is there any chance that you could provide some unit tests for the new functions that you have created? Cheers,
              Hide
              cameron1729 Cameron Ball added a comment -

              Thanks Adrian, I don't think it would be a problem to add unit tests :] If you're OK with patch as it is, I'll add some shortly, and then send for integration review.

              Show
              cameron1729 Cameron Ball added a comment - Thanks Adrian, I don't think it would be a problem to add unit tests :] If you're OK with patch as it is, I'll add some shortly, and then send for integration review.
              Hide
              abgreeve Adrian Greeve added a comment -

              Sounds good .

              Show
              abgreeve Adrian Greeve added a comment - Sounds good .
              Hide
              cameron1729 Cameron Ball added a comment -

              Unit tests added. Sending for integration review.

              Show
              cameron1729 Cameron Ball added a comment - Unit tests added. Sending for integration review.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              dobedobedoh Andrew Nicols added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              poltawski Dan Poltawski added a comment -

              Sorry, i'm reopening this -see all the unit test failures:

              https://travis-ci.org/cameron1729/moodle/jobs/134681136

              Show
              poltawski Dan Poltawski added a comment - Sorry, i'm reopening this -see all the unit test failures: https://travis-ci.org/cameron1729/moodle/jobs/134681136
              Hide
              cameron1729 Cameron Ball added a comment -

              Ahh.. It looks like I messed something up when I rebased my branch. I fixed it and now all the unit tests are passing. Sending for integration review again.

              Show
              cameron1729 Cameron Ball added a comment - Ahh.. It looks like I messed something up when I rebased my branch. I fixed it and now all the unit tests are passing. Sending for integration review again.
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git master (0 errors / 0 warnings) [branch: MDL-41945-master | CI Job ] More information about this report
              Hide
              dobedobedoh Andrew Nicols added a comment -

              I'm going to take this issue out of peer review because in its current form (i.e. with API changes) it cannot be backported.

              Some brief notes:

              1. Missing upgrade.txt
              2. Superflous setUp in all unit tests - there's one test in the testCase so the setUp should be in the test
              3. Some of these should be using dataProviders where they're not (but kudos for getting some in )

              I've not done a detailed review so the integrator who picks this up next may make other points.

              Show
              dobedobedoh Andrew Nicols added a comment - I'm going to take this issue out of peer review because in its current form (i.e. with API changes) it cannot be backported. Some brief notes: Missing upgrade.txt Superflous setUp in all unit tests - there's one test in the testCase so the setUp should be in the test Some of these should be using dataProviders where they're not (but kudos for getting some in ) I've not done a detailed review so the integrator who picks this up next may make other points.
              Hide
              cameron1729 Cameron Ball added a comment -

              Thanks for the feedback Andrew. I have:

              • Added notes to upgrade.txt
              • Refactored the unit tests:
                • They all use dataProviders now
                • The tests for new_submission_empty that pertain to only one submission plugin have been moved in to the test file for that plugin
              Show
              cameron1729 Cameron Ball added a comment - Thanks for the feedback Andrew. I have: Added notes to upgrade.txt Refactored the unit tests: They all use dataProviders now The tests for new_submission_empty that pertain to only one submission plugin have been moved in to the test file for that plugin
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Now that the on-sync period has ended, this issue is being un-held and will be processed normally as part of the integration queue. Thanks for your patience.

              Also, you may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Now that the on-sync period has ended, this issue is being un-held and will be processed normally as part of the integration queue. Thanks for your patience. Also, you may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git master (1 errors / 0 warnings) [branch: MDL-41945-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              poltawski Dan Poltawski added a comment -

              Given this issue has 25 votes, I do feel like we need a solution which is back-portable, even if the bug remains for non-standard submission plugins - what do you think about the backportable solution Cam?

              Show
              poltawski Dan Poltawski added a comment - Given this issue has 25 votes, I do feel like we need a solution which is back-portable, even if the bug remains for non-standard submission plugins - what do you think about the backportable solution Cam?
              Hide
              cameron1729 Cameron Ball added a comment -

              Sure, I think it should be OK to backport the parts of my patch that check if the submission has been modified while another student is looking at it, shouldn't it?

              If so I'll add backport branches for that.

              Show
              cameron1729 Cameron Ball added a comment - Sure, I think it should be OK to backport the parts of my patch that check if the submission has been modified while another student is looking at it, shouldn't it? If so I'll add backport branches for that.
              Hide
              poltawski Dan Poltawski added a comment -

              Sorry I didn't respond - yep sounds good

              Show
              poltawski Dan Poltawski added a comment - Sorry I didn't respond - yep sounds good
              Hide
              poltawski Dan Poltawski added a comment -

              Just sending this back to get those changes for next week

              Show
              poltawski Dan Poltawski added a comment - Just sending this back to get those changes for next week
              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
              cameron1729 Cameron Ball added a comment -

              I cherry-picked the commit that adds the time checking in to 31 and 30. Sending back for integration review.

              Show
              cameron1729 Cameron Ball added a comment - I cherry-picked the commit that adds the time checking in to 31 and 30. Sending back for integration review.
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git MOODLE_30_STABLE (0 errors / 0 warnings) [branch: MDL-41945-30 | CI Job ] MOODLE_31_STABLE (0 errors / 0 warnings) [branch: MDL-41945-31 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-41945-master | CI Job ] More information about this report
              Hide
              dobedobedoh Andrew Nicols added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              dobedobedoh Andrew Nicols added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git MOODLE_30_STABLE (0 errors / 0 warnings) [branch: MDL-41945-30 | CI Job ] MOODLE_31_STABLE (0 errors / 0 warnings) [branch: MDL-41945-31 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-41945-master | CI Job ] More information about this report
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Cam, integrated to master, 31 and 30.

              BTW, kudos on the more detailed commit messages

              Show
              poltawski Dan Poltawski added a comment - Thanks Cam, integrated to master, 31 and 30. BTW, kudos on the more detailed commit messages
              Hide
              cameron1729 Cameron Ball added a comment -

              Awesome, thanks!

              Show
              cameron1729 Cameron Ball added a comment - Awesome, thanks!
              Hide
              jaked Jake Dallimore added a comment -

              Hi all,
              Tested the first test in master, 31 and 30 and the second test in master only. Everything works as expected, so test passed .

              Cam, are there any plans to open an issue relating to the issue where empty submissions are saving the data before displaying the error message about the empty submission (for 30 and 31)? I expect if it was possible, you would have already backported it however.

              Show
              jaked Jake Dallimore added a comment - Hi all, Tested the first test in master, 31 and 30 and the second test in master only. Everything works as expected, so test passed . Cam, are there any plans to open an issue relating to the issue where empty submissions are saving the data before displaying the error message about the empty submission (for 30 and 31)? I expect if it was possible, you would have already backported it however.
              Hide
              cameron1729 Cameron Ball added a comment -

              Thanks Jake!

              I don't know if we can resolve the issue in 30 and 31. I think the way I have done it here is about as simple as it could be done... If we really want it, this patch should backport fairly easily on to 31 and 30.

              Show
              cameron1729 Cameron Ball added a comment - Thanks Jake! I don't know if we can resolve the issue in 30 and 31. I think the way I have done it here is about as simple as it could be done... If we really want it, this patch should backport fairly easily on to 31 and 30.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for working on this Cameron,

              unfortunately this is causing behat failure

              --- Failed steps:
               
                  Then I should see "Submitted for grading" # /store/moodle/behat_whole_suite_m_parallel/grade/tests/behat/grade_view.feature:43
                    "Submitted for grading" text was not found in the page (Behat\Mink\Exception\ExpectationException)
               
                  Then I should see "Submitted for grading" # /store/moodle/behat_whole_suite_m_parallel/grade/tests/behat/grade_view.feature:43
                    "Submitted for grading" text was not found in the page (Behat\Mink\Exception\ExpectationException)
              

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for working on this Cameron, unfortunately this is causing behat failure --- Failed steps:   Then I should see "Submitted for grading" # /store/moodle/behat_whole_suite_m_parallel/grade/tests/behat/grade_view.feature:43 "Submitted for grading" text was not found in the page (Behat\Mink\Exception\ExpectationException)   Then I should see "Submitted for grading" # /store/moodle/behat_whole_suite_m_parallel/grade/tests/behat/grade_view.feature:43 "Submitted for grading" text was not found in the page (Behat\Mink\Exception\ExpectationException)
              Hide
              jaked Jake Dallimore added a comment -

              Just one additional note after we discussed this morning - the error message string 'The submission has been modified by somebody else. Please refresh and try again' should be changed as refreshing the page reposts the form and generates the same error message over and over again. Not a great user experience. I must have missed this as the testing instructions specifically stated to navigate to the submission again (i.e. not refresh).

              Show
              jaked Jake Dallimore added a comment - Just one additional note after we discussed this morning - the error message string 'The submission has been modified by somebody else. Please refresh and try again' should be changed as refreshing the page reposts the form and generates the same error message over and over again. Not a great user experience. I must have missed this as the testing instructions specifically stated to navigate to the submission again (i.e. not refresh).
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              The reason this is failing is $data->firstviewed and $submission->timemodified is same on non-js run.
              Further looking at it, it seems to be a potential race problem is a couple of cases:

              1. This check happens for assignment status new as well, so if user1 has just visited assignment (not yet press "add submission") and user2 enters the assignment and try submit assignment then it will fail. Where as it should not as the user1 has not started anything.
              2. In above case if user1 submits draft then should user2 be not allowed to submit ?
              3. This error can be viewed by the user who is not in group as well, if logged in from 2 different browsers.
              4. Finally Jake pointed: that refresh doesn't work, so the error message is misleading.

              Was talking to Damyon about this and it will be nice to use submission timemodified rather than time() as it can lead to other race conditions.

              NOTE: It will be nice to document all use cases for single and group submission where such error should be visible and not visible.

              Show
              rajeshtaneja Rajesh Taneja added a comment - The reason this is failing is $data->firstviewed and $submission->timemodified is same on non-js run. Further looking at it, it seems to be a potential race problem is a couple of cases: This check happens for assignment status new as well, so if user1 has just visited assignment (not yet press "add submission") and user2 enters the assignment and try submit assignment then it will fail. Where as it should not as the user1 has not started anything. In above case if user1 submits draft then should user2 be not allowed to submit ? This error can be viewed by the user who is not in group as well, if logged in from 2 different browsers. Finally Jake pointed: that refresh doesn't work, so the error message is misleading. Was talking to Damyon about this and it will be nice to use submission timemodified rather than time() as it can lead to other race conditions. NOTE: It will be nice to document all use cases for single and group submission where such error should be visible and not visible.
              Hide
              cameron1729 Cameron Ball added a comment - - edited

              Hi all, here's a patch that works a little better (have not updated testing instructions to cover new cases yet):

              Pull Master Branch: MDL-41945-master-fix
              Pull Master Diff URL: https://github.com/cameron1729/moodle/commit/883d22d69568deaa0dfcf247b47ae323f0a73fc3

              Basically:

              1. The check only happens when the submission is not new
              2. The message has been changed to urge the user to leave the page and try again
              3. There are two different messages depending on whether the user is clobber another user's changes, or their own
              4. time() is no longer used. Instead we check if the last modification seen by the current user has changed.

              One slight annoyance about 4. is that we can no longer check something like timemodified >= lastmodified since the modified time does not update until a submission is saved (and of course we have to do these checks before the submission is saved). So unless another user has modified the submission, we will always have timemodified equal to lastmodified. So we need to do strictly greater than. This means there is a window of 1 second where group submissions could clobber each other.

              Andrew Nicols, thoughts?

              Show
              cameron1729 Cameron Ball added a comment - - edited Hi all, here's a patch that works a little better (have not updated testing instructions to cover new cases yet): Pull Master Branch: MDL-41945 -master-fix Pull Master Diff URL: https://github.com/cameron1729/moodle/commit/883d22d69568deaa0dfcf247b47ae323f0a73fc3 Basically: The check only happens when the submission is not new The message has been changed to urge the user to leave the page and try again There are two different messages depending on whether the user is clobber another user's changes, or their own time() is no longer used. Instead we check if the last modification seen by the current user has changed. One slight annoyance about 4. is that we can no longer check something like timemodified >= lastmodified since the modified time does not update until a submission is saved (and of course we have to do these checks before the submission is saved). So unless another user has modified the submission, we will always have timemodified equal to lastmodified . So we need to do strictly greater than. This means there is a window of 1 second where group submissions could clobber each other. Andrew Nicols , thoughts?
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Cam, Since we're quite late on Thursday i'm gonna revert this for this week so we can give it a bit more time to brew, give people a chance to rebase and not force even more testing on Friday.

              Lets land this next week with some more time.

              Show
              poltawski Dan Poltawski added a comment - Thanks Cam, Since we're quite late on Thursday i'm gonna revert this for this week so we can give it a bit more time to brew, give people a chance to rebase and not force even more testing on Friday. Lets land this next week with some more time.
              Hide
              poltawski Dan Poltawski added a comment -

              Reopening for now (sure it would be helpful for Damyon Wiese to have a look as component maintainer too)

              Show
              poltawski Dan Poltawski added a comment - Reopening for now (sure it would be helpful for Damyon Wiese to have a look as component maintainer too)
              Hide
              cameron1729 Cameron Ball added a comment -

              No probs, will update my original branches. Cheers!

              Show
              cameron1729 Cameron Ball added a comment - No probs, will update my original branches. Cheers!
              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
              cameron1729 Cameron Ball added a comment -

              Branches updated. Sending for integration review.

              Show
              cameron1729 Cameron Ball added a comment - Branches updated. Sending for integration review.
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git MOODLE_30_STABLE (1 errors / 0 warnings) [branch: MDL-41945-30 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , MOODLE_31_STABLE (1 errors / 0 warnings) [branch: MDL-41945-31 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , master (1 errors / 0 warnings) [branch: MDL-41945-master | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              cibot CiBoT added a comment -
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-41945 using repository: git://github.com/cameron1729/moodle.git MOODLE_30_STABLE (1 errors / 0 warnings) [branch: MDL-41945-30 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , MOODLE_31_STABLE (1 errors / 0 warnings) [branch: MDL-41945-31 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , master (1 errors / 0 warnings) [branch: MDL-41945-master | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , More information about this report
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated master, 31 and 30 thanks

              Show
              poltawski Dan Poltawski added a comment - Integrated master, 31 and 30 thanks
              Hide
              lameze Simey Lameze added a comment -

              Sorry it is failing for me.

              First two tests worked really well, on third, test empty submissions, it fails on step 5 onwards.
              I cleared the submission fields, deleting the text and the file from submission, tried to save and I got the "Nothing was submitted".
              When I got back to the submission page, both fields were empty and the testing instructions says:

              6. As the student, go back to the assignment and submit something in the online text area but make sure the files area is empty

              Failing so it can be looked at.

              Show
              lameze Simey Lameze added a comment - Sorry it is failing for me. First two tests worked really well, on third, test empty submissions, it fails on step 5 onwards. I cleared the submission fields, deleting the text and the file from submission, tried to save and I got the "Nothing was submitted". When I got back to the submission page, both fields were empty and the testing instructions says: 6. As the student, go back to the assignment and submit something in the online text area but make sure the files area is empty Failing so it can be looked at.
              Hide
              cameron1729 Cameron Ball added a comment -

              Somehow the empty submission check got lost when I was rebasing. Found it in my reflog and added it back: https://github.com/cameron1729/moodle/commit/b4a67ebb60f77af3d62851df04d285ce85a074ab

              Show
              cameron1729 Cameron Ball added a comment - Somehow the empty submission check got lost when I was rebasing. Found it in my reflog and added it back: https://github.com/cameron1729/moodle/commit/b4a67ebb60f77af3d62851df04d285ce85a074ab
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks, pulled to master - back to testing.

              Show
              poltawski Dan Poltawski added a comment - Thanks, pulled to master - back to testing.
              Hide
              lameze Simey Lameze added a comment -

              It's failing for me on 3.1 on the individual submission test.

              On step 6, Verify you get an error explaining that you have existing submission data.

              I didn't get any error, both submission were saved and not error message displayed.

              Failing to get attention.

              Show
              lameze Simey Lameze added a comment - It's failing for me on 3.1 on the individual submission test. On step 6, Verify you get an error explaining that you have existing submission data. I didn't get any error, both submission were saved and not error message displayed. Failing to get attention.
              Hide
              cameron1729 Cameron Ball added a comment - - edited

              Hey Simey... Yeah that's expected. From the testing instructions:

              "NB: On master both sets of tests need to be carried out. On 31 and 30 only the test involving the two students and a teacher is needed."

              The empty submission functionality was not backportable as it has new methods etc (see the comment exchange between Dan and I further up).

              Show
              cameron1729 Cameron Ball added a comment - - edited Hey Simey... Yeah that's expected. From the testing instructions: "NB: On master both sets of tests need to be carried out. On 31 and 30 only the test involving the two students and a teacher is needed." The empty submission functionality was not backportable as it has new methods etc (see the comment exchange between Dan and I further up).
              Hide
              lameze Simey Lameze added a comment -

              ohh Sorry Cam, thanks for clairfy that.

              Show
              lameze Simey Lameze added a comment - ohh Sorry Cam, thanks for clairfy that.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks guys, sending this back for testing

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks guys, sending this back for testing
              Hide
              lameze Simey Lameze added a comment -

              Thanks for work on this Cameron Ball, works as expected.

              Show
              lameze Simey Lameze added a comment - Thanks for work on this Cameron Ball , works as expected.
              Hide
              cameron1729 Cameron Ball added a comment -

              Hooray! Thanks all.

              Show
              cameron1729 Cameron Ball added a comment - Hooray! Thanks all.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Closing this as fixed, once it's already out there, upstream. Soon the orange hordes will start enjoying with your awesome changes, many many thanks!

              Each problem that I solved became a rule,
              which served afterwards to solve other problems.
              – René Descartes

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Closing this as fixed, once it's already out there, upstream. Soon the orange hordes will start enjoying with your awesome changes, many many thanks! Each problem that I solved became a rule, which served afterwards to solve other problems. – René Descartes
              Hide
              marina Marina Glancy added a comment -

              reported regression MDL-55376

              Show
              marina Marina Glancy added a comment - reported regression MDL-55376

                People

                • Votes:
                  25 Vote for this issue
                  Watchers:
                  24 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Jul/16