Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: Assignment, Plagiarism
    • Labels:
    • Testing Instructions:
      Hide

      To test this you should set up a new assignment that uses the drafts feature
      login as a student
      add some files to the assignment
      use the button to "finalise" the assignment and flag the assignment as finished. (not sure what this button looks like in new assignment)
      Make sure no errors occur when finalizing the assignment.

      No core or contrib plugins currently handle this new event - we'll add it when it's added to core.

      Show
      To test this you should set up a new assignment that uses the drafts feature login as a student add some files to the assignment use the button to "finalise" the assignment and flag the assignment as finished. (not sure what this button looks like in new assignment) Make sure no errors occur when finalizing the assignment. No core or contrib plugins currently handle this new event - we'll add it when it's added to core.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      master_MDL-34544
    • Rank:
      42973

      Description

      The old mod/assignment had a setting to allow users to upload draft files and then "finalize" or submit the file for marking when finished.
      The plagiarism plugins checked if "var4" existed in the form and added a setting to control whether to send each draft file to the plagiarism plugin or to only send on the finalized event - it used a different event trigger for this "assessable_files_done"

      We need to port this functionality to the new mod/assign - checking to see if the drafts feature is enabled and enable/disable a drop list to allow configuration of when to send the file. and then we need to be able to check to see if the user has submitted a final time or if the file is still in draft form before sending to the plugin (it doesn't need to happen using a different event type - just whatever looks most effficient.)

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          Hi Kanika - this would be a good one to get stuck into when you've finished the work around MDL-34382

          Show
          Dan Marsden added a comment - Hi Kanika - this would be a good one to get stuck into when you've finished the work around MDL-34382
          Hide
          Kanika Goyal added a comment -

          Hi Dan,

          I have created the event "assessable_content_done" for this. Let me know if its fine.
          Here is the patch - https://github.com/kanikagoyal/moodle/compare/master_MDL-34544_add_support_for_control_of_drafts_to_new_assign

          Thanks,
          Kanika

          Show
          Kanika Goyal added a comment - Hi Dan, I have created the event "assessable_content_done" for this. Let me know if its fine. Here is the patch - https://github.com/kanikagoyal/moodle/compare/master_MDL-34544_add_support_for_control_of_drafts_to_new_assign Thanks, Kanika
          Hide
          Dan Marsden added a comment -

          Thanks Kanika - looks good to me, adding Damyon for his +1 before we submit for integration.

          Damyon - this replaces the "assessable_files_done" event that is triggered in mod/assignment/type/upload

          Show
          Dan Marsden added a comment - Thanks Kanika - looks good to me, adding Damyon for his +1 before we submit for integration. Damyon - this replaces the "assessable_files_done" event that is triggered in mod/assignment/type/upload
          Hide
          Damyon Wiese added a comment -

          The change itself looks OK - but needs adding in one more place. This needs to go wherever "notify_student_submission_receipt" gets called.

          I'm not 100% clear on how you are handling draft submissions (or if you want to) either.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - The change itself looks OK - but needs adding in one more place. This needs to go wherever "notify_student_submission_receipt" gets called. I'm not 100% clear on how you are handling draft submissions (or if you want to) either. Regards, Damyon
          Hide
          Kanika Goyal added a comment -

          Hi Damyon,

          Thanks for your feedback.
          We need to call this event when user clicks on 'Submit Assignment' button, which calls the action 'submit'.
          "process_submit_for_grading" is called whenever, student clicks on it. I am not sure, but why we need to add it at 2 places (the other one is in "process_save_submission")? This wont trigger the event two times?
          Please elaborate if possible on this more.

          Thanks,
          Kanika

          Show
          Kanika Goyal added a comment - Hi Damyon, Thanks for your feedback. We need to call this event when user clicks on 'Submit Assignment' button, which calls the action 'submit'. "process_submit_for_grading" is called whenever, student clicks on it. I am not sure, but why we need to add it at 2 places (the other one is in "process_save_submission")? This wont trigger the event two times? Please elaborate if possible on this more. Thanks, Kanika
          Hide
          Damyon Wiese added a comment -

          Hi Kanika,

          If "Require users click submit" is not enabled, every time the student edits their submission it is considered as submitted. That is what the second case is for.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Kanika, If "Require users click submit" is not enabled, every time the student edits their submission it is considered as submitted. That is what the second case is for. Regards, Damyon
          Hide
          Dan Marsden added a comment -

          Hi Damyon - that case is already handled by the plagiarism plugin using the existing events afaik - if you would like us to add the extra event for other plugins/systems to use that's fine, but afaik we don't it for the plagiarism plugins.

          Show
          Dan Marsden added a comment - Hi Damyon - that case is already handled by the plagiarism plugin using the existing events afaik - if you would like us to add the extra event for other plugins/systems to use that's fine, but afaik we don't it for the plagiarism plugins.
          Hide
          Damyon Wiese added a comment -

          Hi Dan,

          I don't understand how the 2 cases are different from a plagiarism point of view. Both are cases when a student is submitting their version of work that is ready for grading.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Dan, I don't understand how the 2 cases are different from a plagiarism point of view. Both are cases when a student is submitting their version of work that is ready for grading. Regards, Damyon
          Hide
          Dan Marsden added a comment -

          mod_assign assign_submission_file->save triggers the event 'assessable_file_uploaded' - plagiarism plugins handle those events.

          but... when drafts mode is enabled we don't want to use that event handler - we want to know when a user has files previously uploaded and has hit the submit for marking button.

          make sense?

          Show
          Dan Marsden added a comment - mod_assign assign_submission_file->save triggers the event 'assessable_file_uploaded' - plagiarism plugins handle those events. but... when drafts mode is enabled we don't want to use that event handler - we want to know when a user has files previously uploaded and has hit the submit for marking button. make sense?
          Hide
          Dan Marsden added a comment -

          Kanika's patch is the equivalent to the "assessable_files_done" event that is triggered in the old assignment in upload/assignment.class.php

          Show
          Dan Marsden added a comment - Kanika's patch is the equivalent to the "assessable_files_done" event that is triggered in the old assignment in upload/assignment.class.php
          Hide
          Damyon Wiese added a comment -

          So shouldn't this patch also include not calling assessable_files_uploaded in the submission_file plugin when "require students click submit" is enabled?

          And then how are you going to get the list of files after this sequence:

          1. Student uploads 3 files - has not clicked "submit"
          2. No asessable_files_uploaded event is triggered because "require students click submit" is enabled
          3. Student clicks submit - assessable_content done is called - but that contains no information about the files that were uploaded.

          I would have thought that you would either require the "assessable_files_uploaded" event to include the information that this is a draft, or assessable_content would need to include the list of all files in the submission.

          I am just trying to make sure you are not assuming anything about the submission plugins or file areas used.

          • Damyon
          Show
          Damyon Wiese added a comment - So shouldn't this patch also include not calling assessable_files_uploaded in the submission_file plugin when "require students click submit" is enabled? And then how are you going to get the list of files after this sequence: Student uploads 3 files - has not clicked "submit" No asessable_files_uploaded event is triggered because "require students click submit" is enabled Student clicks submit - assessable_content done is called - but that contains no information about the files that were uploaded. I would have thought that you would either require the "assessable_files_uploaded" event to include the information that this is a draft, or assessable_content would need to include the list of all files in the submission. I am just trying to make sure you are not assuming anything about the submission plugins or file areas used. Damyon
          Hide
          Kanika Goyal added a comment -

          Hi Damyon,

          How we are handling things are-

          • If "require students click submit" is enabled
            • the admin/teacher can configure to send files when file is uploaded/or when student clicks submit button
              • If admin selects send files to plugin when file is uploaded
                • then file_uploaded works normally (on assessable_files_uploaded/content_uploaded)
              • else if it sends only when student clicks submit
                • then if student uploads file (and has not submitted yet)
                  assessable_files_uploaded will be called but plugin will not do anything in this case and will just return
                • else if student submits
                  assessable_files_done/content_done is called then plugin will extract the files/data for that assignment from database (this is hardcoded into plugin)
          • Else
            • assessable_files_uploaded/content_uploaded is called everytime student modifies assignment.

          As your main questions was when assessable_content_done is called, but we are not sending any information this is because, we are extracting the information in plugin only.

          Thanks,
          Kanika

          Show
          Kanika Goyal added a comment - Hi Damyon, How we are handling things are- If "require students click submit" is enabled the admin/teacher can configure to send files when file is uploaded/or when student clicks submit button If admin selects send files to plugin when file is uploaded then file_uploaded works normally (on assessable_files_uploaded/content_uploaded) else if it sends only when student clicks submit then if student uploads file (and has not submitted yet) assessable_files_uploaded will be called but plugin will not do anything in this case and will just return else if student submits assessable_files_done/content_done is called then plugin will extract the files/data for that assignment from database (this is hardcoded into plugin) Else assessable_files_uploaded/content_uploaded is called everytime student modifies assignment. As your main questions was when assessable_content_done is called, but we are not sending any information this is because, we are extracting the information in plugin only. Thanks, Kanika
          Hide
          Damyon Wiese added a comment -

          Thanks Kanika for the explanation.

          I think that there are 2 problems with this solution -

          "If "require students click submit" is enabled"

          • How is this determined ? It should not be looking directly at the mod_assign table as that prevents third party plugins (or even other core plugins) from supporting the plagiarism API.

          "then plugin will extract the files/data for that assignment from database (this is hardcoded into plugin)"

          • Again - this prevents third party plugins from supporting the plagiarism API

          The functions in the plagiarism API should contain everything the plagiarism plugin needs to know about the activity and make 0 assumptions about the table structure, file areas etc supported by the plugin. ie: If there was one extra parameter to the assessable_file_uploaded event ("draft"), the plagiarism plugin could either process draft files immediately or wait until the "assessable_files_done" event based on it's own settings. It would then be able to work for any third party assignment submission plugin or any activity that adds plagiarism support.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks Kanika for the explanation. I think that there are 2 problems with this solution - "If "require students click submit" is enabled" How is this determined ? It should not be looking directly at the mod_assign table as that prevents third party plugins (or even other core plugins) from supporting the plagiarism API. "then plugin will extract the files/data for that assignment from database (this is hardcoded into plugin)" Again - this prevents third party plugins from supporting the plagiarism API The functions in the plagiarism API should contain everything the plagiarism plugin needs to know about the activity and make 0 assumptions about the table structure, file areas etc supported by the plugin. ie: If there was one extra parameter to the assessable_file_uploaded event ("draft"), the plagiarism plugin could either process draft files immediately or wait until the "assessable_files_done" event based on it's own settings. It would then be able to work for any third party assignment submission plugin or any activity that adds plagiarism support. Regards, Damyon
          Hide
          Kanika Goyal added a comment - - edited

          Hi Damyon,

          What you said makes sense, right now we are looking for variables like if "var4" existed in the form, and then gives control to the admin to configure.
          We should send the information to the plugin, and not let plugin itself extract the information.

          @Dan please comment on this, and I will do the changes accordingly. This can be taken as future development

          Thanks,
          Kanika

          Show
          Kanika Goyal added a comment - - edited Hi Damyon, What you said makes sense, right now we are looking for variables like if "var4" existed in the form, and then gives control to the admin to configure. We should send the information to the plugin, and not let plugin itself extract the information. @Dan please comment on this, and I will do the changes accordingly. This can be taken as future development Thanks, Kanika
          Hide
          Dan Marsden added a comment -

          Thanks Kanika,

          I'm quite confused by this; the plagiarism plugins aren't up for review here - what is up for review is that we need an event to be triggered when a student hits the submit for marking button.

          I'm sure there are other improvements we can make to the existing plagiarism code and the way it handles the work(I like the idea of adding a "draft" param to the existing assessable_files_uploaded event) but we still need this new event as implemented by Kanika. The existing assessable_files_upload event IMO should be triggered on each upload - not handled on a condition basis. It's up to whatever plugin manages the event to handle it as required - these events aren't specific to plagiarism, they are designed to be generic (there are already a few other non plagiarism plugins using the same events.) In any case - changes to these existing events should be managed on a separate tracker issue.

          Damyon - if you want us to add further info to the new event from the patch above or shift it to a different location that's fine, can you please clarify if you are objecting to the patch Kanika has proposed here or if your objections are just related to how the plagiarism plugins implement the other events? - we still need this event even if we improve the way the plugins manage the other events.

          The only module that has a "drafts" function is currently the Mod/assign so plagiarism plugins that want to implement some form of handling around this will have to hard code specific handling for mod_assign. I'm sure we can improve this by adding further info to the existing events but that can happen on a separate tracker issue if required.

          This also implements the same style of event as already in the 2.2 assignment - adding this event makes it easier for existing code to be ported to work as it already does in the Moodle 2.2 assignment. The GSOC project is now "pencils down" - so although Kanika might hang around in the community post GSOC and it's also possible I might do some work to make the plugins more generic we are going to need this event any way.

          Also - The Turnitin plugin will be replaced by a new version from Turnitin relatively soon - I'm not all that keen spending any time reworking existing code that is about to be deprecated - but I don't think I should have to, just to get a simple event trigger into core.

          thanks,

          Show
          Dan Marsden added a comment - Thanks Kanika, I'm quite confused by this; the plagiarism plugins aren't up for review here - what is up for review is that we need an event to be triggered when a student hits the submit for marking button. I'm sure there are other improvements we can make to the existing plagiarism code and the way it handles the work(I like the idea of adding a "draft" param to the existing assessable_files_uploaded event) but we still need this new event as implemented by Kanika. The existing assessable_files_upload event IMO should be triggered on each upload - not handled on a condition basis. It's up to whatever plugin manages the event to handle it as required - these events aren't specific to plagiarism, they are designed to be generic (there are already a few other non plagiarism plugins using the same events.) In any case - changes to these existing events should be managed on a separate tracker issue. Damyon - if you want us to add further info to the new event from the patch above or shift it to a different location that's fine, can you please clarify if you are objecting to the patch Kanika has proposed here or if your objections are just related to how the plagiarism plugins implement the other events? - we still need this event even if we improve the way the plugins manage the other events. The only module that has a "drafts" function is currently the Mod/assign so plagiarism plugins that want to implement some form of handling around this will have to hard code specific handling for mod_assign. I'm sure we can improve this by adding further info to the existing events but that can happen on a separate tracker issue if required. This also implements the same style of event as already in the 2.2 assignment - adding this event makes it easier for existing code to be ported to work as it already does in the Moodle 2.2 assignment. The GSOC project is now "pencils down" - so although Kanika might hang around in the community post GSOC and it's also possible I might do some work to make the plugins more generic we are going to need this event any way. Also - The Turnitin plugin will be replaced by a new version from Turnitin relatively soon - I'm not all that keen spending any time reworking existing code that is about to be deprecated - but I don't think I should have to, just to get a simple event trigger into core. thanks,
          Hide
          Damyon Wiese added a comment -

          Hmm - the patch itself is fine. I don't agree with the functionality - but I'll let the integrator make a call on this.

          Right now - the events triggered are

          Case 1. "Require students click submit" is enabled

          • Plagiarism plugin will receive events:
            • assessable_files_uploaded/content_uploaded
            • assessable_files_done/content_done

          Case 2. "Require students click submit" is disabled

          • Plagiarism plugin will receive events:
            • assessable_files_uploaded/content_uploaded

          The plagiarism plugin has no way of knowing if it will ever receive "assessable_files_done/content_done" without peeking at the DB tables for the specific activity.

          Show
          Damyon Wiese added a comment - Hmm - the patch itself is fine. I don't agree with the functionality - but I'll let the integrator make a call on this. Right now - the events triggered are Case 1. "Require students click submit" is enabled Plagiarism plugin will receive events: assessable_files_uploaded/content_uploaded assessable_files_done/content_done Case 2. "Require students click submit" is disabled Plagiarism plugin will receive events: assessable_files_uploaded/content_uploaded The plagiarism plugin has no way of knowing if it will ever receive "assessable_files_done/content_done" without peeking at the DB tables for the specific activity.
          Hide
          Dan Marsden added a comment -

          yes - thanks Damyon - that's the way it operates in < 2.3 anyway.

          in future it would definitely be nice to add an extra param to the assessable_files_uploaded event that specifies the files are in draft mode but that can come later.

          Show
          Dan Marsden added a comment - yes - thanks Damyon - that's the way it operates in < 2.3 anyway. in future it would definitely be nice to add an extra param to the assessable_files_uploaded event that specifies the files are in draft mode but that can come later.
          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
          Eloy Lafuente (stronk7) added a comment -

          Uhm, sorry, I'm not integrating this right now:

          • Looking for events in the code and comments here it seems there is some confusion about "file" or "files". In assignment it's assessable_file_uploaded (singular) but assessable_files_done (plural). I assign there is only assessable_file_uploaded (cannot find any "done" for files, and the patch does not add it at all, is it missing?
          • Those events should be documented somewhere. Not sure if there are any docs about them or which db/events.php files(s) (core or modules) should contain information about them.

          Please clarify those 2 points and I'll be happy to integrate this (although clearly the situation is far from perfect, reading which events are sent on each assign combination.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, sorry, I'm not integrating this right now: Looking for events in the code and comments here it seems there is some confusion about "file" or "files". In assignment it's assessable_file_uploaded (singular) but assessable_files_done (plural). I assign there is only assessable_file_uploaded (cannot find any "done" for files, and the patch does not add it at all, is it missing? Those events should be documented somewhere. Not sure if there are any docs about them or which db/events.php files(s) (core or modules) should contain information about them. Please clarify those 2 points and I'll be happy to integrate this (although clearly the situation is far from perfect, reading which events are sent on each assign combination. 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
          Dan Marsden added a comment -

          thanks Eloy - we used assessable_content_ for all new events as the event doesn't just handle files uploaded - users can add content to the assignment using the online text style stuff so we decided assessable_file/s wasn't the right name for the events.

          older deprecated code (mod/assignment/) uses assessable_files and assessable_file inconsistently but it's now deprecated code so I guess we don't need to fix it there?)

          assessable_file_uploaded is still called once in mod/assign/submission/file as this was copied by Netspot from the old assignment - we could deprecate the event name in master but I'd guess we can't change this in stable... - maybe we should track that on a separate tracker issue?

          I'll add documentation in db/events.php - I hadn't realised we were documenting events there until quite recently - makes sense!

          Show
          Dan Marsden added a comment - thanks Eloy - we used assessable_content_ for all new events as the event doesn't just handle files uploaded - users can add content to the assignment using the online text style stuff so we decided assessable_file/s wasn't the right name for the events. older deprecated code (mod/assignment/) uses assessable_files and assessable_file inconsistently but it's now deprecated code so I guess we don't need to fix it there?) assessable_file_uploaded is still called once in mod/assign/submission/file as this was copied by Netspot from the old assignment - we could deprecate the event name in master but I'd guess we can't change this in stable... - maybe we should track that on a separate tracker issue? I'll add documentation in db/events.php - I hadn't realised we were documenting events there until quite recently - makes sense!
          Hide
          Dan Marsden added a comment -

          Hi Eloy,

          have re-submitted for integration with updated documentation and have created 2 new tracker issues (linked) to fix documentation in other plugins and one to deprecate the file_uploaded event still in mod/assign - let me know if you need anything else for this particular bug to pass.

          Show
          Dan Marsden added a comment - Hi Eloy, have re-submitted for integration with updated documentation and have created 2 new tracker issues (linked) to fix documentation in other plugins and one to deprecate the file_uploaded event still in mod/assign - let me know if you need anything else for this particular bug to pass.
          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
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
          Hide
          Rajesh Taneja added a comment -

          Works Great,

          Thanks for fixing this Dan.

          Show
          Rajesh Taneja added a comment - Works Great, Thanks for fixing this Dan.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: