Uploaded image for project: '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
    • Status: Closed
    • Priority: 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 Master Branch:
      MDL-36667-master

      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.

        Gliffy Diagrams

          Activity

          Hide
          dobedobedoh Andrew Nicols added a comment -

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

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

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

          Show
          damyon Damyon Wiese added a comment - Thanks Andrew, I'll take a look at this.
          Hide
          damyon 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 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 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 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 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 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
          poltawski Dan Poltawski added a comment -

          Hi Damyon,

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

          Thanks

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

          Rebased for 23 and master.

          Show
          damyon Damyon Wiese added a comment - Rebased for 23 and master.
          Hide
          poltawski 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
          poltawski 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
          poltawski Dan Poltawski added a comment -

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

          Show
          poltawski Dan Poltawski added a comment - I haven't had enough time to look at this properly, so delaying till next week.
          Hide
          poltawski 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
          poltawski 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
          poltawski Dan Poltawski added a comment -

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

          Show
          poltawski Dan Poltawski added a comment - I want another integrators opinion on this one, so stopping on this one for this week.
          Hide
          damyon 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 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 Damyon Wiese added a comment -

          Updated 24 and 23 branches.

          Show
          damyon Damyon Wiese added a comment - Updated 24 and 23 branches.
          Hide
          samhemelryk 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
          samhemelryk 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
          poltawski Dan Poltawski added a comment -

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

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

          Works perfectly, thanks Damyon

          Show
          phalacee Jason Fowler added a comment - Works perfectly, thanks Damyon
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                14/Jan/13