Moodle
  1. Moodle
  2. MDL-37973

"No grade" assignment output grade in Categories and items report

    Details

    • Rank:
      47748

      Description

      If you create an assignment with "No Grade", in the "Categories and items" report ( Grade administration ► Categories and items), it output 100 in Max grade column when it should have been 0 (or empty). Note it didn't add the value in the Course Total summary, which is good, but it shouldn't output 100 in the first place. It happen in both Simple and Full view.

        Issue Links

          Activity

          Hide
          Damyon Wiese added a comment -

          Just triaging this - haven't checked the code - not sure if the activity should not be pushing to the gradebook or the gradebook should be hiding activities with no grade.

          Show
          Damyon Wiese added a comment - Just triaging this - haven't checked the code - not sure if the activity should not be pushing to the gradebook or the gradebook should be hiding activities with no grade.
          Hide
          Lisa Kidder added a comment -

          We discovered this first with the Blackboard Collaborate plugin that is based on the code for the assignment. The user report displays a dash and it is not calculated in the course total, but it is very confusing to a teacher trying to set their aggregation methods in the categories & items tab.

          Show
          Lisa Kidder added a comment - We discovered this first with the Blackboard Collaborate plugin that is based on the code for the assignment. The user report displays a dash and it is not calculated in the course total, but it is very confusing to a teacher trying to set their aggregation methods in the categories & items tab.
          Hide
          Damyon Wiese added a comment -

          This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          Damyon Wiese added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
          Hide
          Eric Merrill added a comment -

          Patch inbound - writing unit tests.

          Show
          Eric Merrill added a comment - Patch inbound - writing unit tests.
          Hide
          Eric Merrill added a comment - - edited

          The basic problem is that when the grade value is set to 0, grademax isn't update in the grade item (I also found a couple other places where the 0 case wasn't handled right).

          This means that there are assign's that are leftover from this bug with out of sync with their grade items. Should I write a upgrade to fix them (but that would mean incrementing version numbers), or just leave it be, and if someone want to fix theirs, they can just go in and edit settings and hit save? I'm open to other suggestions.

          So I've attached a patch. The 2.4 version doesn't have unit tests, because there are basically no mod_assign tests, and none of the support functions.

          Damyon, could you review this and give me your 2 cents?

          Show
          Eric Merrill added a comment - - edited The basic problem is that when the grade value is set to 0, grademax isn't update in the grade item (I also found a couple other places where the 0 case wasn't handled right). This means that there are assign's that are leftover from this bug with out of sync with their grade items. Should I write a upgrade to fix them (but that would mean incrementing version numbers), or just leave it be, and if someone want to fix theirs, they can just go in and edit settings and hit save? I'm open to other suggestions. So I've attached a patch. The 2.4 version doesn't have unit tests, because there are basically no mod_assign tests, and none of the support functions. Damyon, could you review this and give me your 2 cents?
          Hide
          Eric Merrill added a comment -

          I would note that I treated this as a mod_assign bug, but there is probably an equal case to be made that it is a gradebook bug, in that it shouldn't show the grademax value anywhere if the grade type is GRADE_TYPE_TEXT.

          The side benefit of redoing it through the gradebook would be that the patch would work retroactively. Hrm.

          Show
          Eric Merrill added a comment - I would note that I treated this as a mod_assign bug, but there is probably an equal case to be made that it is a gradebook bug, in that it shouldn't show the grademax value anywhere if the grade type is GRADE_TYPE_TEXT. The side benefit of redoing it through the gradebook would be that the patch would work retroactively. Hrm.
          Hide
          Eric Merrill added a comment -

          Ok (sorry for the flood of updates), here is a simple patch for the gradebook side of things:
          https://github.com/merrill-oakland/moodle/commit/1d28cacb56360e2b6157264782a60155b4700560

          I guess the real question now is that is the 'proper' way to do it:
          Should the grade item be updated to a grademax of 0 when the assign is set to no grade?
          Is there a reason to not do it?
          Should the gradebook just not show grademax with the grade type is text?
          Should both patches be applied?

          Once I get some feedback, I'll roll whatever the answer is into a clean commit and branches.

          Show
          Eric Merrill added a comment - Ok (sorry for the flood of updates), here is a simple patch for the gradebook side of things: https://github.com/merrill-oakland/moodle/commit/1d28cacb56360e2b6157264782a60155b4700560 I guess the real question now is that is the 'proper' way to do it: Should the grade item be updated to a grademax of 0 when the assign is set to no grade? Is there a reason to not do it? Should the gradebook just not show grademax with the grade type is text? Should both patches be applied? Once I get some feedback, I'll roll whatever the answer is into a clean commit and branches.
          Hide
          Damyon Wiese added a comment -

          Hi Eric,

          I vote for fixing this in display of the report in the gradebook for a few reasons.

          1. It will affect all modules.
          2. Changing the maxgrade that mod_assign reports may cause grades to be recalculated for a course even though nothing has really changed.
          3. The code in assign looks correct (it is not setting any grademax when the type is GRADE_TYPE_TEXT)

          Therefore +1 for your second gradebook only patch.

          +20 if it can have either a unit test or behat test to go with it.

          Thanks!

          Show
          Damyon Wiese added a comment - Hi Eric, I vote for fixing this in display of the report in the gradebook for a few reasons. 1. It will affect all modules. 2. Changing the maxgrade that mod_assign reports may cause grades to be recalculated for a course even though nothing has really changed. 3. The code in assign looks correct (it is not setting any grademax when the type is GRADE_TYPE_TEXT) Therefore +1 for your second gradebook only patch. +20 if it can have either a unit test or behat test to go with it. Thanks!
          Hide
          Eric Merrill added a comment -

          I've reworked the patch a bit after reviewing the grade code a little more (namely is still shows the - even if the category is Sum of Grades).

          I also added unit tests in grade/tests/edittreelib_test.php (or grade/tests/edittree_test.php for 2.4 and 2.5). While I was in there, I made some minor change to clean up that file and make it pass code checker.

          Take a look around, and please send for integration if you feel it's ready

          Thanks Damyon!

          Show
          Eric Merrill added a comment - I've reworked the patch a bit after reviewing the grade code a little more (namely is still shows the - even if the category is Sum of Grades). I also added unit tests in grade/tests/edittreelib_test.php (or grade/tests/edittree_test.php for 2.4 and 2.5). While I was in there, I made some minor change to clean up that file and make it pass code checker. Take a look around, and please send for integration if you feel it's ready Thanks Damyon!
          Hide
          Damyon Wiese added a comment -

          Thanks Eric,

          As this is now gradebook - I've added Andrew Davis as a watcher.

          This gets +1 from me - Andrew can you take a look and push for integration if there are no issues?

          Show
          Damyon Wiese added a comment - Thanks Eric, As this is now gradebook - I've added Andrew Davis as a watcher. This gets +1 from me - Andrew can you take a look and push for integration if there are no issues?
          Hide
          Andrew Davis added a comment -

          +1 from me too. Submitting for integration.

          Show
          Andrew Davis added a comment - +1 from me too. Submitting for integration.
          Hide
          Sam Hemelryk added a comment -

          Thanks Eric - this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Eric - this has been integrated now
          Hide
          Dan Poltawski added a comment -

          Passes on master, 25 and 24 - thanks!

          Show
          Dan Poltawski added a comment - Passes on master, 25 and 24 - thanks!
          Hide
          Dan Poltawski added a comment -

          You did it!

          Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

          Show
          Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: