Add plagiarism api support to Workshop module

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

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

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

This Looks good.
+1 for integration.
Thanks

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

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.

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!

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!

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

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,

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

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.

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.

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

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.

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

Hi,

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

Thanks,
Kanika

looks good - passing up for integration.

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

Integrated, thanks Kanika!

Thanks Kanika and everyone involved.

No errors found while playing with workshop activity.

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!