Moodle
  1. Moodle
  2. MDL-35710

mod_assign: assessable_content_done event name is not precise

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.4
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide

      The plariarism API is not currently testable so this minimal test will suffice.

      1. Create an assignment with "Require students click submit button" set to true and "Online Text" submissions enabled
      2. Login as a student and add some text to your submission
      3. "Submit" the assignment
      4. Verify there are no errors reported on the page
      Show
      The plariarism API is not currently testable so this minimal test will suffice. Create an assignment with "Require students click submit button" set to true and "Online Text" submissions enabled Login as a student and add some text to your submission "Submit" the assignment Verify there are no errors reported on the page
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
    • Rank:
      44454

      Description

      The event 'assessable_content_done' triggered on assignment submission (mod_assign)

      • does not have a clear name that would hint what it does
      • Inconsistent with naming of content and file upload events (assessable_content_uploaded, assessable_file_uploaded events). assessable_content_done is called on any type of assignment submission (content, file and any other found in submission sub-directory), not for the "content" submission type only.

      Therefore it should be renamed to something clear and independent of submission type.

        Issue Links

          Activity

          Hide
          Ruslan Kabalin added a comment -

          Added Daymon, Kanika and Dan to watch list, as they were those who discussed assignment events in MDL-34544.

          Show
          Ruslan Kabalin added a comment - Added Daymon, Kanika and Dan to watch list, as they were those who discussed assignment events in MDL-34544 .
          Hide
          Dan Marsden added a comment -

          +1 from me - I like Ruslan's "assessable_submitted" better and now is the best time to do this - much harder after 2.4 release. (btw "assesseble_file_submitted" was MD's preference back in Moodle 2.0 - we changed to content as it was no longer just handling files in 2.3 - but assessable_submitted seems better to me. - thanks Ruslan

          Show
          Dan Marsden added a comment - +1 from me - I like Ruslan's "assessable_submitted" better and now is the best time to do this - much harder after 2.4 release. (btw "assesseble_file_submitted" was MD's preference back in Moodle 2.0 - we changed to content as it was no longer just handling files in 2.3 - but assessable_submitted seems better to me. - thanks Ruslan
          Hide
          Dan Marsden added a comment -

          in fact.. - taking away my +1 - I think this needs some more thought.

          the point of this event is a "submit for marking" - so IMO the word "Done" is appropriate. - it's not just submitted, it's done as in finalized, can't submit anymore etc....

          but - I agree that's it's not as clear as it should be...

          Show
          Dan Marsden added a comment - in fact.. - taking away my +1 - I think this needs some more thought. the point of this event is a "submit for marking" - so IMO the word "Done" is appropriate. - it's not just submitted, it's done as in finalized, can't submit anymore etc.... but - I agree that's it's not as clear as it should be...
          Hide
          Ruslan Kabalin added a comment -

          ) Is there a case when assignment can be submitted not for marking? What if teacher release it to drafts and student resubmit the work again? I guess it should still be "submitted" when record in mdl_assign_submission is created or updated, irrespective to whether submit button present, or submission occurs on uploading.

          Show
          Ruslan Kabalin added a comment - ) Is there a case when assignment can be submitted not for marking? What if teacher release it to drafts and student resubmit the work again? I guess it should still be "submitted" when record in mdl_assign_submission is created or updated, irrespective to whether submit button present, or submission occurs on uploading.
          Hide
          Dan Marsden added a comment -

          Hi Ruslan,

          the existing event does what we need it for. - if you want to add another event to work with for your purposes that's fine as long as we can differentiate between this particular event and any others.

          I need to have some way of checking if an event has been triggered because a student hit the "submit for marking button" - doesn't matter when - if teacher releases to drafts and student hits submit for marking again - I need that same event to be triggered.

          Show
          Dan Marsden added a comment - Hi Ruslan, the existing event does what we need it for. - if you want to add another event to work with for your purposes that's fine as long as we can differentiate between this particular event and any others. I need to have some way of checking if an event has been triggered because a student hit the "submit for marking button" - doesn't matter when - if teacher releases to drafts and student hits submit for marking again - I need that same event to be triggered.
          Hide
          Damyon Wiese added a comment -

          I like the name "assessable_submitted" and I would like to see it triggered in both cases:

          1. "Require students click submit button" is enabled, student clicks submit button
          2. "Require students click submit button" is disabled, student adds/updates their submission

          I would also like to see another parameter for this event to distinguish between these to cases "submission_editable" true/false (false in first case, true in second)

          Show
          Damyon Wiese added a comment - I like the name "assessable_submitted" and I would like to see it triggered in both cases: "Require students click submit button" is enabled, student clicks submit button "Require students click submit button" is disabled, student adds/updates their submission I would also like to see another parameter for this event to distinguish between these to cases "submission_editable" true/false (false in first case, true in second)
          Hide
          Dan Marsden added a comment -

          I like Damyon's proposal - unsure about the name "submission_editable" though - isn't it possible under the 2nd case for the event to be triggered but the students aren't able to update their submissions - so the submission isn't really "editable" ? - or have I got that wrong?

          Show
          Dan Marsden added a comment - I like Damyon's proposal - unsure about the name "submission_editable" though - isn't it possible under the 2nd case for the event to be triggered but the students aren't able to update their submissions - so the submission isn't really "editable" ? - or have I got that wrong?
          Hide
          Damyon Wiese added a comment -

          That is only possible if someone changes something else - (a teacher can manually lock a submission from the grading table) - but that would happen after this event. Same with due date / cut off date - ie - once the due date/cut off date is reached the submission would no longer be editable - but that would happen some time after this event fires. At the time that this event fires - the submission would still be editable by a student.

          Happy to hear suggestions for a different name though.

          Show
          Damyon Wiese added a comment - That is only possible if someone changes something else - (a teacher can manually lock a submission from the grading table) - but that would happen after this event. Same with due date / cut off date - ie - once the due date/cut off date is reached the submission would no longer be editable - but that would happen some time after this event fires. At the time that this event fires - the submission would still be editable by a student. Happy to hear suggestions for a different name though.
          Hide
          Dan Marsden added a comment -

          can't think of a better name - only ones I thought of were "submission_done", "submission_final" or something similar but I'm not sure that they are better/worse.
          anyway - +1 from me - I'm not worried about the naming - as long as we can differentiate between the events.

          Thanks Damyon/Ruslan

          Show
          Dan Marsden added a comment - can't think of a better name - only ones I thought of were "submission_done", "submission_final" or something similar but I'm not sure that they are better/worse. anyway - +1 from me - I'm not worried about the naming - as long as we can differentiate between the events. Thanks Damyon/Ruslan
          Hide
          Dan Marsden added a comment -

          in fact - it could be really useful if the same extra param was added to the other events "assessable_file_uploaded" and "assessable_file_uploaded - I think the plan was to deprec assessable_file_uploaded and replace it with "assessable_content_uploaded - as we no longer just deal with "files" - haven't got round to tidying that up yet.

          Show
          Dan Marsden added a comment - in fact - it could be really useful if the same extra param was added to the other events "assessable_file_uploaded" and "assessable_file_uploaded - I think the plan was to deprec assessable_file_uploaded and replace it with "assessable_content_uploaded - as we no longer just deal with "files" - haven't got round to tidying that up yet.
          Hide
          Ruslan Kabalin added a comment -

          Based on the discussion above, I suggest using assessable_submitted for both cases, and adding extra boolean parameter to event data object:

          diff --git a/mod/assign/db/events.php b/mod/assign/db/events.php
          index 26a3022..f73f84d 100644
          --- a/mod/assign/db/events.php
          +++ b/mod/assign/db/events.php
          @@ -45,6 +45,7 @@ assessable_file_uploaded
           
           assessable_submitted
               ->modulename     = 'assign';
          +    ->confirmed      = // whether submission has been confirmed by student (submit button has been pressed)
               ->cmid           = // The cmid of the assign.
               ->itemid         = // The submission id of the user submission.
               ->courseid       = // The course id of the course the assign belongs to.
          

          In the case if submit button pressing is not required in the assignment settings, $eventdata->confirmed will be set to "false".

          Show
          Ruslan Kabalin added a comment - Based on the discussion above, I suggest using assessable_submitted for both cases, and adding extra boolean parameter to event data object: diff --git a/mod/assign/db/events.php b/mod/assign/db/events.php index 26a3022..f73f84d 100644 --- a/mod/assign/db/events.php +++ b/mod/assign/db/events.php @@ -45,6 +45,7 @@ assessable_file_uploaded assessable_submitted ->modulename = 'assign'; + ->confirmed = // whether submission has been confirmed by student (submit button has been pressed) ->cmid = // The cmid of the assign. ->itemid = // The submission id of the user submission. ->courseid = // The course id of the course the assign belongs to. In the case if submit button pressing is not required in the assignment settings, $eventdata->confirmed will be set to "false".
          Hide
          Dan Marsden added a comment -

          I'm not sure if confirmed is better than "submission_editable" - Damyon should make the call on this as the assign maintainer. - but I'd shorten the comment for that var to this:
          // Whether the submit button has been pressed. - remove the "submission has been confirmed..." stuff

          but - totally up to Damyon, it doesn't worry me either way.

          Show
          Dan Marsden added a comment - I'm not sure if confirmed is better than "submission_editable" - Damyon should make the call on this as the assign maintainer. - but I'd shorten the comment for that var to this: // Whether the submit button has been pressed. - remove the "submission has been confirmed..." stuff but - totally up to Damyon, it doesn't worry me either way.
          Hide
          Dan Marsden added a comment -

          btw - MDL-35197 is the bug we had for replacing file_submitted with content_submitted event names.

          Show
          Dan Marsden added a comment - btw - MDL-35197 is the bug we had for replacing file_submitted with content_submitted event names.
          Hide
          Ruslan Kabalin added a comment -

          submission_editable instead of confirmed is fine with me. Daymon, if this is OK I will update the patch.

          Show
          Ruslan Kabalin added a comment - submission_editable instead of confirmed is fine with me. Daymon, if this is OK I will update the patch.
          Hide
          Ruslan Kabalin added a comment - - edited

          In fact, I would elaborate it even further. Add 'params' variable to the event object and keep module-specific parameters there. This will allow to keep assessable_submitted event object structure consistent throughout the system, while particular modules could define own parameters within params variable.

          diff --git a/mod/assign/db/events.php b/mod/assign/db/events.php
          index 26a3022..24fb2cf 100644
          --- a/mod/assign/db/events.php
          +++ b/mod/assign/db/events.php
          @@ -49,4 +49,6 @@ assessable_submitted
               ->itemid         = // The submission id of the user submission.
               ->courseid       = // The course id of the course the assign belongs to.
               ->userid         = // The user id that the attempt belongs to.
          +    ->params         = // Array of module specific parameters
          +        -> submission_editable = // Whether user can edit submission before assessment has been done.
           */
          diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php
          index 6521a26..7a8a281 100644
          --- a/mod/assign/locallib.php
          +++ b/mod/assign/locallib.php
          @@ -3279,6 +3279,9 @@ class assign {
                           $eventdata->itemid       = $submission->id;
                           $eventdata->courseid     = $this->get_course()->id;
                           $eventdata->userid       = $USER->id;
          +                $eventdata->params       = array(
          +                    'submission_editable' => false,
          +                );
                           events_trigger('assessable_submitted', $eventdata);
                       }
                   }
          
          Show
          Ruslan Kabalin added a comment - - edited In fact, I would elaborate it even further. Add 'params' variable to the event object and keep module-specific parameters there. This will allow to keep assessable_submitted event object structure consistent throughout the system, while particular modules could define own parameters within params variable. diff --git a/mod/assign/db/events.php b/mod/assign/db/events.php index 26a3022..24fb2cf 100644 --- a/mod/assign/db/events.php +++ b/mod/assign/db/events.php @@ -49,4 +49,6 @@ assessable_submitted ->itemid = // The submission id of the user submission. ->courseid = // The course id of the course the assign belongs to. ->userid = // The user id that the attempt belongs to. + ->params = // Array of module specific parameters + -> submission_editable = // Whether user can edit submission before assessment has been done. */ diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 6521a26..7a8a281 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -3279,6 +3279,9 @@ class assign { $eventdata->itemid = $submission->id; $eventdata->courseid = $ this ->get_course()->id; $eventdata->userid = $USER->id; + $eventdata->params = array( + 'submission_editable' => false , + ); events_trigger('assessable_submitted', $eventdata); } }
          Hide
          Damyon Wiese added a comment -

          Hi Ruslan and Dan,

          I think these proposals are all about equal so I'm happy with the last patch (with the params ) from Ruslan above. If you can update the branch Ruslan this will be fine with me.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Ruslan and Dan, I think these proposals are all about equal so I'm happy with the last patch (with the params ) from Ruslan above. If you can update the branch Ruslan this will be fine with me. Regards, Damyon
          Hide
          Ruslan Kabalin added a comment -

          Thanks Daymon, I have added second patch that introduces parameter.

          Show
          Ruslan Kabalin added a comment - Thanks Daymon, I have added second patch that introduces parameter.
          Hide
          Damyon Wiese added a comment -

          Thanks Ruslan. I just added a minor patch to fix the comments.

          Show
          Damyon Wiese added a comment - Thanks Ruslan. I just added a minor patch to fix the comments.
          Hide
          Damyon Wiese added a comment -

          This should only be changed for 2.4 as it is not a good idea to change APIs for stable releases.

          Show
          Damyon Wiese added a comment - This should only be changed for 2.4 as it is not a good idea to change APIs for stable releases.
          Hide
          Dan Poltawski added a comment -

          Integrated to master. I also added a commit to bump the version number to take new events. Thanks.

          Show
          Dan Poltawski added a comment - Integrated to master. I also added a commit to bump the version number to take new events. Thanks.
          Hide
          Ruslan Kabalin added a comment -

          Thanks Dan, indeed the version bump was missing, thanks.

          Show
          Ruslan Kabalin added a comment - Thanks Dan, indeed the version bump was missing, thanks.
          Hide
          Ankit Agarwal added a comment -

          Works as expected.
          Thanks!

          Show
          Ankit Agarwal added a comment - Works as expected. Thanks!
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: