Assign_print overview does not correctly report number of assignments to be graded

Sometimes the number of submitted assignments is actually displayed, but the number of assignments to be corrected is inaccurate. See the attached screenshots.

Digging into this it seems that the issue is with line 314 (on M2.3) of mod/assign/lib.php

$rs = $DB->get_recordset_sql("SELECT s.assignment as assignment, s.userid as userid, s.id as id, s.status as status, g.timemodified as timegraded
      FROM {assign_submission} s LEFT JOIN {assign_grades} g ON s.userid = g.userid and s.assignment = g.assignment
      WHERE g.timemodified = 0 OR s.timemodified > g.timemodified
      AND s.assignment $sqlassignmentids", $assignmentidparams);

On MySQL the field g.timemodified holds a value of NULL when there isn't a record in the assign_grades table
I found that altering the SQL statement as follows addressed the issue:

$rs = $DB->get_recordset_sql("SELECT s.assignment as assignment, s.userid as userid, s.id as id, s.status as status, g.timemodified as timegraded
      FROM {assign_submission} s LEFT JOIN {assign_grades} g ON s.userid = g.userid and s.assignment = g.assignment
      WHERE ( g.timemodified = 0 OR g.timemodified is NULL OR s.timemodified > g.timemodified )
      AND s.assignment $sqlassignmentids", $assignmentidparams);

The above fix does not correctly handle drafts. The github branches addresses both the display bug and does not count assignments in draft status as well.

Hi Stephen,

Thanks for adding this issue and posting a patch. The patch is well formatted and the git message/branches are good.

About this specific bug:

I agree it would be good to exclude draft items from the count and that the query in assign_print_overview would produce inconsistent results with other places in the module. There are actually 3 queries in the module that all need to be consistent with each other.

(Note: Code here is from a clean branch without your patch applied)
1. mod/assign/lib.php assign_print_overview

WHERE
                                      g.timemodified = 0 OR
                                      s.timemodified > g.timemodified AND
                                      s.assignment ' . $sqlassignmentids, $assignmentidparams);

2. mod/assign/locallib.php count_submissions_need_grading

 s.assignment = :assignid AND
                        s.timemodified IS NOT NULL AND
                        s.status = :submitted AND
                        (s.timemodified > g.timemodified OR g.timemodified IS NULL)

3. mod/assign/gradingtable.php __construct

if ($filter == ASSIGN_FILTER_REQUIRE_GRADING) {
            $where .= ' AND (s.timemodified > g.timemodified OR
                            (s.timemodified IS NOT NULL AND g.timemodified IS NULL))';
        }

Query 2. is the correct one above, the other 2 should be modified to be the same as that query.

Regards, Damyon

Hi Damyon,

I've updated the query in my patch (on github) for assign_print_overview to be consistent with #2 listed above.

I think the changes to query #3 should be done in a separate issue as it is not directly related to this bug. (I've put together a patch here (https://github.com/sbourget/moodle/commit/7fc5416a4db70d0c0b0fb5b4d77291d67b2e1884) based on M23 for that issue).

One thing I noticed is that the status field is defined as a constant in locallib.php. I couldn't find a way to access that constant from assign_print_overview without including locallib.php in that function which would be bad for performance, so for the time being I've hard coded the value into the query. Is there a better way to do this?

-Steve

Thanks for the updated patch Stephen,

IMO it does make sense to include the locallib file even if it's only for the constant - the maintainability is important. I added a patch to do this (and parameterize the query at the same time).

I also cherry-picked your patch for the grading table - I would like all 3 of these counts/filters in sync - so a teacher sees 3 assignments need grading, goes to the grading table and applies the filter and only sees 3 rows.

Unit tests checked and passed.

All branches updated. Sending for integration review.

Thanks Stephen.

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

Integrated (23, 24 & master), thanks!

It passes, all works as expected, tested in 23, 24 and master

A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

(and won't be revisiting it unless some regression is found)

Thanks and ciao