Moodle
  1. Moodle
  2. MDL-31987

Always "No attempts" if disabled "Send for marking" button

    Details

    • Testing Instructions:
      Hide

      THIS ISSUE MUST BE TESTED WITH ALL 4 DBS.

      1. Add a advanced upload assignment and make sure the 'Enable "Send for marking" button' is set to No.
      2. Upload some files in students accounts
      3. In assignment page, teacher should see the number of submissions for all student.
      4. as admin, change the assignment setting "Send for marking" to yes
      5. as student, submit the assignment for final submission
      6. In assignment page, teacher should see the number of submissions for student that had clicked the 'final submission' button.

      Repeat the test with different databases.

      Show
      THIS ISSUE MUST BE TESTED WITH ALL 4 DBS. 1. Add a advanced upload assignment and make sure the 'Enable "Send for marking" button' is set to No. 2. Upload some files in students accounts 3. In assignment page, teacher should see the number of submissions for all student. 4. as admin, change the assignment setting "Send for marking" to yes 5. as student, submit the assignment for final submission 6. In assignment page, teacher should see the number of submissions for student that had clicked the 'final submission' button. Repeat the test with different databases.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31987_assignment
    • Rank:
      38655

      Description

      The bug was introduced in the latest patch: https://github.com/moodle/moodle/commit/9af65cb3accae49a18877f9ebbd508041d67765a

      For advanced upload assignment, if teachers does not enable the "Send for marking" button, they will always get "No attempts have been made on this assignment" no matter how many students have uploaded.

        Issue Links

          Activity

          Hide
          Sunner Sun added a comment -

          Since the release of 2.1.5 and 2.2.2, I think this bug is critical because it is misleading teachers

          Show
          Sunner Sun added a comment - Since the release of 2.1.5 and 2.2.2, I think this bug is critical because it is misleading teachers
          Hide
          Michael de Raadt added a comment -

          Hi, Sunner.

          Thanks for responding so quickly to this change and providing a patch.

          I was able to reproduce this problem.

          Show
          Michael de Raadt added a comment - Hi, Sunner. Thanks for responding so quickly to this change and providing a patch. I was able to reproduce this problem.
          Hide
          Rossiani Wijaya added a comment -

          Hi Sunner,

          Thank you for reporting and supplying a patch. However, the issue occurs on both situation for send for marking setting. Your patch does fixed the case of disabling it, but not not the other.

          I had a quick fix patch to address the issue by changing the query field from s.data2 to s.timemodified within count_real_submissions(). But I think this should be fixed by assigning the value of ASSIGNMENT_STATUS_XXX (eg: SUBMITTED) into data2 column (during file upload perhaps).

          I will create a proper patch to fix this issue. In the meanwhile, feel free to take a look the quick fix patch.

          Show
          Rossiani Wijaya added a comment - Hi Sunner, Thank you for reporting and supplying a patch. However, the issue occurs on both situation for send for marking setting. Your patch does fixed the case of disabling it, but not not the other. I had a quick fix patch to address the issue by changing the query field from s.data2 to s.timemodified within count_real_submissions(). But I think this should be fixed by assigning the value of ASSIGNMENT_STATUS_XXX (eg: SUBMITTED) into data2 column (during file upload perhaps). I will create a proper patch to fix this issue. In the meanwhile, feel free to take a look the quick fix patch.
          Hide
          Sunner Sun added a comment -

          Hi Rossiani,

          I do not think my patch works only when the send for marking setting is disable.

          If you get "No attempts" when enable send for marking, it is because that no students have press the "send for marking" button. The patch which introduced this bug is intend to make assignment work like this.

          Show
          Sunner Sun added a comment - Hi Rossiani, I do not think my patch works only when the send for marking setting is disable. If you get "No attempts" when enable send for marking, it is because that no students have press the "send for marking" button. The patch which introduced this bug is intend to make assignment work like this.
          Hide
          Steven Chu added a comment - - edited

          Hi Sunner,

          Thanks for the patch, it works.
          But I suggest to add isopen() to handle the case after due date. Because after dute date, there are no indicator to show if the assigment is a draft or not. It will confuse the teachers
          Following is the code:

          // Untracked assignments need NOT judge ASSIGNMENT_STATUS_SUBMITTED
          // Call parent to count submission records
          if (!$this->drafts_tracked() or !$this->isopen())

          { return parent::count_real_submissions($groupid); }
          Show
          Steven Chu added a comment - - edited Hi Sunner, Thanks for the patch, it works. But I suggest to add isopen() to handle the case after due date. Because after dute date, there are no indicator to show if the assigment is a draft or not. It will confuse the teachers Following is the code: // Untracked assignments need NOT judge ASSIGNMENT_STATUS_SUBMITTED // Call parent to count submission records if (!$this->drafts_tracked() or !$this->isopen()) { return parent::count_real_submissions($groupid); }
          Hide
          Rossiani Wijaya added a comment -

          Hi Sunner,

          Thank you for clarifying the issue. I think the best way to fix this is not to refer it back to parent::count_real_submissions(), but rather within the upload count_real_submissions() itself. It will help preventing error in the future if parent::count_real_submissions() is being modify.

          Hi Steve,
          I'm not quite sure by indicating draft assignment after due date. After the due date isn't the submitted file will become student final submission file? Could you provide a scenario for this case?

          Show
          Rossiani Wijaya added a comment - Hi Sunner, Thank you for clarifying the issue. I think the best way to fix this is not to refer it back to parent::count_real_submissions(), but rather within the upload count_real_submissions() itself. It will help preventing error in the future if parent::count_real_submissions() is being modify. Hi Steve, I'm not quite sure by indicating draft assignment after due date. After the due date isn't the submitted file will become student final submission file? Could you provide a scenario for this case?
          Hide
          Sunner Sun added a comment -

          I agree with Steven.

          Perhaps we should draw a clear picture about the number of submissions in different situation of upload assignment. So that we can follow rossiani's opinion to make count_real_submissions() better. Let me try first and wait for your improvement.

          If !is_open() or (is_open() and !drafts_tracked()), the number should be the count of all submissions with at least one file no matter whether they are ASSIGNMENT_STATUS_SUBMITTED

          Else the number should be the count of all submissions which are ASSIGNMENT_STATUS_SUBMITTED.

          The problem is: how to effectively count the file number of submissions. In current code, a clicking of the upload button will create a submission record even though no files are really uploaded. And also, removing all files will not remove the submission record. So the only way to count files is query file storage of each submission. It is really a heavy load.

          I suggest to modify function upload_file(), enable it to count files after uploading and store the number in numfiles field of assignment_submissions table. Then the above problem is solved.

          Show
          Sunner Sun added a comment - I agree with Steven. Perhaps we should draw a clear picture about the number of submissions in different situation of upload assignment. So that we can follow rossiani's opinion to make count_real_submissions() better. Let me try first and wait for your improvement. If !is_open() or (is_open() and !drafts_tracked()), the number should be the count of all submissions with at least one file no matter whether they are ASSIGNMENT_STATUS_SUBMITTED Else the number should be the count of all submissions which are ASSIGNMENT_STATUS_SUBMITTED. The problem is: how to effectively count the file number of submissions. In current code, a clicking of the upload button will create a submission record even though no files are really uploaded. And also, removing all files will not remove the submission record. So the only way to count files is query file storage of each submission. It is really a heavy load. I suggest to modify function upload_file(), enable it to count files after uploading and store the number in numfiles field of assignment_submissions table. Then the above problem is solved.
          Hide
          Michael de Raadt added a comment -

          A patch has been provided in a duplicate issue. That might be worth a look.

          Show
          Michael de Raadt added a comment - A patch has been provided in a duplicate issue. That might be worth a look.
          Hide
          Sunner Sun added a comment -

          I just updated my patch which follows my last comment.

          For updating numfiles of existing submissions, the patch increase assignment version number to trigger upgrade.php which updates all records. The process is slow in big site, so I also ini_set('max_execution_time', 600) and show a progress bar.

          I have tested it in my development site (160k+ submissions) with different tracking option, number of files uploaded. It seems work. Ask for more suggest, test and review

          Show
          Sunner Sun added a comment - I just updated my patch which follows my last comment. For updating numfiles of existing submissions, the patch increase assignment version number to trigger upgrade.php which updates all records. The process is slow in big site, so I also ini_set('max_execution_time', 600) and show a progress bar. I have tested it in my development site (160k+ submissions) with different tracking option, number of files uploaded. It seems work. Ask for more suggest, test and review
          Hide
          Rossiani Wijaya added a comment -

          Hi Sunner,

          Thank you for updating the patch.

          I modified your patch a little bit to reduce query redundancy and added non-mySQL DBs field comparison (MDL-31987).

          Show
          Rossiani Wijaya added a comment - Hi Sunner, Thank you for updating the patch. I modified your patch a little bit to reduce query redundancy and added non-mySQL DBs field comparison ( MDL-31987 ).
          Hide
          Sunner Sun added a comment -

          Hi Rossiani,

          Although I will lost credit in git history, your modification is good. But I notice that you increased the indent level of line 405-409 in mod/assignment/type/upload/assignment.class.php (https://github.com/rwijaya/moodle/compare/MOODLE_21_STABLE...MDL-31987_assignment_m21#diff-1). I don't think it is a good style

          Show
          Sunner Sun added a comment - Hi Rossiani, Although I will lost credit in git history, your modification is good. But I notice that you increased the indent level of line 405-409 in mod/assignment/type/upload/assignment.class.php ( https://github.com/rwijaya/moodle/compare/MOODLE_21_STABLE...MDL-31987_assignment_m21#diff-1 ). I don't think it is a good style
          Hide
          Rossiani Wijaya added a comment -

          Hi Sunner,

          Thank you for reviewing the patch and I will update the alignment for those lines.

          Also, I had mention your name and email as part of the commit's comment.

          Show
          Rossiani Wijaya added a comment - Hi Sunner, Thank you for reviewing the patch and I will update the alignment for those lines. Also, I had mention your name and email as part of the commit's comment.
          Hide
          Ankit Agarwal added a comment -

          Hi Rosie,
          Thanks for explaining the logical flow of the patch. A few things can be done to improve the patch:-

          > $DB>get_recordset_sql can be used instead of $DB->get_records_sql during upgrade, since we expect it to be a large number
          -> instead of get_context_instance the new context classes should be used (context_module::instance) (Master only)
          -> Also a comment, so as why we are doing the upgrade in upgrade.php will help us in future.
          Rest looks good.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Rosie, Thanks for explaining the logical flow of the patch. A few things can be done to improve the patch:- > $DB >get_recordset_sql can be used instead of $DB->get_records_sql during upgrade, since we expect it to be a large number -> instead of get_context_instance the new context classes should be used (context_module::instance) (Master only) -> Also a comment, so as why we are doing the upgrade in upgrade.php will help us in future. Rest looks good. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Thanks Ankit for the feedback.

          Patches have been updated.

          Submitting for integration review.

          Show
          Rossiani Wijaya added a comment - Thanks Ankit for the feedback. Patches have been updated. Submitting for integration review.
          Hide
          Rossiani Wijaya added a comment -

          Side note:

          I noticed that XXX_recordset::rewind is not exist within xxx_moodle_recordset, not sure if this is intentional. The rewind() might be useful while working with iterator object.

          Show
          Rossiani Wijaya added a comment - Side note: I noticed that XXX_recordset::rewind is not exist within xxx_moodle_recordset, not sure if this is intentional. The rewind() might be useful while working with iterator object.
          Hide
          Dan Poltawski added a comment -

          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

          Show
          Dan Poltawski added a comment - 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
          Hide
          Dan Poltawski added a comment -

          Hi,

          I'm afraid this needs some more work - You should use upgrade_set_timeout() to raise the limit rather than init_set like other parts of Moodle do.

          The point of using recordsets is to reduce memory by processing each item at a time rather than generating one big php object with all the data in it. Therefore by doing iterator_to_array() you are rendering the use of the recordset pointless. You should either use a recordset and loop through the iterator (recommended) or just use get_records_sql (which produces this php object in a cleaner way)

          So reopening this

          Show
          Dan Poltawski added a comment - Hi, I'm afraid this needs some more work - You should use upgrade_set_timeout() to raise the limit rather than init_set like other parts of Moodle do. The point of using recordsets is to reduce memory by processing each item at a time rather than generating one big php object with all the data in it. Therefore by doing iterator_to_array() you are rendering the use of the recordset pointless. You should either use a recordset and loop through the iterator (recommended) or just use get_records_sql (which produces this php object in a cleaner way) So reopening this
          Hide
          Rossiani Wijaya added a comment -

          Thanks Dan for the review.

          Update patch.

          Re-submit for integration review.

          Show
          Rossiani Wijaya added a comment - Thanks Dan for the review. Update patch. Re-submit for integration review.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Dan Poltawski added a comment -

          Hi Rossie,

          Sorry I should've been clearer - this needs to be a recordset, big sites will literally have millions of assignment submissions so we need to be incredibly careful about this upgrade step.

          Also I didn't spot last time thatyou should only be selecting assignment submissions which are related to the affect modules (upload and uploadsingle) I think?

          Finally, since this upgrade step is so critical (its a very fundamental module heavily used by many sites) can I ask if we can try and find someone from the community with a large amount of test data to try this patch out on some test data. I'm sure there are a number of universities who would be interested.

          Show
          Dan Poltawski added a comment - Hi Rossie, Sorry I should've been clearer - this needs to be a recordset, big sites will literally have millions of assignment submissions so we need to be incredibly careful about this upgrade step. Also I didn't spot last time thatyou should only be selecting assignment submissions which are related to the affect modules (upload and uploadsingle) I think? Finally, since this upgrade step is so critical (its a very fundamental module heavily used by many sites) can I ask if we can try and find someone from the community with a large amount of test data to try this patch out on some test data. I'm sure there are a number of universities who would be interested.
          Hide
          Rossiani Wijaya added a comment -

          Patch updated

          Show
          Rossiani Wijaya added a comment - Patch updated
          Hide
          Rossiani Wijaya added a comment -
          Show
          Rossiani Wijaya added a comment - Forum post: http://moodle.org/mod/forum/discuss.php?d=201732
          Hide
          Sunner Sun added a comment -

          I tested the latest patch in 21 branch. It works well on my site with 154769 related submissions.

          But since we are using a 3rd party assignment type, onlinejudge, which is a child of upload, I modified the patch to include onlinejudge type as well.

          Show
          Sunner Sun added a comment - I tested the latest patch in 21 branch. It works well on my site with 154769 related submissions. But since we are using a 3rd party assignment type, onlinejudge, which is a child of upload, I modified the patch to include onlinejudge type as well.
          Hide
          Rossiani Wijaya added a comment -

          Thanks Sunner for testing the patch.

          Show
          Rossiani Wijaya added a comment - Thanks Sunner for testing the patch.
          Hide
          Michael de Raadt added a comment -

          The counting of submissions is now accurate, however there is a side-effect that appears when a teacher switches back-and-forth between "yes" and "no" for the "Send for marking" setting. Submissions that a student considered submitted could become drafts and drafts could become submitted without the student's involvement. A new issue should be created to address the labelling/help of this setting in the assignment configuration form.

          Show
          Michael de Raadt added a comment - The counting of submissions is now accurate, however there is a side-effect that appears when a teacher switches back-and-forth between "yes" and "no" for the "Send for marking" setting. Submissions that a student considered submitted could become drafts and drafts could become submitted without the student's involvement. A new issue should be created to address the labelling/help of this setting in the assignment configuration form.
          Hide
          Rossiani Wijaya added a comment -

          rebased patch.

          submitting for integration review.

          Show
          Rossiani Wijaya added a comment - rebased patch. submitting for integration review.
          Hide
          Rossiani Wijaya added a comment -

          Created issue to fix labelling/help for the setting of assignment's configuration MDL-32948.

          Show
          Rossiani Wijaya added a comment - Created issue to fix labelling/help for the setting of assignment's configuration MDL-32948 .
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Dan Poltawski added a comment -

          Hi Rosie,

          Sorry, but this is failing on postgres:

          -->mod_assignment
          Default exception handler: Error reading from database Debug: ERROR:  column "cm.id" must appear in the GROUP BY clause or be used in an aggregate function
          LINE 1: SELECT COUNT(s.id), cm.id AS cmid  FROM mdl_assignment_submi...
                                      ^
          SELECT COUNT(s.id), cm.id AS cmid  FROM mdl_assignment_submissions s
                              INNER JOIN mdl_course_modules cm
                                      ON s.assignment = cm.instance
                                    JOIN mdl_assignment a
                                      ON a.id = s.assignment
                                   WHERE a.assignmenttype in ('upload', 'uploadsingle') AND
                                         cm.module = (SELECT id
                                                        FROM mdl_modules
                                                       WHERE name = 'assignment')
          [array (
          )]
          * line 413 of /lib/dml/moodle_database.php: dml_read_exception thrown
          * line 243 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
          * line 703 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
          * line 1341 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
          * line 1414 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
          * line 1586 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql()
          * line 50 of /mod/assignment/db/upgrade.php: call to moodle_database->count_records_sql()
          * line 627 of /lib/upgradelib.php: call to xmldb_assignment_upgrade()
          * line 358 of /lib/upgradelib.php: call to upgrade_plugins_modules()
          * line 1524 of /lib/upgradelib.php: call to upgrade_plugins()
          * line 156 of /admin/cli/upgrade.php: call to upgrade_noncore()
          
          !!! Error reading from database !!!
          !! ERROR:  column "cm.id" must appear in the GROUP BY clause or be used in an aggregate function
          LINE 1: SELECT COUNT(s.id), cm.id AS cmid  FROM mdl_assignment_submi...
                                      ^
          SELECT COUNT(s.id), cm.id AS cmid  FROM mdl_assignment_submissions s
                              INNER JOIN mdl_course_modules cm
                                      ON s.assignment = cm.instance
                                    JOIN mdl_assignment a
                                      ON a.id = s.assignment
                                   WHERE a.assignmenttype in ('upload', 'uploadsingle') AND
                                         cm.module = (SELECT id
                                                        FROM mdl_modules
                                                       WHERE name = 'assignment')
          [array (
          )] !!
          !! Stack trace: * line 413 of /lib/dml/moodle_database.php: dml_read_exception thrown
          * line 243 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
          * line 703 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
          * line 1341 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
          * line 1414 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
          * line 1586 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql()
          * line 50 of /mod/assignment/db/upgrade.php: call to moodle_database->count_records_sql()
          * line 627 of /lib/upgradelib.php: call to xmldb_assignment_upgrade()
          * line 358 of /lib/upgradelib.php: call to upgrade_plugins_modules()
          * line 1524 of /lib/upgradelib.php: call to upgrade_plugins()
          * line 156 of /admin/cli/upgrade.php: call to upgrade_noncore()
          
          Show
          Dan Poltawski added a comment - Hi Rosie, Sorry, but this is failing on postgres: -->mod_assignment Default exception handler: Error reading from database Debug: ERROR: column "cm.id" must appear in the GROUP BY clause or be used in an aggregate function LINE 1: SELECT COUNT(s.id), cm.id AS cmid FROM mdl_assignment_submi... ^ SELECT COUNT(s.id), cm.id AS cmid FROM mdl_assignment_submissions s INNER JOIN mdl_course_modules cm ON s.assignment = cm.instance JOIN mdl_assignment a ON a.id = s.assignment WHERE a.assignmenttype in ('upload', 'uploadsingle') AND cm.module = (SELECT id FROM mdl_modules WHERE name = 'assignment') [array ( )] * line 413 of /lib/dml/moodle_database.php: dml_read_exception thrown * line 243 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() * line 703 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() * line 1341 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql() * line 1414 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql() * line 1586 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql() * line 50 of /mod/assignment/db/upgrade.php: call to moodle_database->count_records_sql() * line 627 of /lib/upgradelib.php: call to xmldb_assignment_upgrade() * line 358 of /lib/upgradelib.php: call to upgrade_plugins_modules() * line 1524 of /lib/upgradelib.php: call to upgrade_plugins() * line 156 of /admin/cli/upgrade.php: call to upgrade_noncore() !!! Error reading from database !!! !! ERROR: column "cm.id" must appear in the GROUP BY clause or be used in an aggregate function LINE 1: SELECT COUNT(s.id), cm.id AS cmid FROM mdl_assignment_submi... ^ SELECT COUNT(s.id), cm.id AS cmid FROM mdl_assignment_submissions s INNER JOIN mdl_course_modules cm ON s.assignment = cm.instance JOIN mdl_assignment a ON a.id = s.assignment WHERE a.assignmenttype in ('upload', 'uploadsingle') AND cm.module = (SELECT id FROM mdl_modules WHERE name = 'assignment') [array ( )] !! !! Stack trace: * line 413 of /lib/dml/moodle_database.php: dml_read_exception thrown * line 243 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() * line 703 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() * line 1341 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql() * line 1414 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql() * line 1586 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql() * line 50 of /mod/assignment/db/upgrade.php: call to moodle_database->count_records_sql() * line 627 of /lib/upgradelib.php: call to xmldb_assignment_upgrade() * line 358 of /lib/upgradelib.php: call to upgrade_plugins_modules() * line 1524 of /lib/upgradelib.php: call to upgrade_plugins() * line 156 of /admin/cli/upgrade.php: call to upgrade_noncore()
          Hide
          Dan Poltawski added a comment -

          I've taken a deeper look at this upgrade script and created a patch:
          https://github.com/danpoltawski/moodle/commit/ed84db09c961e483a6a624f98e5466bb64f10699

          Some things I did:
          1/ Strip out the subselect, since its only one query and we are calling it twice anyway.
          2/ Only selected assignments which were of type 'upload' and with var4 set to 0 (which I believe is the sendformarking button switched off?
          3/Did only joined on the right records, rather than doing it in the WHERE clause.
          4/ Don't select cmid in the count clause

          Show
          Dan Poltawski added a comment - I've taken a deeper look at this upgrade script and created a patch: https://github.com/danpoltawski/moodle/commit/ed84db09c961e483a6a624f98e5466bb64f10699 Some things I did: 1/ Strip out the subselect, since its only one query and we are calling it twice anyway. 2/ Only selected assignments which were of type 'upload' and with var4 set to 0 (which I believe is the sendformarking button switched off? 3/Did only joined on the right records, rather than doing it in the WHERE clause. 4/ Don't select cmid in the count clause
          Hide
          Sunner Sun added a comment -

          Hi, Dan

          For 2/, I have an opinion for consider.

          If an upload assignment's sendformarking button was turned on, the numfiles field will not be updated in your patch. After upgrading, if the teacher think it is better to switch the button off, then all the old submissions will not be count. Am I right?

          Thanks

          Show
          Sunner Sun added a comment - Hi, Dan For 2/, I have an opinion for consider. If an upload assignment's sendformarking button was turned on, the numfiles field will not be updated in your patch. After upgrading, if the teacher think it is better to switch the button off, then all the old submissions will not be count. Am I right? Thanks
          Hide
          Ian Luxton added a comment -

          Hi

          I have just come across this error BUT I do have the Send for Marking enabled and tested with both ways and the Submission dialogue says none there. Further before the last update 2.2.3( 20102514) all was good. Interestingly, assignments completed before the patch do act correctly both with and without the button enabled

          Show
          Ian Luxton added a comment - Hi I have just come across this error BUT I do have the Send for Marking enabled and tested with both ways and the Submission dialogue says none there. Further before the last update 2.2.3( 20102514) all was good. Interestingly, assignments completed before the patch do act correctly both with and without the button enabled
          Hide
          Rossiani Wijaya added a comment -

          Hi Dan,

          I agree with Sunner, I think the query shouldn't include the var4 checked for the upgrade.

          Other than that, the new query is working great.

          Thanks

          Show
          Rossiani Wijaya added a comment - Hi Dan, I agree with Sunner, I think the query shouldn't include the var4 checked for the upgrade. Other than that, the new query is working great. Thanks
          Hide
          Dan Poltawski added a comment -

          Thanks guys - i had not considered that. So thanks for pointing it out!

          Show
          Dan Poltawski added a comment - Thanks guys - i had not considered that. So thanks for pointing it out!
          Hide
          Michael de Raadt added a comment -

          Carrying over to new sprint.

          Show
          Michael de Raadt added a comment - Carrying over to new sprint.
          Hide
          Rossiani Wijaya added a comment -

          Update patch.

          Requesting for peer review.

          Show
          Rossiani Wijaya added a comment - Update patch. Requesting for peer review.
          Hide
          Ankit Agarwal added a comment -

          Looks good Rosie.
          +1 to integrate
          Thanks

          Show
          Ankit Agarwal added a comment - Looks good Rosie. +1 to integrate Thanks
          Hide
          Rossiani Wijaya added a comment -

          Thanks Ankit for reviewing.

          Submitting for integration review.

          Show
          Rossiani Wijaya added a comment - Thanks Ankit for reviewing. Submitting for integration review.
          Hide
          Dan Poltawski added a comment -

          Hi Rosie,

          Sorry - you got me to look at this and I completely missed this.

          You can't upgrade the version numbers in the stable branches like you are doing because this would prevent the upgrade steps from happening between branches.

          I think the version numbers are currently:
          21 = 2010102600, 22 = 2011112900
          They should be changed to:
          21 = 2010102601, 22 = 2011112901

          (This means that the upgrade steps between 21 and 23 are not missed)

          So could you please update that to makes sure the version numbers are kept in branches.

          Show
          Dan Poltawski added a comment - Hi Rosie, Sorry - you got me to look at this and I completely missed this. You can't upgrade the version numbers in the stable branches like you are doing because this would prevent the upgrade steps from happening between branches. I think the version numbers are currently: 21 = 2010102600, 22 = 2011112900 They should be changed to: 21 = 2010102601, 22 = 2011112901 (This means that the upgrade steps between 21 and 23 are not missed) So could you please update that to makes sure the version numbers are kept in branches.
          Hide
          Rossiani Wijaya added a comment -

          Hi Dan,

          I updated the patch. Please let me know if there's anything else need to be change.

          Show
          Rossiani Wijaya added a comment - Hi Dan, I updated the patch. Please let me know if there's anything else need to be change.
          Hide
          Dan Poltawski added a comment -

          Perfect, thanks Rosie.

          Integrated to 21, 22, 23 and master.

          Show
          Dan Poltawski added a comment - Perfect, thanks Rosie. Integrated to 21, 22, 23 and master.
          Hide
          Tim Barker added a comment -

          Sorry, removing myself as the tester of this. Although I have MYSQL and Postgres I don't have Oracle or MSSQL.

          Show
          Tim Barker added a comment - Sorry, removing myself as the tester of this. Although I have MYSQL and Postgres I don't have Oracle or MSSQL.
          Hide
          Frédéric Massart added a comment -

          Lol. Didn't read the 4 DB pre-requisite... unassigning.

          Show
          Frédéric Massart added a comment - Lol. Didn't read the 4 DB pre-requisite... unassigning.
          Hide
          Aparup Banerjee added a comment -

          this worked fine for me on pg, mysql, mssql and SQLSRV. oracle pending.

          Show
          Aparup Banerjee added a comment - this worked fine for me on pg, mysql, mssql and SQLSRV. oracle pending.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just confirming it's working ok here under 23_STABLE and Oracle.

          Just one side note, the assignment page shows the correct "View X submitted assignments", but clicking on it, the "grading" page shows ALL the submissions, not only the ones sent for grading. Surely it would be more consistent if, with the setting enabled... the default list shows only the submissions sent for grading instead of all.

          Show
          Eloy Lafuente (stronk7) added a comment - Just confirming it's working ok here under 23_STABLE and Oracle. Just one side note, the assignment page shows the correct "View X submitted assignments", but clicking on it, the "grading" page shows ALL the submissions, not only the ones sent for grading. Surely it would be more consistent if, with the setting enabled... the default list shows only the submissions sent for grading instead of all.
          Hide
          Sebastian Tennant added a comment -

          Seems fine to me (only tested with PostgreSQL mind you).

          I don't really agree with Eloy's side note (directly above). When you change the course to 'Send for marking', only final submissions are counted and non-final submissions are clearly labelled 'Draft'. This seems reasonable to me.

          Show
          Sebastian Tennant added a comment - Seems fine to me (only tested with PostgreSQL mind you). I don't really agree with Eloy's side note (directly above). When you change the course to 'Send for marking', only final submissions are counted and non-final submissions are clearly labelled 'Draft'. This seems reasonable to me.
          Hide
          Aparup Banerjee added a comment -

          Thanks for all the help testing this. So testing wise ,Passing this test.

          Show
          Aparup Banerjee added a comment - Thanks for all the help testing this. So testing wise ,Passing this test.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          LOL "clearly labelled 'Draft'". Believe me I didn't see that (and really looked for differences!). Sure it's me, ignore.

          Show
          Eloy Lafuente (stronk7) added a comment - LOL "clearly labelled 'Draft'". Believe me I didn't see that (and really looked for differences!). Sure it's me, ignore.
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

            • Votes:
              34 Vote for this issue
              Watchers:
              34 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: