Moodle
  1. Moodle
  2. MDL-36667

Feedback Available mail is sent for mod_assign when no feedback is available yet if you lock assignments from further changes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.3
    • Fix Version/s: 2.3.4, 2.4.1
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide
      1. Make sure automated crons are not running
      2. Login as admin and manually run cron (/admin/cron.php)
      3. Create an assignment in a course with at least 4 students.
      4. Login as a teacher and go to the grading page for the assignment.
      5. Give the first submission a grade
      6. Lock the second submission ("Prevent submission updates" in the menu for this row)
      7. Give the second submission a grade
      8. Give the third submission a grade
      9. Lock the third submission ("Prevent submission updates" in the menu for this row)
      10. Lock the forth submission ("Prevent submission updates" in the menu for this row)
      11. Now login as admin and manually run cron again (/admin/cron.php)
      12. There will be a message in the cron output: "Done processing 3 assignment submissions"
      Show
      Make sure automated crons are not running Login as admin and manually run cron (/admin/cron.php) Create an assignment in a course with at least 4 students. Login as a teacher and go to the grading page for the assignment. Give the first submission a grade Lock the second submission ("Prevent submission updates" in the menu for this row) Give the second submission a grade Give the third submission a grade Lock the third submission ("Prevent submission updates" in the menu for this row) Lock the forth submission ("Prevent submission updates" in the menu for this row) Now login as admin and manually run cron again (/admin/cron.php) There will be a message in the cron output: "Done processing 3 assignment submissions"
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-36667-master
    • Rank:
      46166

      Description

      I've just seen this on our 2.3 site and have replicated against master.

      When a teacher locks submissions to prevent future changes, an entry is created in the assign_grades table. At present, this is the only requirement to send e-mail to students informing them that a grade and/or feedback has been left.

      This probably should check whether a grade or feedback has been left.

      Replication instructions:

      • Create a new assignment
      • Have a user submit some work to it
      • Confirm that no entry appears in the mdl_assign_grades table for it
      • Lock the assignment to prevent changes
      • Confirm that an entry does now appear in the mdl_assign_grades table for it
      • Confirm that the 'mailed' flag is currently 0
      • Run cron
      • Bug: The feedbackavail notification was sent by cron and the mailed flag set to 1 despite no feedback being available.

        Activity

        Hide
        Andrew Nicols added a comment -

        Increasing priority to Major as this leads to incorrect information output to students.

        Show
        Andrew Nicols added a comment - Increasing priority to Major as this leads to incorrect information output to students.
        Hide
        Damyon Wiese added a comment -

        Thanks Andrew, I'll take a look at this.

        Show
        Damyon Wiese added a comment - Thanks Andrew, I'll take a look at this.
        Hide
        Damyon Wiese added a comment -

        I've added a patch (just for master for now). The side effect of this patch is that a student will receive a notification each time their grades or feedback are updated.

        Thinking about it - I don't think that is what people will want (They currently get an email the first time grades/feedback is added).

        I'll do a new patch tomorrow.

        Show
        Damyon Wiese added a comment - I've added a patch (just for master for now). The side effect of this patch is that a student will receive a notification each time their grades or feedback are updated. Thinking about it - I don't think that is what people will want (They currently get an email the first time grades/feedback is added). I'll do a new patch tomorrow.
        Hide
        Damyon Wiese added a comment -

        Note - this patch means we are now storing 0, 1 or 2 in a column defined as integer length 1. This still fits without changing the database.

        Show
        Damyon Wiese added a comment - Note - this patch means we are now storing 0, 1 or 2 in a column defined as integer length 1. This still fits without changing the database.
        Hide
        Damyon Wiese added a comment -

        Notes on integer length 1 column type.

        MySQL - tinyint -128 to +127
        Postgres - smallint -32768 to +32767
        Oracle - NUMBER(1) -9 to 9
        MSSQL - smallint -32768 to +32767

        We are storing 0, 1 or 2 so we are fine (I was mainly checking to make sure this was not treated as a bit)

        Show
        Damyon Wiese added a comment - Notes on integer length 1 column type. MySQL - tinyint -128 to +127 Postgres - smallint -32768 to +32767 Oracle - NUMBER(1) -9 to 9 MSSQL - smallint -32768 to +32767 We are storing 0, 1 or 2 so we are fine (I was mainly checking to make sure this was not treated as a bit)
        Hide
        Dan Poltawski added a comment -

        Hi Damyon,

        This is conflicting on master. Could you fix up the conflicts?

        Thanks

        Show
        Dan Poltawski added a comment - Hi Damyon, This is conflicting on master. Could you fix up the conflicts? Thanks
        Hide
        Damyon Wiese added a comment -

        Rebased for 23 and master.

        Show
        Damyon Wiese added a comment - Rebased for 23 and master.
        Hide
        Dan Poltawski added a comment -

        On first glance, it seems a bit weird to me to be modifying get_user_grade() to accept an argument for sending a mail. It makes total sense to have an argument for this when modifying grades, but not when getting them.

        Show
        Dan Poltawski added a comment - On first glance, it seems a bit weird to me to be modifying get_user_grade() to accept an argument for sending a mail. It makes total sense to have an argument for this when modifying grades, but not when getting them.
        Hide
        Dan Poltawski added a comment -

        I haven't had enough time to look at this properly, so delaying till next week.

        Show
        Dan Poltawski added a comment - I haven't had enough time to look at this properly, so delaying till next week.
        Hide
        Dan Poltawski added a comment -

        GRR, I didn't get this one solved in time again. I want to get another integrator to look a this and see their thoughts.

        Show
        Dan Poltawski added a comment - GRR, I didn't get this one solved in time again. I want to get another integrator to look a this and see their thoughts.
        Hide
        Dan Poltawski added a comment -

        I want another integrators opinion on this one, so stopping on this one for this week.

        Show
        Dan Poltawski added a comment - I want another integrators opinion on this one, so stopping on this one for this week.
        Hide
        Damyon Wiese added a comment -

        Thanks Dan - I changed the patch based on the comments about not liking the $sendmail parameter for upgrade_grade - there is now a separate function "notify_grade_update" which needs to be called when a change is made that should trigger an email.

        I only updated the master branch so far - I'll push the other branches in a second.

        Show
        Damyon Wiese added a comment - Thanks Dan - I changed the patch based on the comments about not liking the $sendmail parameter for upgrade_grade - there is now a separate function "notify_grade_update" which needs to be called when a change is made that should trigger an email. I only updated the master branch so far - I'll push the other branches in a second.
        Hide
        Damyon Wiese added a comment -

        Updated 24 and 23 branches.

        Show
        Damyon Wiese added a comment - Updated 24 and 23 branches.
        Hide
        Sam Hemelryk added a comment -

        Aha reading through this it looks like it may be best Dan looks over this one as he was familiar with the previous patch.

        I've looked over it myself, personally I am OK with the changes but will leave it up to you Dan.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Aha reading through this it looks like it may be best Dan looks over this one as he was familiar with the previous patch. I've looked over it myself, personally I am OK with the changes but will leave it up to you Dan. Cheers Sam
        Hide
        Dan Poltawski added a comment -

        Thanks Damyon, this looks way better this way and i've integrated this now.

        Show
        Dan Poltawski added a comment - Thanks Damyon, this looks way better this way and i've integrated this now.
        Hide
        Jason Fowler added a comment -

        Works perfectly, thanks Damyon

        Show
        Jason Fowler added a comment - Works perfectly, thanks Damyon
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And your fantastic code has met core, hope they become good friends for a long period.

        Closing, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: