Moodle
  1. Moodle
  2. MDL-38267

Submit button stays active after cut-off date in Assignment

    Details

    • Testing Instructions:
      Hide
      1. Create an assignment with a due date in one minute and a cut off date in 2 minutes time. Set require students click submit to on.
      2. Login as a student and add a submission (quickly before the cut-off date expires)
      3. Verify you now see the submit button.
      4. Wait until after the cut off date and click the button.
      5. You should see an error page saying submissions closed.
      6. Go back to the assignment. You should not see a submit button.
      Show
      Create an assignment with a due date in one minute and a cut off date in 2 minutes time. Set require students click submit to on. Login as a student and add a submission (quickly before the cut-off date expires) Verify you now see the submit button. Wait until after the cut off date and click the button. You should see an error page saying submissions closed. Go back to the assignment. You should not see a submit button.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38267-master

      Description

      Assignment requires students to click submit and has a cut-off date.
      Student uploads file or enters online text before the due date
      The cut-off time passes - students are no longer able to modify their assignment.
      The edit button is no longer visible to the students.
      The submit button is still available to students.
      Student clicks the submit button after the cut-off date which changes the last modified date for the submission and now indicates that the assignment is late by x amount.

      The Submit button should also disappear after the cut-off date.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Damyon Wiese added a comment -

            Thanks for reporting this.

            I've put that on the backlog.

            In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

            Show
            Damyon Wiese added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
            Hide
            Prateek Sachan added a comment - - edited

            Hi, I thought of fixing this issue. I looked up locallib.php in mod/assign directory. I was able to tweak and remove the submit button. But, it displays "This assignment is not accepting submissions" under Submission status and "Assignment is overdue by: 38 mins 20 secs" under Time Remaining.

            So, even if the user cannot submit/edit the submissions, is it wise to display the latter text (Time Remaining)? As of now I'm attaching the links to my commits. There was no check being made for the cutoff date in $showsubmit variable (view_student_summary()). Please guide me in getting a fix for this issue if the one I've provided here is not appropriate.

            master : https://github.com/prateeksachan/moodle/compare/MDL-38267
            MOODLE_24_STABLE : https://github.com/prateeksachan/moodle/compare/MOODLE_24_STABLE...MDL-38267_24

            Show
            Prateek Sachan added a comment - - edited Hi, I thought of fixing this issue. I looked up locallib.php in mod/assign directory. I was able to tweak and remove the submit button. But, it displays "This assignment is not accepting submissions" under Submission status and "Assignment is overdue by: 38 mins 20 secs" under Time Remaining. So, even if the user cannot submit/edit the submissions, is it wise to display the latter text (Time Remaining)? As of now I'm attaching the links to my commits. There was no check being made for the cutoff date in $showsubmit variable ( view_student_summary() ). Please guide me in getting a fix for this issue if the one I've provided here is not appropriate. master : https://github.com/prateeksachan/moodle/compare/MDL-38267 MOODLE_24_STABLE : https://github.com/prateeksachan/moodle/compare/MOODLE_24_STABLE...MDL-38267_24
            Hide
            Aparup Banerjee added a comment -

            Hi Prateek,
            There's one more thing which seems to be described besides not showing the button. The submission should never have gone through fully if a cut-off date was set. (as defined by cut-off date in settings). There may be a bug further down the call trace too, have you checked? There should have been an exception i think which didn't happen.

            Show
            Aparup Banerjee added a comment - Hi Prateek, There's one more thing which seems to be described besides not showing the button. The submission should never have gone through fully if a cut-off date was set. (as defined by cut-off date in settings). There may be a bug further down the call trace too, have you checked? There should have been an exception i think which didn't happen.
            Hide
            Damyon Wiese added a comment -

            (I haven't time to look at this immediately - but here are my thoughts on a correct fix)

            1. in view_student_summary - showsubmit needs to be &&ed with submissions_open().

            2. process_submit_for_grading() and process_save_submission() should check for submissions_open and throw an error.

            3. add a unit test

            Show
            Damyon Wiese added a comment - (I haven't time to look at this immediately - but here are my thoughts on a correct fix) 1. in view_student_summary - showsubmit needs to be &&ed with submissions_open(). 2. process_submit_for_grading() and process_save_submission() should check for submissions_open and throw an error. 3. add a unit test
            Hide
            Prateek Sachan added a comment -

            Thanks Damyon. I followed your advice and have updated the commit.
            As its my first unit test, (if you have time) could you please have a quick look at it? I've not pushed it as I'm not sure whether its the correct way or not. (The tests were successful). If you give it a go I'll update the commit with the unit test as well.

            The unit test is here: https://gist.github.com/prateeksachan/5559789

            Show
            Prateek Sachan added a comment - Thanks Damyon. I followed your advice and have updated the commit. As its my first unit test, (if you have time) could you please have a quick look at it? I've not pushed it as I'm not sure whether its the correct way or not. (The tests were successful). If you give it a go I'll update the commit with the unit test as well. The unit test is here: https://gist.github.com/prateeksachan/5559789
            Hide
            Aparup Banerjee added a comment -

            Hi Prateek, nice to see you back into it

            The unit test looks alright to me aside from:

            • not sure why you had to "Add a submission but don't submit now", it doesn't seem to be testing anything. I'm missing something?
            • theres two extra $now=time(); so i think you probably had in mind to test something more? anyway, i think you're getting the right idea with the unit test.

            nice going , i think Damyon has mentioned which functions should throw errors. Once those two functions are patched, you could add them into the test case too.

            ps: go go Global Search :-D

            Show
            Aparup Banerjee added a comment - Hi Prateek, nice to see you back into it The unit test looks alright to me aside from: not sure why you had to "Add a submission but don't submit now", it doesn't seem to be testing anything. I'm missing something? theres two extra $now=time(); so i think you probably had in mind to test something more? anyway, i think you're getting the right idea with the unit test. nice going , i think Damyon has mentioned which functions should throw errors. Once those two functions are patched, you could add them into the test case too. ps: go go Global Search :-D
            Hide
            Prateek Sachan added a comment - - edited

            "Add a submission but don't submit now" is required as given in the description. The testing requires the student to upload/enter text but does not submit it. (So that the "edit button" is still there). that is why, I've saved it only(save($submission, $data);) and not submitted it ($submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED;)

            The second $now=time() is irrelevant. I forgot to delete it. The gap between cutoffdate and duedate is too big to assign time() again. The functions that Damyon mentioned, I've updated them to throw errors. I'll try adding them in tests now.

            Show
            Prateek Sachan added a comment - - edited "Add a submission but don't submit now" is required as given in the description. The testing requires the student to upload/enter text but does not submit it. (So that the "edit button" is still there). that is why, I've saved it only( save($submission, $data); ) and not submitted it ( $submission->status = ASSIGN_SUBMISSION_STATUS_SUBMITTED; ) The second $now=time() is irrelevant. I forgot to delete it. The gap between cutoffdate and duedate is too big to assign time() again. The functions that Damyon mentioned, I've updated them to throw errors. I'll try adding them in tests now.
            Hide
            Aparup Banerjee added a comment -

            aha thanks for explaining about 'edit button'

            Show
            Aparup Banerjee added a comment - aha thanks for explaining about 'edit button'
            Hide
            Damyon Wiese added a comment -

            This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

            For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            Show
            Damyon Wiese added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
            Hide
            Raymond Antonio added a comment -

            Hi Damyon, Aparup and Prateek,

            I have a look at prateek's patch. I think that's pretty much an implementation of what Damyon suggested above ( initially thought, maybe just added $USER->id in submissions_open's parameter used in process_submit and process_save functions but jut noticed now that this is not needed as there is a checking for it in submissions_open function) and it works as expected. this is my 2 cents

            Regards,

            Raymond

            Show
            Raymond Antonio added a comment - Hi Damyon, Aparup and Prateek, I have a look at prateek's patch. I think that's pretty much an implementation of what Damyon suggested above ( initially thought, maybe just added $USER->id in submissions_open's parameter used in process_submit and process_save functions but jut noticed now that this is not needed as there is a checking for it in submissions_open function) and it works as expected. this is my 2 cents Regards, Raymond
            Hide
            Damyon Wiese added a comment -

            I have just added branches with Prateeks patch (and unit test) which all looked good. I have set a blocker link for this issue because I rebased it on another patch that I knew would conflict (and resolved the conflicts).

            Thanks Prateek, sending for integration.

            Show
            Damyon Wiese added a comment - I have just added branches with Prateeks patch (and unit test) which all looked good. I have set a blocker link for this issue because I rebased it on another patch that I knew would conflict (and resolved the conflicts). Thanks Prateek, sending for integration.
            Hide
            Dan Poltawski added a comment -

            This looks ok, but I am not so sure about these phpunit assertions:

            $this->assertNotContains($output, get_string('editsubmission', 'assign'), 'Cannot edit after cutoff date.');
            $this->assertNotContains($output, get_string('submitassignment', 'assign'), 'Cannot submit after cutoff date.');
            

            I think it'd be better for the last param (the message for failure) to indicate better the failure.

            Show
            Dan Poltawski added a comment - This looks ok, but I am not so sure about these phpunit assertions: $this->assertNotContains($output, get_string('editsubmission', 'assign'), 'Cannot edit after cutoff date.'); $this->assertNotContains($output, get_string('submitassignment', 'assign'), 'Cannot submit after cutoff date.'); I think it'd be better for the last param (the message for failure) to indicate better the failure.
            Hide
            Damyon Wiese added a comment -

            Thanks Dan - I added a commit to change the messages for master and 25 (no tests for 24).

            Show
            Damyon Wiese added a comment - Thanks Dan - I added a commit to change the messages for master and 25 (no tests for 24).
            Hide
            Dan Poltawski added a comment -

            Thanks Damyon and Prateek, integrated to master 25 and 24

            Show
            Dan Poltawski added a comment - Thanks Damyon and Prateek, integrated to master 25 and 24
            Hide
            Dan Poltawski added a comment -

            Tested on all branches, looks good - thanks

            Show
            Dan Poltawski added a comment - Tested on all branches, looks good - thanks
            Hide
            Petr Skoda added a comment -

            This collides badly with MDL-37804, please revert this or that...

            Show
            Petr Skoda added a comment - This collides badly with MDL-37804 , please revert this or that...
            Hide
            Damyon Wiese added a comment -

            This issue along with 77 of it's friends has been sent upstream and released to the world.

            Thankyou for your contribution.

            Show
            Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.
            Hide
            Gilles-Philippe Leblanc added a comment -

            Tested on 2.4 latest stable and I don't see the error message. The draft file is submitted for grading. It is normal ?

            Show
            Gilles-Philippe Leblanc added a comment - Tested on 2.4 latest stable and I don't see the error message. The draft file is submitted for grading. It is normal ?
            Hide
            Colin Campbell added a comment -

            This breaks when using LoggedInAs. Error message is "Cannot call moodle_page::add_body_class after output has been started." The problem is that render_assign_header gets called in this stack due to new code.

            line 215 of /lib/outputrenderers.php: call to mod_assign_renderer->render_assign_header()
            line 2648 of /mod/assign/locallib.php: call to plugin_renderer_base->render()
            line 3612 of /mod/assign/locallib.php: call to assign->view_student_error_message()
            line 345 of /mod/assign/locallib.php: call to assign->process_submit_for_grading()
            line 53 of /mod/assign/view.php: call to assign->view()

            And then again in this stack
            line 215 of /lib/outputrenderers.php: call to mod_assign_renderer->render_assign_header()\n
            line 3067 of /mod/assign/locallib.php: call to plugin_renderer_base->render()\n
            line 426 of /mod/assign/locallib.php: call to assign->view_submission_page()\n
            line 53 of /mod/assign/view.php: call to assign->view()\n

            view_student_error_message was apparently not designed to be called prior to calling view_submission_page() or similar view functions. It's other use is a call from within a view function where the semantics make sense.

            Show
            Colin Campbell added a comment - This breaks when using LoggedInAs. Error message is "Cannot call moodle_page::add_body_class after output has been started." The problem is that render_assign_header gets called in this stack due to new code. line 215 of /lib/outputrenderers.php: call to mod_assign_renderer->render_assign_header() line 2648 of /mod/assign/locallib.php: call to plugin_renderer_base->render() line 3612 of /mod/assign/locallib.php: call to assign->view_student_error_message() line 345 of /mod/assign/locallib.php: call to assign->process_submit_for_grading() line 53 of /mod/assign/view.php: call to assign->view() And then again in this stack line 215 of /lib/outputrenderers.php: call to mod_assign_renderer->render_assign_header()\n line 3067 of /mod/assign/locallib.php: call to plugin_renderer_base->render()\n line 426 of /mod/assign/locallib.php: call to assign->view_submission_page()\n line 53 of /mod/assign/view.php: call to assign->view()\n view_student_error_message was apparently not designed to be called prior to calling view_submission_page() or similar view functions. It's other use is a call from within a view function where the semantics make sense.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: