PROBLEM G - wrong id used in db in file upload assignment.

I came across this bug whilst updating my assignment type uploadpdf to Moodle 2.0 (once again, using the built-in 'advanced uploading of files' assignment type as my model).

I'm trying to understand why using the 'user id' rather than the 'submission id' is incorrect, so that I can make sure I get it right in my plugin.

As far as I can tell, given the context id, either the 'user id' or the 'submission id' would uniquely identify the file storage area. Additionally, the submission can be retrieved by either id. What difference does this choice make?

i'm not sure why userid was used instead of the submission id.
i think..
It seems it was a little easier to use userid. Using submission id would seem to me to be more logically separating the file submissions. using userid allows for a clash when looking up the unique file submissions.. however i'm not sure if the clash will actually happen as lookups are giving back sufficiently unique responses for now.

anyway i've created a patch that uses submission id instead. i wonder about regression issues with already existing file submission though. existing file submissions being referenced with userid may somehow need to be updated to use the submission id in mdl_files.

Submission is better because it's the "real" item in this module. It's possible an assignment type could allow multiple submissions per user, for example.

this patch addresses the itemid for files being submitted in assignment submissions.

it also modifies the upgrade code to move moodle 1.9 assignment submissions files to moodle 2.0 with refereence to submissionid instead of userid as the itemid.

i have however come upon the situation that a submission being graded/responded/fedback to where the feedback file is uploaded to the course_content file area. that file link does break after upgrading to 2.0 and trying to view the feedback file.

patch now addresses this bug's issue in single upload as well as advanced (multi) upload assignment types. (as well as migrating the old 1.9 files)

patch created for commit into HEAD.

reopening
1/ we should not crate dirs in assignment upgrade if they are not there - we can just "continue" with next submission
2/ what is the point of random changing of names of variables that contain the path?

oh, the directory creation there is completely bogus, reverting everything and changing just the userid-->submission id

reclosing

oh, the userid-->submissionid was not changed everywhere, reopening once more and fixing

found a security issue in deleting of submission files in upload type, it allowed to delete files from any submission due to the fact that the newly introduced submissionid parameter was not verified at all

oh, the submission record may not exist when we want to upload the response files, so it would make better sense to keep using the userid there, this will require a lot more work...

hmm if the response is supposed to be related to an empty submission then can't we create an empty submission at that time?

oh just noticed the deleting code was reverted so its broken now. will verify the submission id this time.

MDL-22893 will affect the management of files here too... linking

MDL-22893 fixed.
using the filemanager and filepicker lements now to manager files.
please test and update/resolve this bug as necessary.

Following on from this change, there seem to be a number of calls to $this->count_user_files(), in mod/assignment/type/upload/assignment.class.php which pass in the parameter '$USER->id', rather than '$submission->id'. This means that this regularly gives the wrong answer (potentially allowing more/less files than the assignment limit, stopping the 'submit for marking' button from appearing, showing incorrect data in the navigation block).

The function 'user_complete' (in mod/assignment/lib.php) is also broken, as it counts files based on the user id, rather than the submission id.

Thank you Davo, thats been fixed in head.

Hopefully the last one - 'view_upload_form' calls 'can_upload_file', which calls 'count_user_files' on '$submission->id', but nowhere in this chain is there a check to see if '$submission' is null, which results in a debugging error message at the point when a student views an assignment for which they have not submitted any work yet.

Fix - change the line 904 of mod/assignment/type/upload/assignment.class.php to:

and (empty($submission) or $this->count_user_files($submission->id) < $this->assignment->var1) // file limit not reached

(adding the 'empty' check at the start of the line).

thanks, its fixed in head.

the itemid change from userid to submission id has been done in head.