Moodle
  1. Moodle
  2. MDL-38267

Submit button stays active after cut-off date in Assignment

    Details

    • Rank:
      48117

      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.

        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 Škoda added a comment -

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

          Show
          Petr Škoda 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: