Details

    • Testing Instructions:
      Hide

      No Plagiarism plugins exist in core so it is sufficient to test a user submission to ensure that no errors occur.

      • Create an Old assignment using online assignment type. - make sure no errors occur on editing page.
      • login as a student and submit some content - make sure no errors occur.
      • Login as teacher and view submissions page - make sure no errors occur.
      Show
      No Plagiarism plugins exist in core so it is sufficient to test a user submission to ensure that no errors occur. Create an Old assignment using online assignment type. - make sure no errors occur on editing page. login as a student and submit some content - make sure no errors occur. Login as teacher and view submissions page - make sure no errors occur.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      master_MDL-32228_add_plagiarism_api_support_to_online_assignment_type
    • Rank:
      39006

      Description

      Add the plagiarism api hooks to the online assignment type.

        Activity

        Hide
        Dan Marsden added a comment -

        would be good for someone else to peer review Kanika's work here too..

        Ideally I'd like to get this into 2.2Stable and master if poss although I expect mod/assignment will be removed from 2.4 or 2.5 sometime soon.

        thanks!

        Show
        Dan Marsden added a comment - would be good for someone else to peer review Kanika's work here too.. Ideally I'd like to get this into 2.2Stable and master if poss although I expect mod/assignment will be removed from 2.4 or 2.5 sometime soon. thanks!
        Hide
        Ankit Agarwal added a comment - - edited

        Hi Kanika and Dan,
        The patch looks great. Here are a few suggestions to fix/discuss:-

        1. $course_context should be $coursecontext. I don't think we allow _ in variable names.
        2. 'userid'=>$USER->id should be 'userid' => $USER->id (Mark the spaces) - This needs to be updated for all similar instances.
        3. should we be not doing the event trigger for other assignment types?
        4. will it be better to use html writer class for outputing the plagiarism_get_links() ?
        5. shouldn't we trigger the hook when the content is actually saved (after $this->update_submission()) instead of doing it later after the redirect to the same page? Say user edited his submission and is redirected to view.php?Saved=1 . Now the user reloads the page, it will trigger the event hook again although, there is no submission made.

        Thanks

        Show
        Ankit Agarwal added a comment - - edited Hi Kanika and Dan, The patch looks great. Here are a few suggestions to fix/discuss:- $course_context should be $coursecontext. I don't think we allow _ in variable names. 'userid'=>$USER->id should be 'userid' => $USER->id (Mark the spaces) - This needs to be updated for all similar instances. should we be not doing the event trigger for other assignment types? will it be better to use html writer class for outputing the plagiarism_get_links() ? shouldn't we trigger the hook when the content is actually saved (after $this->update_submission()) instead of doing it later after the redirect to the same page? Say user edited his submission and is redirected to view.php?Saved=1 . Now the user reloads the page, it will trigger the event hook again although, there is no submission made. Thanks
        Hide
        Dan Marsden added a comment -

        Thanks for the feedback Ankit - the other assignment types already have this event trigger for the plagiarism api, it's the online assignment type that doesn't have it yet.
        using html_writer to output plagiarism_get_links sounds like an ok idea but we should probably tackle that separately and if we decide to use it, update all existing calls to plagiarism_get_links in core code.

        I hadn't looked closely at where the event was added yet - good spotting! - Kanika can you please move the event trigger just after update_submission is called? - thanks!

        Show
        Dan Marsden added a comment - Thanks for the feedback Ankit - the other assignment types already have this event trigger for the plagiarism api, it's the online assignment type that doesn't have it yet. using html_writer to output plagiarism_get_links sounds like an ok idea but we should probably tackle that separately and if we decide to use it, update all existing calls to plagiarism_get_links in core code. I hadn't looked closely at where the event was added yet - good spotting! - Kanika can you please move the event trigger just after update_submission is called? - thanks!
        Hide
        Kanika Goyal added a comment -

        Hi Dan and Ankit,
        Thanks for your feedback and suggestions. I have changed the code as per the suggestions and updated the link too.
        Please have a look again and let me know if further changes are required.

        Thanks,
        Kanika

        Show
        Kanika Goyal added a comment - Hi Dan and Ankit, Thanks for your feedback and suggestions. I have changed the code as per the suggestions and updated the link too. Please have a look again and let me know if further changes are required. Thanks, Kanika
        Hide
        Ankit Agarwal added a comment -

        Hi Kanika,
        Looks good to me. Just as Dan said, make sure these lines are not too lengthy. You might want to install and use the code checker plugins. Helps a lot with coding guidelines. https://github.com/moodlehq/moodle-local_codechecker

        Thanks

        Show
        Ankit Agarwal added a comment - Hi Kanika, Looks good to me. Just as Dan said, make sure these lines are not too lengthy. You might want to install and use the code checker plugins. Helps a lot with coding guidelines. https://github.com/moodlehq/moodle-local_codechecker Thanks
        Hide
        Dan Marsden added a comment -

        great! - pushing this one up for integration.
        Note to integrator: MASTER only please.

        Show
        Dan Marsden added a comment - great! - pushing this one up for integration. Note to integrator: MASTER only please.
        Hide
        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
        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
        Sam Hemelryk added a comment -

        Thanks guys, this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks guys, this has been integrated now
        Hide
        Tim Barker added a comment -

        I don' think that this has been integrated yet as the commit message doesn't appear in git log.

        Show
        Tim Barker added a comment - I don' think that this has been integrated yet as the commit message doesn't appear in git log.
        Hide
        Dan Marsden added a comment -

        looks like it's there to me - I see Sams merge branch commit and Kanika's patch later - this page lists it:
        http://git.moodle.org/gw?p=integration.git;a=shortlog

        just search for '32228' and you should see both

        Show
        Dan Marsden added a comment - looks like it's there to me - I see Sams merge branch commit and Kanika's patch later - this page lists it: http://git.moodle.org/gw?p=integration.git;a=shortlog just search for '32228' and you should see both
        Hide
        Sam Hemelryk added a comment -

        I've just checked master as well.

        git log --oneline --grep='MDL-32228' ./
        cbbdc7d Merge branch 'master_MDL-32228_add_plagiarism_api_support_to_online_assignment_type' of git://github.com/kanikagoyal/moodle
        93d52d8 MDL-32228: Plagiarism API - add support for plagiarism api to online assignment type

        Looks like its definitely there Tim.

        Show
        Sam Hemelryk added a comment - I've just checked master as well. git log --oneline --grep=' MDL-32228 ' ./ cbbdc7d Merge branch 'master_ MDL-32228 _add_plagiarism_api_support_to_online_assignment_type' of git://github.com/kanikagoyal/moodle 93d52d8 MDL-32228 : Plagiarism API - add support for plagiarism api to online assignment type Looks like its definitely there Tim.
        Hide
        Tim Barker added a comment -

        I've just tried git log | grep MDL-32228 again this morning and it is indeed there. I did get Dan P to look for it yesterday also and he couldn't find it either. Sam will you reset this so I can test it?

        Show
        Tim Barker added a comment - I've just tried git log | grep MDL-32228 again this morning and it is indeed there. I did get Dan P to look for it yesterday also and he couldn't find it either. Sam will you reset this so I can test it?
        Hide
        Sam Hemelryk added a comment -

        Reset to testing

        Show
        Sam Hemelryk added a comment - Reset to testing
        Hide
        Dan Poltawski added a comment -

        Sorry, we must've been grepping the wrong thing! :S

        Show
        Dan Poltawski added a comment - Sorry, we must've been grepping the wrong thing! :S
        Hide
        Tim Barker added a comment -

        Nope, I just checked my command history via cli using the up arrow and we were definitely greping the right thing.

        Show
        Tim Barker added a comment - Nope, I just checked my command history via cli using the up arrow and we were definitely greping the right thing.
        Hide
        Dan Poltawski added a comment -

        *Notice*: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26

        Congratulations

        {tracker.user.name}

        !

        You've made into Moodle

        {tracker.fixversion-1}

        +

        I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world.

        cheers!

        {tracker.friendlyintegrator}
        Show
        Dan Poltawski added a comment - * Notice *: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26 Congratulations {tracker.user.name} ! You've made into Moodle {tracker.fixversion-1} + I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world. cheers! {tracker.friendlyintegrator}

          People

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

            Dates

            • Created:
              Updated:
              Resolved: