Moodle
  1. Moodle
  2. MDL-22549 Assignment important issues
  3. MDL-22609

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

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Assignment (2.2)
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      35977

      Description

      Create one 1-file upload assignment.
      As student upload one file.

      Seems ok, but the "itemid" in the files table is the submission->userid. It should be the submission->id instead.

      1. 20100630-MDL-22609.patch
        6 kB
        Aparup Banerjee
      2. 20100701-MDL-22609.patch
        12 kB
        Aparup Banerjee
      3. MDL-22609.patch
        2 kB
        Aparup Banerjee

        Issue Links

          Activity

          Hide
          Davo Smith added a comment -

          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?

          Show
          Davo Smith added a comment - 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?
          Hide
          Aparup Banerjee added a comment -

          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.

          Show
          Aparup Banerjee added a comment - 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.
          Hide
          Martin Dougiamas added a comment -

          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.

          Show
          Martin Dougiamas added a comment - 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.
          Hide
          Aparup Banerjee added a comment -

          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.

          Show
          Aparup Banerjee added a comment - 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.
          Hide
          Aparup Banerjee added a comment -

          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)

          Show
          Aparup Banerjee added a comment - 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)
          Hide
          Aparup Banerjee added a comment -

          patch created for commit into HEAD.

          Show
          Aparup Banerjee added a comment - patch created for commit into HEAD.
          Hide
          Petr Škoda added a comment -

          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?

          Show
          Petr Škoda added a comment - 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?
          Hide
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - oh, the directory creation there is completely bogus, reverting everything and changing just the userid-->submission id
          Hide
          Petr Škoda added a comment -

          reclosing

          Show
          Petr Škoda added a comment - reclosing
          Hide
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - oh, the userid-->submissionid was not changed everywhere, reopening once more and fixing
          Hide
          Petr Škoda added a comment -

          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

          Show
          Petr Škoda added a comment - 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
          Hide
          Petr Škoda added a comment -

          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...

          Show
          Petr Škoda added a comment - 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...
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - hmm if the response is supposed to be related to an empty submission then can't we create an empty submission at that time?
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - oh just noticed the deleting code was reverted so its broken now. will verify the submission id this time.
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - MDL-22893 will affect the management of files here too... linking
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - MDL-22893 fixed. using the filemanager and filepicker lements now to manager files. please test and update/resolve this bug as necessary.
          Hide
          Davo Smith added a comment -

          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.

          Show
          Davo Smith added a comment - 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.
          Hide
          Aparup Banerjee added a comment -

          Thank you Davo, thats been fixed in head.

          Show
          Aparup Banerjee added a comment - Thank you Davo, thats been fixed in head.
          Hide
          Davo Smith added a comment -

          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).

          Show
          Davo Smith added a comment - 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).
          Hide
          Aparup Banerjee added a comment -

          thanks, its fixed in head.

          Show
          Aparup Banerjee added a comment - thanks, its fixed in head.
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - the itemid change from userid to submission id has been done in head.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: