Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.4
    • Component/s: Assignment
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Ideally this would be tested on Oracle & MSSQL

      1. Create and Assignment
      2. Set the due date to 1 weeks time, set the cut off date to 1 weeks time (both default)
      3. Login as a student, verify you can submit to the assignment
      4. Login as a teacher and edit the settings, change the due date to yesterday
      5. Login as a student, the assignment should say "Time remaining | Assignment is overdue by: 1 day", and still let you submit
      6. Login as a teacher and edit the settings, change the cut off date to yesterday
      7. Login as a student, the assignment should say "Time remaining | Assignment is overdue by: 1 day" and you should not be able to submit
      8. Login as a teacher and grant the student an extension until tomorrow (assignment grading interface - choose grant extension from the menu against each submission)
      9. Login as a student, the assignment should say "Extension due date | (date), Time remaining | (some number of hours) ", and still let you submit
      10. Login as a teacher and try and grant an extension before the duedate/cutoffdate. Form should fail validation and not let you save the extension.
      Show
      Ideally this would be tested on Oracle & MSSQL Create and Assignment Set the due date to 1 weeks time, set the cut off date to 1 weeks time (both default) Login as a student, verify you can submit to the assignment Login as a teacher and edit the settings, change the due date to yesterday Login as a student, the assignment should say "Time remaining | Assignment is overdue by: 1 day", and still let you submit Login as a teacher and edit the settings, change the cut off date to yesterday Login as a student, the assignment should say "Time remaining | Assignment is overdue by: 1 day" and you should not be able to submit Login as a teacher and grant the student an extension until tomorrow (assignment grading interface - choose grant extension from the menu against each submission) Login as a student, the assignment should say "Extension due date | (date), Time remaining | (some number of hours) ", and still let you submit Login as a teacher and try and grant an extension before the duedate/cutoffdate. Form should fail validation and not let you save the extension.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-31295-POST23
    • Rank:
      37760

      Description

      Allow individual submission date extensions for students.

      1. assignment_cut_off_date.docx
        127 kB
        Helen Foster
      2. patch-assing23-ulpgc.txt
        50 kB
        Enrique Castro
      3. smurf.xml
        18 kB
        Dan Poltawski

        Issue Links

          Activity

          Hide
          Grette Wilkinson added a comment -

          Could this feature work for individual students, groups of students, or whole classes?

          The extension date needs to be displayed to markers on the submission overview screen...and any statuses (ie. submitted late/early) calculated from the extension date.

          Show
          Grette Wilkinson added a comment - Could this feature work for individual students, groups of students, or whole classes? The extension date needs to be displayed to markers on the submission overview screen...and any statuses (ie. submitted late/early) calculated from the extension date.
          Hide
          Damyon Wiese added a comment -

          The wiki page for the feature is updated with a walkthrough and screenshots.

          Show
          Damyon Wiese added a comment - The wiki page for the feature is updated with a walkthrough and screenshots.
          Hide
          Stephen Bourget added a comment -

          I looked at the Wiki page with the current screenshots and have one question... Is there a way to grant an extension to all members of a group or do they have to be done on a user by user basis?

          For example if I have a course with four groups (G1, G2, G3, and G4) of students, 25 users per group and I decide that all students in group 3 (G3) need a one week extension of the due date can that be done in bulk or do you have to go into every student individually to grant the extension?

          Show
          Stephen Bourget added a comment - I looked at the Wiki page with the current screenshots and have one question... Is there a way to grant an extension to all members of a group or do they have to be done on a user by user basis? For example if I have a course with four groups (G1, G2, G3, and G4) of students, 25 users per group and I decide that all students in group 3 (G3) need a one week extension of the due date can that be done in bulk or do you have to go into every student individually to grant the extension?
          Hide
          Michael Hughes added a comment -

          The observation that I'd like to make is that this shouldn't be part of mod_assign it should be a core feature, as extensions should be able to be applied to any activity (that has a deadline). Obviously this has knock on effects on every mod (but I'll touch upon how I think this could be done without touching them).

          Also it would then allow extensions to be granted to
          1)a user/group(i.e. extends all of a users'/group's activities' (in every course) submission dates by a set amount),
          2) a category (all activities in all courses the user is enrolled in, in a category) ,
          3) a course (all activities in a single course)
          4) a user or group for multiple activities and
          5) a user or group for a single activity

          Also generally extensions would attract some sort of sanction or penalty e.g. 10% penalty or -5marks. So it strikes me that taking into account an extension is really an administrative action that affects the determining of @final_grade@ (so in my mind implies changes to the @grade_update_grade@.

          Applying a penalty to a submissions grade at the activity level (@raw_grade@) skews comparison of the performance of the task, as it means that 2 students submitting the same quality of work would get different grade, whilst actually being of the same grade. Applying penalties when getting the @final_grade@ means that this is an administrative sanction rather than one against performance.

          So it strikes me that associating the application of extension penalties in the Gradebook (/grade & gradelib.php) would immediately allow this to apply to any activity. The granting/management of extensions and sanctions could be separated out (e.g. into a /extensions core plugin).

          The change to @grade_update()@ would check if there is an extension for a grade item (hence activity) for a user, apply the penalty(penalties) and then store the modified @raw_grade@ in @final_grade@.

          Show
          Michael Hughes added a comment - The observation that I'd like to make is that this shouldn't be part of mod_assign it should be a core feature, as extensions should be able to be applied to any activity (that has a deadline). Obviously this has knock on effects on every mod (but I'll touch upon how I think this could be done without touching them). Also it would then allow extensions to be granted to 1)a user/group(i.e. extends all of a users'/group's activities' (in every course) submission dates by a set amount), 2) a category (all activities in all courses the user is enrolled in, in a category) , 3) a course (all activities in a single course) 4) a user or group for multiple activities and 5) a user or group for a single activity Also generally extensions would attract some sort of sanction or penalty e.g. 10% penalty or -5marks. So it strikes me that taking into account an extension is really an administrative action that affects the determining of @final_grade@ (so in my mind implies changes to the @grade_update_grade@. Applying a penalty to a submissions grade at the activity level (@raw_grade@) skews comparison of the performance of the task, as it means that 2 students submitting the same quality of work would get different grade, whilst actually being of the same grade. Applying penalties when getting the @final_grade@ means that this is an administrative sanction rather than one against performance. So it strikes me that associating the application of extension penalties in the Gradebook (/grade & gradelib.php) would immediately allow this to apply to any activity. The granting/management of extensions and sanctions could be separated out (e.g. into a /extensions core plugin). The change to @grade_update()@ would check if there is an extension for a grade item (hence activity) for a user, apply the penalty(penalties) and then store the modified @raw_grade@ in @final_grade@.
          Hide
          Damyon Wiese added a comment -

          Hi Michael,

          I considered where the best place might be to put this functionality and I think this still fits best within the assign module. The reason is that the different activities use due dates in different ways - e.g. the workshop has a due date for submissions and then another one for the marking process. Forcing all activities to handle this in the same way could prevent activity modules from supporting special features (e.g. extension dates would need special handling for an assignment where students can submit in teams).

          As for automatically applying a penalty - what I have suggested is just that the fact that an assignment extension has been granted is clearly marked on all grading interfaces so the marker can decide how to apply a penalty or not (e.g. there is a difference if a student has supplied a medical certificate or is just late and should be penalised). If we are going to automatically apply penalties it could possibly be done on the grant extension page (e.g. set a penalty at the same time as granting an extension).

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Michael, I considered where the best place might be to put this functionality and I think this still fits best within the assign module. The reason is that the different activities use due dates in different ways - e.g. the workshop has a due date for submissions and then another one for the marking process. Forcing all activities to handle this in the same way could prevent activity modules from supporting special features (e.g. extension dates would need special handling for an assignment where students can submit in teams). As for automatically applying a penalty - what I have suggested is just that the fact that an assignment extension has been granted is clearly marked on all grading interfaces so the marker can decide how to apply a penalty or not (e.g. there is a difference if a student has supplied a medical certificate or is just late and should be penalised). If we are going to automatically apply penalties it could possibly be done on the grant extension page (e.g. set a penalty at the same time as granting an extension). Regards, Damyon
          Hide
          Michael Hughes added a comment - - edited

          I think that when mods should be specifying an expected due date for the activity they are implement and I accept the use of due dates may be used by mods for other specific purposes. I don't think that each mod should have to maintain per-user deadline exceptions themselves.

          Given that the obvious deadlines typically affects submitting users then for the workshop example given then dates specified for marking would not be affected (or the module could be enhanced to take an check for an exception and take it into account and slide the marking date appropriately).

          My concern is that by having different mechanisms for granting extensions depending on the mod being used makes for unhappier end-users (in our case).

          There are only 2 things that Moodle currently supports as "entities that can submit work" (technically 1 until group assignments are in) and these are User and Groups, then surely having centralised tables to hold these exceptions for the wider use makes more use than each mod having to handle this information separately.

          So the new mod_assign could query against against this table(s) when generating a list view of assignments submissions.

          This would permit current implementations but also allow for progressive enhancements to make use of this data as they see fit. The fact that mods should be reporting what features they support (via @*_supports()@) means that mods could explicitly say they permit extensions via the core aspect or use their own system by defaulting a @FEATURE_EXTENSIONS@ option to @false@ until support is more widely adopted by mods.

          Also from our experience administration of extensions by staff is done on a per-user basis rather than per-activity.

          I'm not fully convinced that this data should be so closely tied up to the mod_assign as the relationship between an assignment and extensions seems to me to be more akin to the relationship between an assignments and groups (which are effectively course-level data structures).

          A thought that just occurs to me now is that a better location for this data to be held would actually be in amongst the User data tables.

          OH I agree that any penalty that should be applied (manually or otherwise) should be recorded against the extension being granted (and predict that some mechanism to manage pre-set penalties, much like the system for Learning Outcomes & Scales) would end up being requested).

          Show
          Michael Hughes added a comment - - edited I think that when mods should be specifying an expected due date for the activity they are implement and I accept the use of due dates may be used by mods for other specific purposes. I don't think that each mod should have to maintain per-user deadline exceptions themselves. Given that the obvious deadlines typically affects submitting users then for the workshop example given then dates specified for marking would not be affected (or the module could be enhanced to take an check for an exception and take it into account and slide the marking date appropriately). My concern is that by having different mechanisms for granting extensions depending on the mod being used makes for unhappier end-users (in our case). There are only 2 things that Moodle currently supports as "entities that can submit work" (technically 1 until group assignments are in) and these are User and Groups, then surely having centralised tables to hold these exceptions for the wider use makes more use than each mod having to handle this information separately. So the new mod_assign could query against against this table(s) when generating a list view of assignments submissions. This would permit current implementations but also allow for progressive enhancements to make use of this data as they see fit. The fact that mods should be reporting what features they support (via @*_supports()@) means that mods could explicitly say they permit extensions via the core aspect or use their own system by defaulting a @FEATURE_EXTENSIONS@ option to @false@ until support is more widely adopted by mods. Also from our experience administration of extensions by staff is done on a per-user basis rather than per-activity. I'm not fully convinced that this data should be so closely tied up to the mod_assign as the relationship between an assignment and extensions seems to me to be more akin to the relationship between an assignments and groups (which are effectively course-level data structures). A thought that just occurs to me now is that a better location for this data to be held would actually be in amongst the User data tables. OH I agree that any penalty that should be applied (manually or otherwise) should be recorded against the extension being granted (and predict that some mechanism to manage pre-set penalties, much like the system for Learning Outcomes & Scales) would end up being requested).
          Hide
          Michael Hughes added a comment - - edited

          Hoping not to state the obvious but it seems that this conversation is duplicates a lot of the work going on for http://tracker.moodle.org/browse/MDL-7315 ...?

          Show
          Michael Hughes added a comment - - edited Hoping not to state the obvious but it seems that this conversation is duplicates a lot of the work going on for http://tracker.moodle.org/browse/MDL-7315 ...?
          Hide
          Damyon Wiese added a comment -

          This needs a better description and testing instructions. The code has been cleaned up and the git information is correct - but it is based on Moodle master branch with other fixes cherry-picked on top. I will wait for the other fixes to be integrated before submitting this for review.

          Show
          Damyon Wiese added a comment - This needs a better description and testing instructions. The code has been cleaned up and the git information is correct - but it is based on Moodle master branch with other fixes cherry-picked on top. I will wait for the other fixes to be integrated before submitting this for review.
          Hide
          Enrique Castro added a comment -

          Hi,
          I think the proposed patch add TWO separate changes that must be considered separately. One thing in the due date extension and the other is the change of preventlate for datecutoff.

          Being able to set different due dates for individual users is a highly asked demand. The addition of extensionduedate to assign_grades table is a good way to manage this demand. It works.

          However, this is NOT related to changing preventlate setting into cutoff due date. This change is INDEPENDENT of the above. You can "preventlate" whenever the real due date is. Most teachers are used to "preventlate" setting. If not enables submissions can proceed without limitation. If and when the teacher feels there is a neccessity fro limitation, he simply has to enable "preventlate" and all submissions are terminated. Just one initial setting ans a single intervention when a limit is needed.

          However, with cutoff setting the teacher need to know in advance how long to allow submissions, the cutoff date. If wrong, or dynamic in nature (e.g. course sections, terms) she may need to change the setting several times.

          I would prefer to keep "preventlate" setting and introduce duedate extension managing on top of duedate/preventlate. I have attached a patch against current assign 2.3 (as 2012-06-25) that just do that: adding dude date extension in an individual user basis, without touching other settings.

          Show
          Enrique Castro added a comment - Hi, I think the proposed patch add TWO separate changes that must be considered separately. One thing in the due date extension and the other is the change of preventlate for datecutoff. Being able to set different due dates for individual users is a highly asked demand. The addition of extensionduedate to assign_grades table is a good way to manage this demand. It works. However, this is NOT related to changing preventlate setting into cutoff due date. This change is INDEPENDENT of the above. You can "preventlate" whenever the real due date is. Most teachers are used to "preventlate" setting. If not enables submissions can proceed without limitation. If and when the teacher feels there is a neccessity fro limitation, he simply has to enable "preventlate" and all submissions are terminated. Just one initial setting ans a single intervention when a limit is needed. However, with cutoff setting the teacher need to know in advance how long to allow submissions, the cutoff date. If wrong, or dynamic in nature (e.g. course sections, terms) she may need to change the setting several times. I would prefer to keep "preventlate" setting and introduce duedate extension managing on top of duedate/preventlate. I have attached a patch against current assign 2.3 (as 2012-06-25) that just do that: adding dude date extension in an individual user basis, without touching other settings.
          Hide
          Enrique Castro added a comment -

          Patch against mod/assign/ in moodle 2.3 rc1 (2012-06-25)
          Duedate extensions by user

          Show
          Enrique Castro added a comment - Patch against mod/assign/ in moodle 2.3 rc1 (2012-06-25) Duedate extensions by user
          Hide
          Grette Wilkinson added a comment -

          I agree that there are two separate changes, that being able to set different due dates for users is a great feature and that this feature is independent of the management of 'late' submissions.

          However I think there is a valid argument for the addition of cutoff date. In many cases teachers do know in advance how long to allow late submissions, eg. this might be 5 days past the due date, after which date no further submissions will be accepted unless the student has been granted an extension. This is often governed by institutional policy. The issue with the current preventlate is that students could submit after the marking process has commenced, meaning papers may get missed in the marking process, or (if the submit button is disabled) students may make changes to papers after their initial submission.

          I accept the point that preventlate works well for a dynamic course, however in that case would the teacher not disable both due date and cut-off date? Or set the cut-off date to coincide with the end of the academic year?

          Show
          Grette Wilkinson added a comment - I agree that there are two separate changes, that being able to set different due dates for users is a great feature and that this feature is independent of the management of 'late' submissions. However I think there is a valid argument for the addition of cutoff date. In many cases teachers do know in advance how long to allow late submissions, eg. this might be 5 days past the due date, after which date no further submissions will be accepted unless the student has been granted an extension. This is often governed by institutional policy. The issue with the current preventlate is that students could submit after the marking process has commenced, meaning papers may get missed in the marking process, or (if the submit button is disabled) students may make changes to papers after their initial submission. I accept the point that preventlate works well for a dynamic course, however in that case would the teacher not disable both due date and cut-off date? Or set the cut-off date to coincide with the end of the academic year?
          Hide
          Damyon Wiese added a comment -

          Thanks for the patch you posted Enrique - that resubmissions plugin looks interesting. Could you please split that into a separate patch and attach it to a new Jira issue?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Thanks for the patch you posted Enrique - that resubmissions plugin looks interesting. Could you please split that into a separate patch and attach it to a new Jira issue? Thanks, Damyon
          Hide
          Enrique Castro added a comment -

          Hi Damyon, please see MDL-34881 for resubmission history plugin

          Show
          Enrique Castro added a comment - Hi Damyon, please see MDL-34881 for resubmission history plugin
          Hide
          Damyon Wiese added a comment -

          Can someone peer review this - it is blocking several other issues?

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Can someone peer review this - it is blocking several other issues? Thanks, Damyon
          Hide
          Damyon Wiese added a comment -

          Raising this to blocker as it is holding up integration of several features (I will set all the blocker links).

          Show
          Damyon Wiese added a comment - Raising this to blocker as it is holding up integration of several features (I will set all the blocker links).
          Hide
          Damyon Wiese added a comment -

          Never mind - just found some rebase problems - fixing these now.

          Show
          Damyon Wiese added a comment - Never mind - just found some rebase problems - fixing these now.
          Hide
          Damyon Wiese added a comment -

          Rebased again and ready for review.

          Show
          Damyon Wiese added a comment - Rebased again and ready for review.
          Hide
          Adrian Greeve added a comment -

          Hi Damyon,

          I had a look through the code and it looks good.
          Possible things to consider:

          1. mod/assign/renderer.php on line 275:
            $late = get_string('nomoresubmissionsaccepted', 'assign');
            

            This string doesn't exist. I'm guessing that it should be nosubmissionaccepted which does.

          2. I think it would be nice if the cut-off date defaulted to the same value as the due date.
          3. Setting the extension date before the due date and the allow submissions from date doesn't throw an error, it just returns to the page. It looks like you have code there to handle this. I haven't had a close enough look to see why that isn't working.
          4. I'm not sure if this is in the scope of this issue, but allowing extensions seems like something that should be logged.

          Thanks again.

          Show
          Adrian Greeve added a comment - Hi Damyon, I had a look through the code and it looks good. Possible things to consider: mod/assign/renderer.php on line 275: $late = get_string('nomoresubmissionsaccepted', 'assign'); This string doesn't exist. I'm guessing that it should be nosubmissionaccepted which does. I think it would be nice if the cut-off date defaulted to the same value as the due date. Setting the extension date before the due date and the allow submissions from date doesn't throw an error, it just returns to the page. It looks like you have code there to handle this. I haven't had a close enough look to see why that isn't working. I'm not sure if this is in the scope of this issue, but allowing extensions seems like something that should be logged. Thanks again.
          Hide
          Damyon Wiese added a comment -

          Thanks Adrian,

          I have addressed all these issues and rebased the branch again.

          Can you take another look?

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks Adrian, I have addressed all these issues and rebased the branch again. Can you take another look? Regards, Damyon
          Hide
          Adrian Greeve added a comment -

          Hi Damyon,

          Thanks for the quick fix of the problems.
          I'm really sorry, but I just have one more problem.

          • When granting an extension the form will submit, but it doesn't tell the user that it has saved. I think that when I tested this yesterday the page would return back to the grading screen (Sorry I honestly can't remember exactly what it did).

          Everything else is great. If you could fix that and then send it though to integration.

          Show
          Adrian Greeve added a comment - Hi Damyon, Thanks for the quick fix of the problems. I'm really sorry, but I just have one more problem. When granting an extension the form will submit, but it doesn't tell the user that it has saved. I think that when I tested this yesterday the page would return back to the grading screen (Sorry I honestly can't remember exactly what it did). Everything else is great. If you could fix that and then send it though to integration.
          Hide
          Damyon Wiese added a comment -

          Thanks Adrian - my last change added logging and I accidentally removed the return from the function which caused that bug. This is all fixed now - I'll just write some test instructions and submit for integration.

          Show
          Damyon Wiese added a comment - Thanks Adrian - my last change added logging and I accidentally removed the return from the function which caused that bug. This is all fixed now - I'll just write some test instructions and submit for integration.
          Hide
          Damyon Wiese added a comment -

          Ready for integration, thanks again Adrian.

          Show
          Damyon Wiese added a comment - Ready for integration, thanks again Adrian.
          Hide
          Dan Poltawski added a comment -

          Hi Damyon,

          There are numerous problems detected by the codechecker, i've attached the 'surf.xml' file with the report of problems. (I'm afraid the best thing is to open in browser window, the plan is for this report to be generated and displayed automatically, but for now we just have the raw file)

          Some are very minor, but there are a few more serious which would be great to fix up since this is new code.

          Show
          Dan Poltawski added a comment - Hi Damyon, There are numerous problems detected by the codechecker, i've attached the 'surf.xml' file with the report of problems. (I'm afraid the best thing is to open in browser window, the plan is for this report to be generated and displayed automatically, but for now we just have the raw file) Some are very minor, but there are a few more serious which would be great to fix up since this is new code.
          Hide
          Damyon Wiese added a comment -

          Hi Dan,

          I have pushed to the branch again with the changes listed from the smurf.xml file except for some false positives listed in that file.

          It included complaints about some doc block comments on variables which were valid comments (e.g. renderer.php line 423).

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Dan, I have pushed to the branch again with the changes listed from the smurf.xml file except for some false positives listed in that file. It included complaints about some doc block comments on variables which were valid comments (e.g. renderer.php line 423). Regards, Damyon
          Hide
          Dan Poltawski added a comment - - edited

          Hi Damyon,

          Thanks for doing that so quickly, but sorry on closer inspection i've realised something major is missing.

          • You are dropping the preventlatesubmissions setting, but as far as I can tell, you are not upgrading existing assignments so that this is reflected in the cutoff date? So we'll be breaking all the existing created assignments ettings in this regard?

          Additionally some minor things:

          • Should the grantextensions clone permissions from one of the existing assignment permissions, to ensure users who could previously set preventlatesubmissions can do now too.
          • I noticed that the comments are wrong in the upgrade.php

          Sorry I didn't spot those issues earlier, but unless i'm missing something really that first point seems quite major, so reopening.

          Show
          Dan Poltawski added a comment - - edited Hi Damyon, Thanks for doing that so quickly, but sorry on closer inspection i've realised something major is missing. You are dropping the preventlatesubmissions setting, but as far as I can tell, you are not upgrading existing assignments so that this is reflected in the cutoff date? So we'll be breaking all the existing created assignments ettings in this regard? Additionally some minor things: Should the grantextensions clone permissions from one of the existing assignment permissions, to ensure users who could previously set preventlatesubmissions can do now too. I noticed that the comments are wrong in the upgrade.php Sorry I didn't spot those issues earlier, but unless i'm missing something really that first point seems quite major, so reopening.
          Hide
          CiBoT added a comment -

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

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

          Hi Dan:
          I was all in favor of "preventlate" way of manage this topic, but once cutoffdate is the setted consensus I think that it would be unrealistic to try to address ALL problems around the change with upgrade.php code. The change need to be addressed in DOCUMENTATION.

          Preventlate was unlimited. If not set students could submit without any date limit. If set, the due date was strictly enforced. If nothing is done, that's the behavior if cutoffdate is NOT enabled (as default). That's OK.

          What could be addressed would be assignment instances with preventlate=0 (= submissions open). Then a cutoffdate could be set but ¿when to stop submissions? We do not have a general course termination date. Perhaps adding setting a cutoffdate = duedate+one_year may do the trick: every existing non-limited assignment will still continue accepting submissions, and a natural year is usually more than an academic term in most contexts.

          Show
          Enrique Castro added a comment - Hi Dan: I was all in favor of "preventlate" way of manage this topic, but once cutoffdate is the setted consensus I think that it would be unrealistic to try to address ALL problems around the change with upgrade.php code. The change need to be addressed in DOCUMENTATION. Preventlate was unlimited. If not set students could submit without any date limit. If set, the due date was strictly enforced. If nothing is done, that's the behavior if cutoffdate is NOT enabled (as default). That's OK. What could be addressed would be assignment instances with preventlate=0 (= submissions open). Then a cutoffdate could be set but ¿when to stop submissions? We do not have a general course termination date. Perhaps adding setting a cutoffdate = duedate+one_year may do the trick: every existing non-limited assignment will still continue accepting submissions, and a natural year is usually more than an academic term in most contexts.
          Hide
          Damyon Wiese added a comment -

          Hi Dan,

          I'll add an upgrade step to set the cutoffdate to the due date if preventlate submissions was enabled.

          • Damyon
          Show
          Damyon Wiese added a comment - Hi Dan, I'll add an upgrade step to set the cutoffdate to the due date if preventlate submissions was enabled. Damyon
          Hide
          Enrique Castro added a comment -

          Hi Damyon:
          I think that setting cutoffdate=duedate when preventlate=1 is useless. If preventlate is set then the students cannot submit at all after due date. That's exactly the behavior when duedate is set and cutoffdate is NOT enabled.

          The problem on upgrade is just the opposite: what to do when preventlate is NOT set and submissions are to be ALLOWED.

          Show
          Enrique Castro added a comment - Hi Damyon: I think that setting cutoffdate=duedate when preventlate=1 is useless. If preventlate is set then the students cannot submit at all after due date. That's exactly the behavior when duedate is set and cutoffdate is NOT enabled. The problem on upgrade is just the opposite: what to do when preventlate is NOT set and submissions are to be ALLOWED.
          Hide
          Damyon Wiese added a comment -

          Thanks Enrique - you are right.

          I am actually in favour of changing the way this works when there is a due date and no cutoffdate (should allow late submissions and flag them as late).

          I'll update the patch to work like this (and then we can fully support upgrades).

          • Damyon
          Show
          Damyon Wiese added a comment - Thanks Enrique - you are right. I am actually in favour of changing the way this works when there is a due date and no cutoffdate (should allow late submissions and flag them as late). I'll update the patch to work like this (and then we can fully support upgrades). Damyon
          Hide
          Damyon Wiese added a comment -

          This patch changes the logic to what I described in my last comment and updates the upgrade (and assignment upgrade tool as well) to set the cutoff date to the duedate if preventlate is set.

          I have updated all the help and changed the assignment overview page to make it a bit clearer.

          I tested this with 3 upgrades from Assignment 2.2. (no due date, due date but not prevent late, due date and prevent late)
          I tested this with 3 upgrades from an older Assignment 2.3. (no due date, due date but not prevent late, due date and prevent late)

          All working correctly.
          Note: integration currently has a completion bug preventing saving assignment settings - I worked around this for now as there is a patch submitted in a separate ticket.

          Show
          Damyon Wiese added a comment - This patch changes the logic to what I described in my last comment and updates the upgrade (and assignment upgrade tool as well) to set the cutoff date to the duedate if preventlate is set. I have updated all the help and changed the assignment overview page to make it a bit clearer. I tested this with 3 upgrades from Assignment 2.2. (no due date, due date but not prevent late, due date and prevent late) I tested this with 3 upgrades from an older Assignment 2.3. (no due date, due date but not prevent late, due date and prevent late) All working correctly. Note: integration currently has a completion bug preventing saving assignment settings - I worked around this for now as there is a patch submitted in a separate ticket.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Damyon Wiese added a comment -

          Rebased

          Show
          Damyon Wiese added a comment - Rebased
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Daymon (et all),

          I was going to integrate this but found a bunch of things that should be sorted out. Here they are:

          1) Note there will be one conflict with your code and master, so, in your next rebase (24h form now) you'll have to fix it. It's @ mod/assign/gradingtable.php#93. You will have to keep both firstname and lastname out from the list to avoid some problems with Oracle (see MDL-34192).

          2) No matter that conflict, there are some points related with restore that are not being handled properly in your patch:

          • On restore, you have to mimic exactly the same logic used on upgrade. So, if the backup file being restored is old and has preventlatesubmissions enabled, then you must perform the cutoffdate = duedate, exactly the same that upgrade does.
          • Both assign->cutoffdate and assign_grades->extensionduedate (the 2 new columns introduced by the patch) are unix times, hence you need to add support on restore to be able to roll dates automatically (aka, ->apply_date_offset()). That's missing.

          3) As Dan spotted before... isn't there any other capability that we could define as a good target to clone permissions from when creating the new 'grantextension' capability? Also note that non-editing teacher archetype has perms to grade but not to grant extension (just in case it wrong, that's your decision).

          4) In the install.xml file you've defined assign_grades->extensionduedate as nullable, but in the corresponding upgrade.php code you are defining it as not nullable. They must be 100% the same definition, surely not nullable, like the rest of dates there. Please, please, always use the XMLDB editor both both changes in install.xml files and to get the correct upgrade code.

          5) More on upgrade, as it seems you've been copying and pasting code... the comments there are wrong, please review them and fix the column name to the correct one (once again, lol, the XMLDB editor is your friend to avoid these problems).

          6) The upgrade step performing the cutoffdate = duedate has some serious problems:

          • It is performing one count that is used for... nothing.
          • It is incrementing one $i variable for... nothing.
          • You are going and updating the assignments 1 by 1. Why cannot one simpler and way faster update be used instead:
            UPDATE {assign} SET cutoffdate = duedate WHERE preventlatesubmissions = 1
            

          7) Tiny, add one blank line between upgrade steps , and also within the upgrade block, to separate different tasks, each one with its comment (helps for readability a lot).

          8) When looking code... what happens if extensionduedate > cutoffdate ? It seems that is never checked (unless I've missed it) and obviously it does not have sense to grant one extensionduedate beyond the global cutoffdate. In other terms, extensionduedate should be always between duedate and cutoffdate it the later is set, isn' it? And also, you should not allow to modify cutoffdate in the activity if there are already extensionduedate granted for students being > than the candidate cutoffdate.

          And those are all the "structural" things I've been able to find in your current patch. The rest of code looks formally correct (and testing should tell us if works as expected).

          So I'm reopening this to address the points above, many thanks for the hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Daymon (et all), I was going to integrate this but found a bunch of things that should be sorted out. Here they are: 1) Note there will be one conflict with your code and master, so, in your next rebase (24h form now) you'll have to fix it. It's @ mod/assign/gradingtable.php#93. You will have to keep both firstname and lastname out from the list to avoid some problems with Oracle (see MDL-34192 ). 2) No matter that conflict, there are some points related with restore that are not being handled properly in your patch: On restore, you have to mimic exactly the same logic used on upgrade. So, if the backup file being restored is old and has preventlatesubmissions enabled, then you must perform the cutoffdate = duedate, exactly the same that upgrade does. Both assign->cutoffdate and assign_grades->extensionduedate (the 2 new columns introduced by the patch) are unix times, hence you need to add support on restore to be able to roll dates automatically (aka, ->apply_date_offset()). That's missing. 3) As Dan spotted before... isn't there any other capability that we could define as a good target to clone permissions from when creating the new 'grantextension' capability? Also note that non-editing teacher archetype has perms to grade but not to grant extension (just in case it wrong, that's your decision). 4) In the install.xml file you've defined assign_grades->extensionduedate as nullable, but in the corresponding upgrade.php code you are defining it as not nullable. They must be 100% the same definition, surely not nullable, like the rest of dates there. Please, please, always use the XMLDB editor both both changes in install.xml files and to get the correct upgrade code. 5) More on upgrade, as it seems you've been copying and pasting code... the comments there are wrong, please review them and fix the column name to the correct one (once again, lol, the XMLDB editor is your friend to avoid these problems). 6) The upgrade step performing the cutoffdate = duedate has some serious problems: It is performing one count that is used for... nothing. It is incrementing one $i variable for... nothing. You are going and updating the assignments 1 by 1. Why cannot one simpler and way faster update be used instead: UPDATE {assign} SET cutoffdate = duedate WHERE preventlatesubmissions = 1 7) Tiny, add one blank line between upgrade steps , and also within the upgrade block, to separate different tasks, each one with its comment (helps for readability a lot). 8) When looking code... what happens if extensionduedate > cutoffdate ? It seems that is never checked (unless I've missed it) and obviously it does not have sense to grant one extensionduedate beyond the global cutoffdate. In other terms, extensionduedate should be always between duedate and cutoffdate it the later is set, isn' it? And also, you should not allow to modify cutoffdate in the activity if there are already extensionduedate granted for students being > than the candidate cutoffdate. And those are all the "structural" things I've been able to find in your current patch. The rest of code looks formally correct (and testing should tell us if works as expected). So I'm reopening this to address the points above, many thanks for the hard work! Ciao
          Hide
          CiBoT added a comment -

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

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

          Thanks Eloy,

          I have looked at all these issues and updated the branch.

          More detail about the points you raised.

          1. I haven't touched this - I'll till integration gets updated and rebase this again then.

          2. a) Restore code now correctly handles preventlatesubmissions the same as upgrade.
          b) apply_date_offset added to both fields

          3. I don't really have a sense if whether non-editing teachers should have this permission or not - but it makes sense if they can grade they should be able to grant extensions. I added non-editing teacher to the list. I also added a line to clone the permissions from grade/graderreport:view

          4. I made the column NOT NULL and loaded and saved the xml in the xmldb editor.

          5. Some columns were missing comments (Added) but I'm not sure which was the one you saw that had the wrong comment.

          6. Changed to do a direct SQL update - I was just trying to avoid using SQL directly - but I agree this is better.

          7. Blank lines added

          8. Extension date is allowed to be past the cutoffdate - this is expected behaviour.

          Show
          Damyon Wiese added a comment - Thanks Eloy, I have looked at all these issues and updated the branch. More detail about the points you raised. 1. I haven't touched this - I'll till integration gets updated and rebase this again then. 2. a) Restore code now correctly handles preventlatesubmissions the same as upgrade. b) apply_date_offset added to both fields 3. I don't really have a sense if whether non-editing teachers should have this permission or not - but it makes sense if they can grade they should be able to grant extensions. I added non-editing teacher to the list. I also added a line to clone the permissions from grade/graderreport:view 4. I made the column NOT NULL and loaded and saved the xml in the xmldb editor. 5. Some columns were missing comments (Added) but I'm not sure which was the one you saw that had the wrong comment. 6. Changed to do a direct SQL update - I was just trying to avoid using SQL directly - but I agree this is better. 7. Blank lines added 8. Extension date is allowed to be past the cutoffdate - this is expected behaviour.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Damyon Wiese added a comment -

          Rebased

          Show
          Damyon Wiese added a comment - Rebased
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested on master under MySQL, PostgreSQL, Oracle and MS SQL.

          Some feedback for your consideration, Damyon.

          • When applying an extension to multiple students, it is quite a deliberate act to select students, select Grant extension, then to click Go. I don't see any point in confirming this action.
          • On the extension page, the extension date should be enabled by default. I assume most teachers will want to set an extension date.
          • I assume that if you don't enable the due date, the extension is indefinite. This isn't really intuitive. Perhaps a better mechanism could be considered, rather than enabling/disabling the date. Perhaps there could be an control for "Due by a specific date" or "Indefinite extension" and have the extension date dependent on that.
          Show
          Michael de Raadt added a comment - Test result: Success! Tested on master under MySQL, PostgreSQL, Oracle and MS SQL. Some feedback for your consideration, Damyon. When applying an extension to multiple students, it is quite a deliberate act to select students, select Grant extension, then to click Go. I don't see any point in confirming this action. On the extension page, the extension date should be enabled by default. I assume most teachers will want to set an extension date. I assume that if you don't enable the due date, the extension is indefinite. This isn't really intuitive. Perhaps a better mechanism could be considered, rather than enabling/disabling the date. Perhaps there could be an control for "Due by a specific date" or "Indefinite extension" and have the extension date dependent on that.
          Hide
          Damyon Wiese added a comment -

          Thanks Michael,

          We can definitely look at removing the "Go" button.

          Points 2 and 3 - the reason for the checkbox is not to enable an infinite extension - it is to allow you remove an extension once it is granted. It defaults to off because by default the student does not have an extension granted.

          Show
          Damyon Wiese added a comment - Thanks Michael, We can definitely look at removing the "Go" button. Points 2 and 3 - the reason for the checkbox is not to enable an infinite extension - it is to allow you remove an extension once it is granted. It defaults to off because by default the student does not have an extension granted.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work.

          These changes have been spread upstream and are already available in the git and cvs repositories.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao
          Hide
          Sam Hemelryk added a comment -

          Looks like this issue has caused a nasty regression guys, check out MDL-35337.

          Show
          Sam Hemelryk added a comment - Looks like this issue has caused a nasty regression guys, check out MDL-35337 .
          Hide
          Helen Foster added a comment -

          Attaching doc from Grette Wilkinson which may help as a starting point for documenting this new feature.

          Show
          Helen Foster added a comment - Attaching doc from Grette Wilkinson which may help as a starting point for documenting this new feature.
          Hide
          Mary Cooch added a comment -

          Removing the qa_test_required label as this issue now has QA tests: MDLQA-4587
          and MDLQA-4588 (Note that these are QA test master copies which will be cloned
          for upcoming QA cycles.)

          Show
          Mary Cooch added a comment - Removing the qa_test_required label as this issue now has QA tests: MDLQA-4587 and MDLQA-4588 (Note that these are QA test master copies which will be cloned for upcoming QA cycles.)
          Hide
          Mary Cooch added a comment -

          Removing the docs_required label as this is now documented here http://docs.moodle.org/24/en/Assignment_settings

          Show
          Mary Cooch added a comment - Removing the docs_required label as this is now documented here http://docs.moodle.org/24/en/Assignment_settings
          Hide
          Mark Glynn added a comment -

          Mary / Damyon,

          Just following on earlier point made by Stephen Bourget - is it possible to grant extensions to groups and not just individuals.

          Kind regards

          Mark

          Show
          Mark Glynn added a comment - Mary / Damyon, Just following on earlier point made by Stephen Bourget - is it possible to grant extensions to groups and not just individuals. Kind regards Mark

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: