Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            sunner 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 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
            salvetore 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
            salvetore 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
            rwijaya 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
            rwijaya 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 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 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
            steven99 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
            steven99 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
            rwijaya 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
            rwijaya 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 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 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
            salvetore Michael de Raadt added a comment -

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

            Show
            salvetore Michael de Raadt added a comment - A patch has been provided in a duplicate issue. That might be worth a look.
            Hide
            sunner 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 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
            rwijaya 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
            rwijaya 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 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 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
            rwijaya 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
            rwijaya 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_frenz 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_frenz 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
            rwijaya Rossiani Wijaya added a comment -

            Thanks Ankit for the feedback.

            Patches have been updated.

            Submitting for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks Ankit for the feedback. Patches have been updated. Submitting for integration review.
            Hide
            rwijaya 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
            rwijaya 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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
            rwijaya Rossiani Wijaya added a comment -

            Thanks Dan for the review.

            Update patch.

            Re-submit for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks Dan for the review. Update patch. Re-submit for integration review.
            Hide
            stronk7 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
            stronk7 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
            poltawski 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
            poltawski 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
            rwijaya Rossiani Wijaya added a comment -

            Patch updated

            Show
            rwijaya Rossiani Wijaya added a comment - Patch updated
            Hide
            rwijaya Rossiani Wijaya added a comment -
            Show
            rwijaya Rossiani Wijaya added a comment - Forum post: http://moodle.org/mod/forum/discuss.php?d=201732
            Hide
            sunner 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 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
            rwijaya Rossiani Wijaya added a comment -

            Thanks Sunner for testing the patch.

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks Sunner for testing the patch.
            Hide
            salvetore 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
            salvetore 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
            rwijaya Rossiani Wijaya added a comment -

            rebased patch.

            submitting for integration review.

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

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

            Show
            rwijaya Rossiani Wijaya added a comment - Created issue to fix labelling/help for the setting of assignment's configuration MDL-32948 .
            Hide
            stronk7 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
            stronk7 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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 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 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
            luxo52 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
            luxo52 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
            rwijaya 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
            rwijaya 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
            poltawski Dan Poltawski added a comment -

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

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

            Carrying over to new sprint.

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

            Update patch.

            Requesting for peer review.

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

            Looks good Rosie.
            +1 to integrate
            Thanks

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

            Thanks Ankit for reviewing.

            Submitting for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks Ankit for reviewing. Submitting for integration review.
            Hide
            poltawski 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
            poltawski 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
            rwijaya 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
            rwijaya 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
            poltawski Dan Poltawski added a comment -

            Perfect, thanks Rosie.

            Integrated to 21, 22, 23 and master.

            Show
            poltawski Dan Poltawski added a comment - Perfect, thanks Rosie. Integrated to 21, 22, 23 and master.
            Hide
            timb 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
            timb 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
            fred Frédéric Massart added a comment -

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

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

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

            Show
            nebgor Aparup Banerjee added a comment - this worked fine for me on pg, mysql, mssql and SQLSRV. oracle pending.
            Hide
            stronk7 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
            stronk7 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
            sebyte 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
            sebyte 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
            nebgor Aparup Banerjee added a comment -

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

            Show
            nebgor Aparup Banerjee added a comment - Thanks for all the help testing this. So testing wise ,Passing this test.
            Hide
            stronk7 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
            stronk7 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
            samhemelryk 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
            samhemelryk 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:
                  Fix Release Date:
                  9/Jul/12