Moodle
  1. Moodle
  2. MDL-37533

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1, 2.5
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide

      For 23, 24 and 25 branches:

      To test:

      1. Create a course with 1 teacher, and 2 students.
      2. As the teacher create an assignment named A1
      3. Set a future due date in the future
      4. Set require students to click submit button to "Yes"
      5. Save the assignment
      6. As the teacher create an assignment named A2
      7. Set a future due date in the future
      8. Set require students to click submit button to "no"
      9. Save the assignment
      10 Navigate to the "My Moodle" page. Both assignments should be listed

      As student1:
      1. Log into moodle and navigate to the My Moodle page.
      2. Two assignments should be listed.
      3. Click on assignment A1
      4. Upload a file and submit the assignment.
      5. Be sure to click the submit assignment button
      6. Return to the my page
      7. Click on assignment A2
      8. Upload a file and save.

      As student2
      1. Log into moodle and navigate to the My Moodle page.
      2. Two assignments should be listed.
      3. Click on assignment A1
      4. Upload a file and save.
      5. DO NOT click the submit assignment button (We want this in draft status)
      6. Return to the my page
      7. Click on assignment A2
      8. Upload a file and save.

      As the teacher
      1. Visit the My Moodle page
      2. Assignment A1 should show that there is 1 assignment needing to be graded
      3. Assignment A2 should show that there are 2 assignments needing to be graded
      4. Visit the grading page for each assignment. Enable the "Requires grading" filter.
      5. Verify the number of rows in the grading table matches the number of assignments needing to be graded on the "My Moodle" page.

      Show
      For 23, 24 and 25 branches: To test: 1. Create a course with 1 teacher, and 2 students. 2. As the teacher create an assignment named A1 3. Set a future due date in the future 4. Set require students to click submit button to "Yes" 5. Save the assignment 6. As the teacher create an assignment named A2 7. Set a future due date in the future 8. Set require students to click submit button to "no" 9. Save the assignment 10 Navigate to the "My Moodle" page. Both assignments should be listed As student1: 1. Log into moodle and navigate to the My Moodle page. 2. Two assignments should be listed. 3. Click on assignment A1 4. Upload a file and submit the assignment. 5. Be sure to click the submit assignment button 6. Return to the my page 7. Click on assignment A2 8. Upload a file and save. As student2 1. Log into moodle and navigate to the My Moodle page. 2. Two assignments should be listed. 3. Click on assignment A1 4. Upload a file and save. 5. DO NOT click the submit assignment button (We want this in draft status) 6. Return to the my page 7. Click on assignment A2 8. Upload a file and save. As the teacher 1. Visit the My Moodle page 2. Assignment A1 should show that there is 1 assignment needing to be graded 3. Assignment A2 should show that there are 2 assignments needing to be graded 4. Visit the grading page for each assignment. Enable the "Requires grading" filter. 5. Verify the number of rows in the grading table matches the number of assignments needing to be graded on the "My Moodle" page.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-37533-master
    • Rank:
      47180

      Description

      Assignments displayed on the course overview block (on the my moodle page) do not accurately reflect the number of submissions to be graded. The attached screenshots show the issue:

      Steps to reproduce:
      1. Create a course with 3 users (1 Teacher, 2 students)
      2. Create a assignment allowing file uploads with a future due date
      3. Have one of the students submit a file
      4. Log in as the teacher and view the my moodle page

      Expected result: You should have a message saying that there is one submission needing grading

      Actual result: no information is displayed

      Note: I was able to reproduce this using the Moodle QA site using the
      "NEW File Assignment with Multiple File Uploads (Currently 2 Files)" existing assignment.

      1. Assignment1 - Course.png
        38 kB
      2. Assignment1 -MyMoodle.png
        17 kB
      3. Assignment2 - Course.png
        29 kB
      4. Assignment2 -MyMoodle.png
        20 kB

        Activity

        Hide
        Stephen Bourget added a comment -

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

        Show
        Stephen Bourget added a comment - Sometimes the number of submitted assignments is actually displayed, but the number of assignments to be corrected is inaccurate. See the attached screenshots.
        Hide
        Stephen Bourget added a comment -

        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);
        
        Show
        Stephen Bourget added a comment - 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);
        Hide
        Stephen Bourget added a comment -

        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.

        Show
        Stephen Bourget added a comment - 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.
        Hide
        Damyon Wiese added a comment -

        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

        Show
        Damyon Wiese added a comment - 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
        Hide
        Stephen Bourget added a comment -

        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

        Show
        Stephen Bourget added a comment - 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
        Hide
        Damyon Wiese added a comment -

        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.

        Show
        Damyon Wiese added a comment - 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.
        Hide
        Damyon Wiese added a comment -

        Unit tests checked and passed.

        Show
        Damyon Wiese added a comment - Unit tests checked and passed.
        Hide
        Damyon Wiese added a comment -

        All branches updated. Sending for integration review.

        Thanks Stephen.

        Show
        Damyon Wiese added a comment - All branches updated. Sending for integration review. Thanks Stephen.
        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
        Eloy Lafuente (stronk7) added a comment -

        Integrated (23, 24 & master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
        Hide
        David Monllaó added a comment -

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

        Show
        David Monllaó added a comment - It passes, all works as expected, tested in 23, 24 and master
        Hide
        Eloy Lafuente (stronk7) added a comment -

        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

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved: