Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39760

mod_assign: Add missing submission plugins callbacks.

    Details

    • Testing Instructions:
      Hide

      Make sure that file and online text submissions (when user submit assignment) work as expected in the cases:

      1. User does not require to press submit button (in assignment settings)
      2. User require to press submit button (in assignment settings)
      3. Teacher locks submission
      4. Teacher unlocks submission
      5. In a test case 2, teacher releases submission to draft
      Show
      Make sure that file and online text submissions (when user submit assignment) work as expected in the cases: User does not require to press submit button (in assignment settings) User require to press submit button (in assignment settings) Teacher locks submission Teacher unlocks submission In a test case 2, teacher releases submission to draft
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-39760-master

      Description

      Currently there are only two callback methods related to assignment submission (mod_assign) student/teacher actions - "save" and "submit_for_grading". It would be good to add some more and improve existing:

      • submit_for_grading could be more useful with $submission parameter (in pre-2.5 versions),
      • Add lock, unlock, release_to_draft callbacks.

      This could be useful for submission plugins that are interacting with external systems to reflect the submission status on the remote side (e.g. lock the submitted work when it is locked by teacher in Moodle)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            kabalin Ruslan Kabalin added a comment -

            In case you are curious, the change needed for Mahara assignment plugin I am upgrading to new assignment submission type to make it work with Moodle 2.3 onwards.

            Show
            kabalin Ruslan Kabalin added a comment - In case you are curious, the change needed for Mahara assignment plugin I am upgrading to new assignment submission type to make it work with Moodle 2.3 onwards.
            Hide
            damyon Damyon Wiese added a comment -

            Hi Ruslan,

            The code looks good - I have a question about the design though.

            Is it better to have lots of different hooks for the different stages, or is it better to have more generic, 'submission updated', 'grade updated', 'flags updated' hooks and pass in the submission, grades and flags with the updated state? That way we won't have to keep adding more and more hooks as we add new features.

            Let me know what you think of that approach.

            Cheers, Damyon

            Show
            damyon Damyon Wiese added a comment - Hi Ruslan, The code looks good - I have a question about the design though. Is it better to have lots of different hooks for the different stages, or is it better to have more generic, 'submission updated', 'grade updated', 'flags updated' hooks and pass in the submission, grades and flags with the updated state? That way we won't have to keep adding more and more hooks as we add new features. Let me know what you think of that approach. Cheers, Damyon
            Hide
            kabalin Ruslan Kabalin added a comment -

            Hello Damyon, I was also thinking about that. In my opinion having many hooks is better, manly because it is easier to follow the code and for backward compatibility. In case of the single hook "submission updated", you will need to pass an action as parameter (e.g. save, lock, unlock, release_to_draft), thus developer will need to check action using the "case" structure and likely end up with many functions for each action anyway if something more than 2-3 lines of code needs to be done for particular action. There is nothing wrong with adding new hooks when new features are added I think.

            Thanks for revision by the way.

            Show
            kabalin Ruslan Kabalin added a comment - Hello Damyon, I was also thinking about that. In my opinion having many hooks is better, manly because it is easier to follow the code and for backward compatibility. In case of the single hook "submission updated", you will need to pass an action as parameter (e.g. save, lock, unlock, release_to_draft), thus developer will need to check action using the "case" structure and likely end up with many functions for each action anyway if something more than 2-3 lines of code needs to be done for particular action. There is nothing wrong with adding new hooks when new features are added I think. Thanks for revision by the way.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Damyon, have you reviewed it (I saw status change)? If so, shall I submit it for integration then?

            Show
            kabalin Ruslan Kabalin added a comment - Damyon, have you reviewed it (I saw status change)? If so, shall I submit it for integration then?
            Hide
            damyon Damyon Wiese added a comment -

            Hi Ruslan,

            No problem - your reasoning about separate functions makes sense and I am happy with that.

            One new issue for 25 and master is that, I have split the "locked" functionality out of the submissions table and into a "flags" table (Because a student may have multiple submissions attempts for a single assignment). I think for those branches you should pass the "flags" to the plugin and not the submission. The submission will just be the latest submission attempt by the student, but the "lock" action applies to all submissions from that user.

            Show
            damyon Damyon Wiese added a comment - Hi Ruslan, No problem - your reasoning about separate functions makes sense and I am happy with that. One new issue for 25 and master is that, I have split the "locked" functionality out of the submissions table and into a "flags" table (Because a student may have multiple submissions attempts for a single assignment). I think for those branches you should pass the "flags" to the plugin and not the submission. The submission will just be the latest submission attempt by the student, but the "lock" action applies to all submissions from that user.
            Hide
            kabalin Ruslan Kabalin added a comment -

            I think I am interested in the last one anyway, because in the plugin I need to get submission details to determine which remote item I need to lock (I do not need to lock items in the earlier submission attempts, as they will not be evaluated).

            Show
            kabalin Ruslan Kabalin added a comment - I think I am interested in the last one anyway, because in the plugin I need to get submission details to determine which remote item I need to lock (I do not need to lock items in the earlier submission attempts, as they will not be evaluated).
            Hide
            damyon Damyon Wiese added a comment -

            OK - the flags doesn't tell you more than the fact that "lock" has been called anyway and the submission can be useful. Also your current version has the same API for all version which is easier for plugin authors.

            I am convinced - sending for integration (thanks!).

            Note for integrators - backporting: This is 100% safe (new functions added - default empty implementation for existing plugins). IMO this is missing functionality and is required for the Mahara plugin. Feel free to ping me about it.

            Show
            damyon Damyon Wiese added a comment - OK - the flags doesn't tell you more than the fact that "lock" has been called anyway and the submission can be useful. Also your current version has the same API for all version which is easier for plugin authors. I am convinced - sending for integration (thanks!). Note for integrators - backporting: This is 100% safe (new functions added - default empty implementation for existing plugins). IMO this is missing functionality and is required for the Mahara plugin. Feel free to ping me about it.
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks Ruslan, this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Ruslan, this has been integrated now
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for improving this Ruslan,

            Assignment works fine with different settings.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for improving this Ruslan, Assignment works fine with different settings.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Thanks for giving me joys and smiles
            Thanks for sharing my trouble's pile

            Thanks for wipeing the tears of my eye
            Thanks for showing me the glad view of sky

            Thanks for lending me your shoulders to lean
            Thanks for giving my words a proper mean

            Thanks for telling me the value of life
            Thanks for showing me the rules to survive

            Thanks for lending me the sympathetic ears
            Thanks for showing how much you care

            From all this what I mean in the end
            Is thanks for being my special friend.

            – Seema Chowdhury

            Sent upstream so... closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Thanks for giving me joys and smiles Thanks for sharing my trouble's pile Thanks for wipeing the tears of my eye Thanks for showing me the glad view of sky Thanks for lending me your shoulders to lean Thanks for giving my words a proper mean Thanks for telling me the value of life Thanks for showing me the rules to survive Thanks for lending me the sympathetic ears Thanks for showing how much you care From all this what I mean in the end Is thanks for being my special friend. – Seema Chowdhury Sent upstream so... closing, thanks!
            Hide
            davosmith Davo Smith added a comment -

            Just a note that changing APIs in stable versions isn't 100% safe. This introduced a strict standards warning which broke my pdf annotation plugin for Moodle 2.4 users with debugging on.

            This is probably because I was the only person using the 'submit_for_grading' function (as I suggested it in the first place and fixed it to add the submission param in 2.5, but without specifying 'stdClass' in front of the parameters).

            This note's just here as a reminder to be careful in the future ...

            Show
            davosmith Davo Smith added a comment - Just a note that changing APIs in stable versions isn't 100% safe. This introduced a strict standards warning which broke my pdf annotation plugin for Moodle 2.4 users with debugging on. This is probably because I was the only person using the 'submit_for_grading' function (as I suggested it in the first place and fixed it to add the submission param in 2.5, but without specifying 'stdClass' in front of the parameters). This note's just here as a reminder to be careful in the future ...

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  8/Jul/13