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
    • Rank:
      39007

      Description

      Add plagiarism api hooks to files attached to forum posts

        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: