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 workshop - 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 a workshop - 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-32227_add_plagiarism_api_support_to_workshop
    • Rank:
      39005

      Description

      Add plagiarism support to Workshop module

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

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

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

          Hi guys,
          The patch is spot on. Just two small things to consider:-

          1. is if (!empty($content)) check needed?
          2. if ($CFG->enableplagiarism) should be if (!empty($CFG->enableplagiarism)) unless it is assured that it is always set

          Thanks

          Show
          Ankit Agarwal added a comment - Hi guys, The patch is spot on. Just two small things to consider:- is if (!empty($content)) check needed? if ($CFG->enableplagiarism) should be if (!empty($CFG->enableplagiarism)) unless it is assured that it is always set Thanks
          Hide
          Dan Marsden added a comment -

          Thanks Ankit - good spotting, Kanika has made those changes too.

          Show
          Dan Marsden added a comment - Thanks Ankit - good spotting, Kanika has made those changes too.
          Hide
          Ankit Agarwal added a comment -

          This Looks good.
          +1 for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - This Looks good. +1 for integration. Thanks
          Hide
          Dan Marsden added a comment -

          Adding David here - would quite like his +1 as maintainer of workshop before we submit for integration.

          Show
          Dan Marsden added a comment - Adding David here - would quite like his +1 as maintainer of workshop before we submit for integration.
          Hide
          David Mudrak added a comment -

          Hi Dan. Thanks for letting me know. The only thing that caught my eye was https://github.com/kanikagoyal/moodle/compare/master_MDL-32227_add_plagiarism_api_support_to_workshop#L1R145

          Why do you pass the submission id via a parameter called 'workshop' to plagiarism_get_links()? (and then again at #L1R788) Is that some common approach in the plagiarism API? I admit I did not even try to look into some plagiarism plugin code to see how they work with the passed parameters. It's just that this naming sounds a bit nonintuitive to me.

          Show
          David Mudrak added a comment - Hi Dan. Thanks for letting me know. The only thing that caught my eye was https://github.com/kanikagoyal/moodle/compare/master_MDL-32227_add_plagiarism_api_support_to_workshop#L1R145 Why do you pass the submission id via a parameter called 'workshop' to plagiarism_get_links()? (and then again at #L1R788) Is that some common approach in the plagiarism API? I admit I did not even try to look into some plagiarism plugin code to see how they work with the passed parameters. It's just that this naming sounds a bit nonintuitive to me.
          Hide
          Dan Marsden added a comment -

          heh - good spotting! - I don't use that param in my existing plugins so missed that!
          in the Assignment patches we pass assignment->id into the param 'assignment' but in this patch it's passing the submissionid - Kanika I haven't checked but is this param used in your new code? - if it is can you please change 'workshop' to 'submissionid'
          We might need to review this params usage across the other patches too (and the other plagiarism plugins written by others)

          thanks David!

          Show
          Dan Marsden added a comment - heh - good spotting! - I don't use that param in my existing plugins so missed that! in the Assignment patches we pass assignment->id into the param 'assignment' but in this patch it's passing the submissionid - Kanika I haven't checked but is this param used in your new code? - if it is can you please change 'workshop' to 'submissionid' We might need to review this params usage across the other patches too (and the other plagiarism plugins written by others) thanks David!
          Hide
          Dan Marsden added a comment -

          Hi Kanika - just realised you wren't a watcher on this bug so might not have seen the above comments - can you please take a look and check to make sure you're watching all the plagiarism bugs? and take a look at the comments here? - thanks!

          Show
          Dan Marsden added a comment - Hi Kanika - just realised you wren't a watcher on this bug so might not have seen the above comments - can you please take a look and check to make sure you're watching all the plagiarism bugs? and take a look at the comments here? - thanks!
          Hide
          Kanika Goyal added a comment -

          Hi Dan,
          I somehow missed to watch this issue
          I have changed the 'workshop' to 'submissionid'. But this param is actually not getting used
          in the plagiarism plugin code. Should I remove it? or it might be required in future(same as in assignment)?

          Thanks,
          Kanika

          Show
          Kanika Goyal added a comment - Hi Dan, I somehow missed to watch this issue I have changed the 'workshop' to 'submissionid'. But this param is actually not getting used in the plagiarism plugin code. Should I remove it? or it might be required in future(same as in assignment)? Thanks, Kanika
          Hide
          Dan Marsden added a comment -

          Lets make it consistent with the other modules and keep the 'workshop' name but set it to $workshop->id instead of the existing submissionid

          David - does that make sense? - or would it be better to use 'workshopid' ?

          thanks,

          Show
          Dan Marsden added a comment - Lets make it consistent with the other modules and keep the 'workshop' name but set it to $workshop->id instead of the existing submissionid David - does that make sense? - or would it be better to use 'workshopid' ? thanks,
          Hide
          Ankit Agarwal added a comment -

          We are using "forum" in the forums, so I guess it should be "workshop" here to keep it consistent?
          Thanks

          Show
          Ankit Agarwal added a comment - We are using "forum" in the forums, so I guess it should be "workshop" here to keep it consistent? Thanks
          Hide
          David Mudrak added a comment -

          Well, as said above, I don't know much about plagiarism API. So I have no idea what these params are supposed to be used for. If they were to identify the content being checked, I would personally prefer the standard combination of the component (mod_forum, mod_assign, ...), instanceid ($forum->id, $workshop->id), contentarea (post, submission) and itemid ($post->id, $submission->id). Such a "holy four" has been successfully used in various parts of Moodle, being robust and clear.

          Anyway, as long as you pass $workshop->id, I am ok with either workshop or workshopid. And Ankit's comment is valid, of course.

          Show
          David Mudrak added a comment - Well, as said above, I don't know much about plagiarism API. So I have no idea what these params are supposed to be used for. If they were to identify the content being checked, I would personally prefer the standard combination of the component (mod_forum, mod_assign, ...), instanceid ($forum->id, $workshop->id), contentarea (post, submission) and itemid ($post->id, $submission->id). Such a "holy four" has been successfully used in various parts of Moodle, being robust and clear. Anyway, as long as you pass $workshop->id, I am ok with either workshop or workshopid. And Ankit's comment is valid, of course.
          Hide
          Dan Marsden added a comment -

          Thanks David, We use the cmid as a key in the plagiarism plugins a lot so we wouldn't use the others but we could add them in future if other plugin types wanted to use the events and needed that info.

          Kanika can you please back to 'workshop' => workshop->id and we'll push this one up for integration.

          Show
          Dan Marsden added a comment - Thanks David, We use the cmid as a key in the plagiarism plugins a lot so we wouldn't use the others but we could add them in future if other plugin types wanted to use the events and needed that info. Kanika can you please back to 'workshop' => workshop->id and we'll push this one up for integration.
          Hide
          Kanika Goyal added a comment -

          I wanted to do 'workshop' => $workshop->id only, but $workshop is not defined in mod/workshop/renderer.php.
          Is there any way to make it work?

          Thanks,
          Kanika

          Show
          Kanika Goyal added a comment - I wanted to do 'workshop' => $workshop->id only, but $workshop is not defined in mod/workshop/renderer.php. Is there any way to make it work? Thanks, Kanika
          Hide
          David Mudrak added a comment -

          Correct, the render_workshop_submission() does not have access to the workshop->id currently because there was no need for it so far. So if you really need it for this purpose, you will have to modify locallib.php and add the support for this field into workshop_submission class.

          Show
          David Mudrak added a comment - Correct, the render_workshop_submission() does not have access to the workshop->id currently because there was no need for it so far. So if you really need it for this purpose, you will have to modify locallib.php and add the support for this field into workshop_submission class.
          Hide
          Dan Marsden added a comment -

          hehe - things are never simple! we don't need it, so lets just remove it completely - thanks David/Kanika.

          Show
          Dan Marsden added a comment - hehe - things are never simple! we don't need it, so lets just remove it completely - thanks David/Kanika.
          Hide
          Kanika Goyal added a comment -

          Hi,

          I have removed 'workshop'. Is this is correct?

          Thanks,
          Kanika

          Show
          Kanika Goyal added a comment - Hi, I have removed 'workshop'. Is this is correct? Thanks, Kanika
          Hide
          Dan Marsden added a comment -

          looks good - passing up for integration.

          Show
          Dan Marsden added a comment - looks good - passing up for integration.
          Hide
          Aparup Banerjee 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
          Aparup Banerjee 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
          Dan Poltawski added a comment -

          Integrated, thanks Kanika!

          Show
          Dan Poltawski added a comment - Integrated, thanks Kanika!
          Hide
          Rajesh Taneja added a comment -

          Thanks Kanika and everyone involved.

          No errors found while playing with workshop activity.

          Show
          Rajesh Taneja added a comment - Thanks Kanika and everyone involved. No errors found while playing with workshop activity.
          Hide
          Dan Poltawski added a comment -

          asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ!

          Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

          Show
          Dan Poltawski added a comment - asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ! Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: