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 a forum. - make sure no errors occur on editing page.
      • login as a student and submit some posts with content and files - make sure no errors occur.
      • Login as teacher and view forum - 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 a forum. - make sure no errors occur on editing page. login as a student and submit some posts with content and files - make sure no errors occur. Login as teacher and view forum - make sure no errors occur.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      master_MDL-32229_add_plagiarism_api_support_to_forum

      Description

      Add plagiarism api hooks to files attached to forum posts

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Marsden added a comment -

            bouncing this up for peer review - would be good to get someone elses eyes on this too.

            Show
            Dan Marsden added a comment - bouncing this up for peer review - would be good to get someone elses eyes on this too.
            Hide
            Ankit Agarwal added a comment - - edited

            Hi Dan and Kanika,
            The patch looks great. Here are a few small things to consider:-

            1. get_context_instance is deprecated. context_module::instance should be used instead.
            2. PHP doc for the class forum_trigger_content_uploaded_event is missing @return.
            3. I guess it will be better to use empty instead of if ($CFG->enableplagiarism) unless we are sure it is always set
            4. How does this patch deals with file alias?

            Just a note for discussion:-
            Shouldn't we have different triggers for upload and edit?

            Thanks

            Show
            Ankit Agarwal added a comment - - edited Hi Dan and Kanika, The patch looks great. Here are a few small things to consider:- get_context_instance is deprecated. context_module::instance should be used instead. PHP doc for the class forum_trigger_content_uploaded_event is missing @return. I guess it will be better to use empty instead of if ($CFG->enableplagiarism) unless we are sure it is always set How does this patch deals with file alias? Just a note for discussion:- Shouldn't we have different triggers for upload and edit? Thanks
            Hide
            Kanika Goyal added a comment -

            Hi Ankit,

            Thanks for your suggestions, I have modified the code accordingly.
            The event "assessable_content_uploaded" contains both message as well as file.

            We are triggering the event whenever a post is added/updated. On triggering the event "assessable_content_uploaded", plagiarism plugin will check if that data has already been submitted, if yes then it will not send the file for detection. What is your idea behind different triggers?

            Thanks,
            Kanika

            Show
            Kanika Goyal added a comment - Hi Ankit, Thanks for your suggestions, I have modified the code accordingly. The event "assessable_content_uploaded" contains both message as well as file. We are triggering the event whenever a post is added/updated. On triggering the event "assessable_content_uploaded", plagiarism plugin will check if that data has already been submitted, if yes then it will not send the file for detection. What is your idea behind different triggers? Thanks, Kanika
            Hide
            Dan Marsden added a comment -

            thanks Ankit/Kanika,

            Ankit is suggesting that there should probably be a way to distinguish between an upload and an edit event - we may not need this for plagiarism plugins but it makes sense to future proof for other plugins that might use these events.
            maybe we could do this by adding an extra param to forum_trigger_content_uploaded_event that sets $eventdata->name for each type of event triggered - we could use the function names they come from eg "forum_add_discussion", "forum_add_new_post" etc.

            make sense?

            Show
            Dan Marsden added a comment - thanks Ankit/Kanika, Ankit is suggesting that there should probably be a way to distinguish between an upload and an edit event - we may not need this for plagiarism plugins but it makes sense to future proof for other plugins that might use these events. maybe we could do this by adding an extra param to forum_trigger_content_uploaded_event that sets $eventdata->name for each type of event triggered - we could use the function names they come from eg "forum_add_discussion", "forum_add_new_post" etc. make sense?
            Hide
            Ankit Agarwal added a comment -

            Thanks Dan for explaining that. Yeah it makes sense to use function names. Also if we decide to do this here, it needs to be done with assign and assignments as well IMHO.
            By file aliases I meant, in 2.3+ you can use shortcut to a actual file, without actually uploading it. Say I have a file in private files area, I can simply create a shortcut and use that as forum attachment in a forum post. Am not sure how the current patch is treating(or is supposed to treat) such cases.

            Show
            Ankit Agarwal added a comment - Thanks Dan for explaining that. Yeah it makes sense to use function names. Also if we decide to do this here, it needs to be done with assign and assignments as well IMHO. By file aliases I meant, in 2.3+ you can use shortcut to a actual file, without actually uploading it. Say I have a file in private files area, I can simply create a shortcut and use that as forum attachment in a forum post. Am not sure how the current patch is treating(or is supposed to treat) such cases.
            Hide
            Dan Marsden added a comment -

            Hi Ankit, afaik the use of aliases is fine - get_area_files returns valid pathnamehashes which are used to access the files by the plagiarism plugins themselves.

            Kanika - can you please add an $eventdata->name to the other "new" events being added - once these have been integrated we will add a patch to add ->name to existing assessable events to keep them consistent.

            Show
            Dan Marsden added a comment - Hi Ankit, afaik the use of aliases is fine - get_area_files returns valid pathnamehashes which are used to access the files by the plagiarism plugins themselves. Kanika - can you please add an $eventdata->name to the other "new" events being added - once these have been integrated we will add a patch to add ->name to existing assessable events to keep them consistent.
            Hide
            Kanika Goyal added a comment -

            Hi Dan,

            I have added $eventdata->name by setting it as function name from where it is called in forum posts, but what should I set in other modules like in online assignment where event is called from a function named "update_submission".

            Thanks,
            Kanika

            Show
            Kanika Goyal added a comment - Hi Dan, I have added $eventdata->name by setting it as function name from where it is called in forum posts, but what should I set in other modules like in online assignment where event is called from a function named "update_submission". Thanks, Kanika
            Hide
            Dan Marsden added a comment -

            What are your thoughts on this patch ankit? - Kanika will email you about the other functions.

            Show
            Dan Marsden added a comment - What are your thoughts on this patch ankit? - Kanika will email you about the other functions.
            Hide
            Ankit Agarwal added a comment -

            Looks good to me.
            +1 for integration.
            Thanks

            Show
            Ankit Agarwal added a comment - Looks good to me. +1 for integration. Thanks
            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
            Rajesh Taneja added a comment -

            Thanks for fixing this Kanika,

            Was able to create forum, post to it with attachments and view it as another user.

            Show
            Rajesh Taneja added a comment - Thanks for fixing this Kanika, Was able to create forum, post to it with attachments and view it as another user.
            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: