danmarsden / moodle-plagiarism_urkund

Ouriginal plagiarism plugin for Moodle (previously called Urkund)
https://ouriginal.com/
12 stars 19 forks source link

Submitting on behalf of a student doesn't work (still/again) #71

Closed thepurpleblob closed 8 years ago

thepurpleblob commented 8 years ago

Create a completely new Assignment, enabling Urkund but otherwise standard settings

Go to the Assignment submissions screen and pick an enrolled student. Click the edit dropdown and submit a file on their behalf.

It does not get processed by Urkund.

Interestingly, once this has been done the student can't re-submit it either. There is no entry in the plagiarism-urkund-files table.

danmarsden commented 8 years ago

sounds like something weird/conflicting in the events handling - I'll try to track it down and fix it in the next release.

thepurpleblob commented 8 years ago

Just to note that (although not 100% proven) we have anecdotal evidence that this does work for some users, some of the time. I think there may be some subtlety in the exact settings and circumstances.

danmarsden commented 8 years ago

cool - I can see the draft-submission setting (if enabled) interfering with this. I'm going to spend some time looking at some of this today.

danmarsden commented 8 years ago

looks like Moodle core only triggers the assessable submitted event when draft submission is not enabled - I don't think that is correct - it means that the setting "When should the file be submitted" => "Submit the file when first uploaded" will not work.

danmarsden commented 8 years ago

no - that's correct, it still triggers the assessable_uploaded event which is right... I can't reproduce any issues around this yet. works for me when submitting on behalf

danmarsden commented 8 years ago

I can see a possible issue with group submissions - do the issues with submitting on-behalf happen on assignments that use group-based submissions?

danmarsden commented 8 years ago

mentioned this in a previous commit that I was working that I thought would fix some potential issues but I had the detail wrong. (so reopening this as it was closed automatically incorrectly.)

When group submission is being used it uses the original submitter as the $userid related to the file submission - I thought about changing that to use an id from a group member but that isn't correct because when group submission is in use the file is "owned" by the original submitter (including when submitting on behalf as a teacher) - the fix was slightly different. I assume this bug I found isn't the same as the one you are having - I presume you are seeing issues with the report/score not showing to teachers/admins - but the issue I found was with the score not showing to students when team submission was enabled.

danmarsden commented 8 years ago

I think I've reproduce this - it's slightly related to #75 - working on the best way forward now.

thepurpleblob commented 8 years ago

These are not group submissions.

danmarsden commented 8 years ago

thanks - I think I have the final solution... it won't fix "existing" records with a problem but I should be able to write a SQL query to find problematic records.

danmarsden commented 8 years ago

part of the problem is that the core get_links() function called by Moodle doesn't pass the userid related to the actual submission - it passes the userid of the file "owner" which will be the admin/teacher that has uploaded the file. I looked at adding a "relateduserid" field to core but I'm going for a simpler option in the urkund code instead.

danmarsden commented 8 years ago

-basically we use the relateduserid when creating the record in the files table so that we can get the right submission id - then we change the userid in the plagiarism table to the userid of the file owner - and allow the get_links() function to show details of files not owned by the current user - which is needed for team submissions as well. The security (can the user see the file) happens much higher in the process so we don't need to verify if the user has access to the file at this point.

danmarsden commented 8 years ago

This ugly looking query (that could probably be optimised a lot) should return all the files in the plagiarism_urkund_files table that have an incorrect userid set in the urkund files table.

SELECT uf.id, uf.cm, uf.filename, uf.userid, f1.userid as userfileid FROM mdl_plagiarism_urkund_files uf
JOIN mdl_files f1 ON uf.identifier = f1.contenthash
JOIN mdl_context c1 ON c1.id = f1.contextid
AND component = 'assignsubmission_file'
AND c1.instanceid = uf.cm 
WHERE uf.id NOT IN (
SELECT p.id
FROM mdl_plagiarism_urkund_files p
JOIN mdl_files f ON p.identifier = f.contenthash
JOIN mdl_context c ON c.id = f.contextid
AND component = 'assignsubmission_file'
AND c.instanceid = p.cm 
AND p.userid = f.userid)
AND uf.statuscode <> 'pending'

I would not recommend a whole-sale update fixing the userid field in the urkund_files table with the userfileid value shown in that query but verifying each one and updating the userid field with the correct one shown in the userfileid but hopefully there aren't that many and you can go through each manually and verify in the interface that updating them makes the score appear correctly.

This will possibly only work in conjunction with commitid 76231db1e1e60a3a2f1e25038ea70fef4cd38b53

danmarsden commented 8 years ago

FYI - I've also downgraded the stability of the code to "ALPHA" as this could do with some extra testing if possible. I'm working later than usual today and I want to make sure I haven't missed anything before we update the plugins db!

danmarsden commented 8 years ago

..also might be useful to mention that the "cog" image won't show up immediately when a teacher/admin submits a file on-behalf - but it will show as soon as it has sent the file to urkund in the next cron run (scheduled to run every 5min)

thepurpleblob commented 8 years ago

Great thanks. I'll try that query when I get into the office :)

thepurpleblob commented 8 years ago

That took 15 minutes to run... and came up with 306 results. I'm not sure how concerned I should be. Oddly whole blocks seemed to be identical... Removing that block and discounting ones that look like us trying to break it there are about 70 that look like genuine submissions. Still a bit of worry.

danmarsden commented 8 years ago

haha - ugly looking query that doesn't even work..... :-) I'll try to write a better one!

danmarsden commented 8 years ago

ok - take 2?

this query returns all assignment submissions that have a record in the files table matching the user in the plagiarism record (all good/correct submissions)

SELECT uf.id
FROM mdl_plagiarism_urkund_files uf
JOIN mdl_course_modules cm on uf.cm = cm.id
JOIN mdl_modules m on m.id = cm.module
JOIN mdl_assign a ON a.id = cm.instance
JOIN mdl_assign_plugin_config ap ON ap.assignment = a.id
JOIN mdl_files f ON (f.contenthash = uf.identifier AND uf.userid = f.userid)
JOIN mdl_context c1 ON (c1.id = f.contextid AND c1.instanceid = cm.id)
WHERE m.name = 'assign' AND (ap.plugin = 'file' AND ap.subtype = 'assignsubmission' AND ap.name='enabled' and ap.value = '1')
AND f.filearea = 'submission_files'

And this query (hopefully) will return all assignment submission files that don't have a matching user.

SELECT DISTINCT uf.id, uf.cm, uf.filename, uf.userid, f.userid as userfileid
FROM mdl_plagiarism_urkund_files uf
JOIN mdl_course_modules cm on uf.cm = cm.id
JOIN mdl_modules m on m.id = cm.module
JOIN mdl_assign a ON a.id = cm.instance
JOIN mdl_assign_plugin_config ap ON ap.assignment = a.id
JOIN mdl_files f ON (f.contenthash = uf.identifier)
JOIN mdl_context c1 ON (c1.id = f.contextid AND c1.instanceid = cm.id)
WHERE m.name = 'assign' AND (ap.plugin = 'file' AND ap.subtype = 'assignsubmission' AND ap.name='enabled' and ap.value = '1')
AND uf.id NOT IN(
SELECT uf.id
FROM mdl_plagiarism_urkund_files uf
JOIN mdl_course_modules cm on uf.cm = cm.id
JOIN mdl_modules m on m.id = cm.module
JOIN mdl_assign a ON a.id = cm.instance
JOIN mdl_assign_plugin_config ap ON ap.assignment = a.id
JOIN mdl_files f ON (f.contenthash = uf.identifier AND uf.userid = f.userid)
JOIN mdl_context c1 ON (c1.id = f.contextid AND c1.instanceid = cm.id)
WHERE m.name = 'assign' AND (ap.plugin = 'file' AND ap.subtype = 'assignsubmission' AND ap.name='enabled' and ap.value = '1')
AND f.filearea = 'submission_files'
)

If there are multiple records for the same id then there might be multiple users that have uploaded the file with the same contenthash other than the user associated with this record - hopefully it generates less records than the previous query though.

thepurpleblob commented 8 years ago

The latter query took the same time and returned a very similar looking 311 submissions. Over 100 of them appear to be the same submission. The only thing that differs is the 'userfileid' field - not sure what when on there but it's a long-dead Assignment with submissions in groups so I'm not bothered about that. Many of the others will be old too.

We've updated the live site to the latest master (after successful testing) so we're happy to see how we get on I think.

danmarsden commented 8 years ago

cool - probably need to exclude "group-based" submissions from that query as well then - I'm sure I'll hear from you if you spot any other issues!

thepurpleblob commented 8 years ago

I think we'll just see how it goes. Nothing horrible appears to have happened (yet) over the weekend and submissions seem to be going through this morning.

I was surprised how many staff seem to have been using Assignment's "edit" feature to submit/update on behalf of students. This place can be a bit peculiar though ;-)