Moodle
  1. Moodle
  2. MDL-30144

"Download all as a zip" contains submissions from unenroled students

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Prerequisite

      1. Assignment 2.2 should be enabled.
      2. Go to Site administration ► Plugins ► Activity modules ► Manage activities and enable Assignment 2.2

      Test

      1. Create course with assignment 2.2 (online text/upload a single file)
      2. Enrol 3 students (S1, S2, S3)
      3. As S1 submit an assignment
      4. As S2 submit an assignment
      5. As S3 submit an assignment
      6. As teacher go to assignment and 'download all as zip' assignments.
      7. You should only see all assignment (from s1, s2 & s3) in zip file.
      8. unenrol S1 from the course
      9. As teacher go to assignment and 'download all' assignments.
      10. You should only see two assignment (from s2 & s3) in zip file.
      Show
      Prerequisite Assignment 2.2 should be enabled. Go to Site administration ► Plugins ► Activity modules ► Manage activities and enable Assignment 2.2 Test Create course with assignment 2.2 (online text/upload a single file) Enrol 3 students (S1, S2, S3) As S1 submit an assignment As S2 submit an assignment As S3 submit an assignment As teacher go to assignment and 'download all as zip' assignments. You should only see all assignment (from s1, s2 & s3) in zip file. unenrol S1 from the course As teacher go to assignment and 'download all' assignments. You should only see two assignment (from s2 & s3) in zip file.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      wip-mdl-30144
    • Rank:
      24836

      Description

      While unenrolled students do not appear on the assignment grading list, their submissions are going to be added to the 'Download all' zip file. This is inconsistent behaviour and probably should be resolved. Either all submitted assignments should be displayed, whether students are enrolled or not, OR assignments from unenrolled students are not added to the 'Download all as a zip' archive.

      Replication steps:

      1. a student enrols to a course
      2. the student submits an assignment
      3. the student unenrols from the course
      4. lecturer uses 'download all' option - assignment of an unenrolled student is there even though student is not on the list.
      1. MDL-30144_MOODLE_21_STABLE.patch
        2 kB
        Dimuthu Upeksha
      2. MDL-35533.patch
        1 kB
        Michael Hughes

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          I've put it on our backlog and we'll try to get to it as soon as we can.

          If you are able to provide a patch, please add a patch label so we will spot it.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog and we'll try to get to it as soon as we can. If you are able to provide a patch, please add a patch label so we will spot it.
          Hide
          Dimuthu Upeksha added a comment -

          Problem is in moodle/mod/assignment/lib.php file.
          There is a function called "assignment_get_all_submissions" that returns all assignment submissions form assignment_submissions table without looking at the user_enrolments table.
          This causes to download the files of unenrolled people also. Here is the patch which can handle this problem. Any comments???

          Show
          Dimuthu Upeksha added a comment - Problem is in moodle/mod/assignment/lib.php file. There is a function called "assignment_get_all_submissions" that returns all assignment submissions form assignment_submissions table without looking at the user_enrolments table. This causes to download the files of unenrolled people also. Here is the patch which can handle this problem. Any comments???
          Hide
          Michael Hughes added a comment -

          I can definitely confirm this in 2.2.3+

          The code in count_real_submissions() in the upload type has explicit handling for checking the enrolled users (see L411 - L427 mod/assignment/type/upload/assignment.class.php)

          Show
          Michael Hughes added a comment - I can definitely confirm this in 2.2.3+ The code in count_real_submissions() in the upload type has explicit handling for checking the enrolled users (see L411 - L427 mod/assignment/type/upload/assignment.class.php)
          Hide
          Michael Hughes added a comment -

          Proposed patch for Moodle 2.3

          Show
          Michael Hughes added a comment - Proposed patch for Moodle 2.3
          Hide
          Rajesh Taneja added a comment -

          Taking this in next sprint, so I can have a look at it.

          Show
          Rajesh Taneja added a comment - Taking this in next sprint, so I can have a look at it.
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for providing the patch Michael,

          Created a branch on your behalf, although did few alterations on top:

          1. I think we should use mod/assignment:submit capability check rather then mod/assignment:view
          2. Enroled sql should only contain active users.
          3. Changed "u.id =s.userid" to "u.id = a.userid"
          4. Also, changed the way context was retrieved.

          Requesting Damyon to review this, as he is familiar with this area.

          Show
          Rajesh Taneja added a comment - - edited Thanks for providing the patch Michael, Created a branch on your behalf, although did few alterations on top: I think we should use mod/assignment:submit capability check rather then mod/assignment:view Enroled sql should only contain active users. Changed "u.id =s.userid" to "u.id = a.userid" Also, changed the way context was retrieved. Requesting Damyon to review this, as he is familiar with this area.
          Hide
          Damyon Wiese added a comment -

          Linked issue addresses suspended enrollments.

          Show
          Damyon Wiese added a comment - Linked issue addresses suspended enrollments.
          Hide
          Damyon Wiese added a comment -

          This patch is inconsistent with the rest of the usage of get_enrolled_sql in the assignment (passes flag to only get active users).

          Suggest we do not pass that flag so that the linked issue can address this.

          Show
          Damyon Wiese added a comment - This patch is inconsistent with the rest of the usage of get_enrolled_sql in the assignment (passes flag to only get active users). Suggest we do not pass that flag so that the linked issue can address this.
          Hide
          Damyon Wiese added a comment -

          Thanks Raj heres your checklist:

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [Y] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git (Nice giving credit)
          [Y] Sanity check

          If you take out the active param from get_enrolled_sql this is ready for integration.

          Thanks - Damyon

          Show
          Damyon Wiese added a comment - Thanks Raj heres your checklist: [Y] Syntax [-] Output [Y] Whitespace [-] Language [Y] Databases [Y] Testing [-] Security [-] Documentation [Y] Git (Nice giving credit) [Y] Sanity check If you take out the active param from get_enrolled_sql this is ready for integration. Thanks - Damyon
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks Damyon,

          I have removed activeusers, pushing it for integration review.

          FYI:
          If download should only consider activeuser, then it should be fixed by MDL-35731

          Show
          Rajesh Taneja added a comment - - edited Thanks Damyon, I have removed activeusers, pushing it for integration review. FYI: If download should only consider activeuser, then it should be fixed by MDL-35731
          Hide
          Rajesh Taneja added a comment -

          @integrators:
          Sorry forgot to add 23 and 24 branchs,

          Will do that on Tuesday, when I come back.

          Show
          Rajesh Taneja added a comment - @integrators: Sorry forgot to add 23 and 24 branchs, Will do that on Tuesday, when I come back.
          Hide
          Dan Poltawski added a comment -

          (Its OK Raj, I cherry picked)

          Integrated to master, 24 and 23. Thanks!

          Show
          Dan Poltawski added a comment - (Its OK Raj, I cherry picked) Integrated to master, 24 and 23. Thanks!
          Hide
          Rajesh Taneja added a comment -

          Thanks Dan

          Show
          Rajesh Taneja added a comment - Thanks Dan
          Hide
          Andrew Davis added a comment - - edited

          Hi. I'm getting an error in master. This is when, as a teacher, I've viewed the submitted assignments and click "download all assigments as zip".

          The URL is /mod/assignment/submissions.php?id=45&download=zip

          Can not find data record in database table course_modules.
          
          More information about this error
          Debug info: SELECT id,course FROM {course_modules} WHERE id = ?
          [array (
          0 => '1',
          )]
          Error code: invalidrecord
          Stack trace:
          
              line 1376 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
              line 1352 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
              line 6825 of /lib/accesslib.php: call to moodle_database->get_record()
              line 3577 of /mod/assignment/lib.php: call to context_module::instance()
              line 1921 of /mod/assignment/lib.php: call to assignment_get_all_submissions()
              line 435 of /mod/assignment/type/online/assignment.class.php: call to assignment_base->get_submissions()
              line 55 of /mod/assignment/submissions.php: call to assignment_online->download_submissions()
          Show
          Andrew Davis added a comment - - edited Hi. I'm getting an error in master. This is when, as a teacher, I've viewed the submitted assignments and click "download all assigments as zip". The URL is /mod/assignment/submissions.php?id=45&download=zip Can not find data record in database table course_modules. More information about this error Debug info: SELECT id,course FROM {course_modules} WHERE id = ? [array ( 0 => '1', )] Error code: invalidrecord Stack trace: line 1376 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown line 1352 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 6825 of /lib/accesslib.php: call to moodle_database->get_record() line 3577 of /mod/assignment/lib.php: call to context_module::instance() line 1921 of /mod/assignment/lib.php: call to assignment_get_all_submissions() line 435 of /mod/assignment/type/online/assignment.class.php: call to assignment_base->get_submissions() line 55 of /mod/assignment/submissions.php: call to assignment_online->download_submissions()
          Hide
          Rajesh Taneja added a comment - - edited

          Hello Andrew,

          This is valid error, if you are using two browsers and unenrol user from one browser.
          At this point teacher is still viewing submission page, but as student is unenroled, the assignment should not be downloaded.

          If there is valid assignment submission then you should not see this error.

          Show
          Rajesh Taneja added a comment - - edited Hello Andrew, This is valid error, if you are using two browsers and unenrol user from one browser. At this point teacher is still viewing submission page, but as student is unenroled, the assignment should not be downloaded. If there is valid assignment submission then you should not see this error.
          Hide
          David Monllaó added a comment -

          Hi Raj,

          Andrew asked for a second test, I'm receiving the same error:

          Can not find data record in database table course_modules.

          Debug info: SELECT id,course FROM {course_modules} WHERE id = ?
          [array (
          0 => '1',
          )]
          Error code: invalidrecord
          Stack trace:
          line 1376 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
          line 1352 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
          line 6825 of /lib/accesslib.php: call to moodle_database->get_record()
          line 3577 of /mod/assignment/lib.php: call to context_module::instance()
          line 1921 of /mod/assignment/lib.php: call to assignment_get_all_submissions()
          line 435 of /mod/assignment/type/online/assignment.class.php: call to assignment_base->get_submissions()
          line 55 of /mod/assignment/submissions.php: call to assignment_online->download_submissions()
          
          Show
          David Monllaó added a comment - Hi Raj, Andrew asked for a second test, I'm receiving the same error: Can not find data record in database table course_modules. Debug info: SELECT id,course FROM {course_modules} WHERE id = ? [array ( 0 => '1', )] Error code: invalidrecord Stack trace: line 1376 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown line 1352 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 6825 of /lib/accesslib.php: call to moodle_database->get_record() line 3577 of /mod/assignment/lib.php: call to context_module::instance() line 1921 of /mod/assignment/lib.php: call to assignment_get_all_submissions() line 435 of /mod/assignment/type/online/assignment.class.php: call to assignment_base->get_submissions() line 55 of /mod/assignment/submissions.php: call to assignment_online->download_submissions()
          Hide
          David Monllaó added a comment -

          I see the error when downloading the 3 files

          Show
          David Monllaó added a comment - I see the error when downloading the 3 files
          Hide
          Andrew Davis added a comment -

          "This is valid error, if you are using two browsers and unenrol user from one browser."

          This error is before getting to the unenrollment in the testing instructions. I'm also working in a single tab throughout.

          Show
          Andrew Davis added a comment - "This is valid error, if you are using two browsers and unenrol user from one browser." This error is before getting to the unenrollment in the testing instructions. I'm also working in a single tab throughout.
          Hide
          Rajesh Taneja added a comment -

          Thanks David and Andrew,

          I have pushed one more commit to fix this. I was passing wrong info to retrieve context.

          @Dan can you please pick this commit.

          Show
          Rajesh Taneja added a comment - Thanks David and Andrew, I have pushed one more commit to fix this. I was passing wrong info to retrieve context. @Dan can you please pick this commit.
          Hide
          David Monllaó added a comment -

          We've just tested with Rajesh patch integrated and it worked

          Show
          David Monllaó added a comment - We've just tested with Rajesh patch integrated and it worked
          Hide
          Dan Poltawski added a comment -

          Integrated the fix, please re-test.

          Show
          Dan Poltawski added a comment - Integrated the fix, please re-test.
          Hide
          Andrew Davis added a comment -

          It's now working fine. I've raised MDL-37906 as users with a suspended enrolment are still mixed in with students with an active enrolment.

          Show
          Andrew Davis added a comment - It's now working fine. I've raised MDL-37906 as users with a suspended enrolment are still mixed in with students with an active enrolment.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          I think MDL-35731 was mentioning about suspended enrolments.

          Show
          Rajesh Taneja added a comment - Thanks Andrew, I think MDL-35731 was mentioning about suspended enrolments.
          Hide
          Damyon Wiese added a comment -

          Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon

            People

            • Votes:
              9 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: