Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37973

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

    Details

    • Testing Instructions:
      Hide
      1. Create an assign with grade set to 100
      2. Create an assign with grade set to the standard scale
      3. Create an assign with grade set to No Grade
      4. In the gradebook simple view, verify that max grade for the 3 assignments are listed (in order) as "100.00", "Mostly connected knowing (3)", and " - ".
      5. Change the category containing the 3 assignments to Sum of Grades.
      6. In the gradebook simple view, verify that max grade for the 3 assignments are listed (in order) as "100.00", "3.00", and " - ".
      Show
      Create an assign with grade set to 100 Create an assign with grade set to the standard scale Create an assign with grade set to No Grade In the gradebook simple view, verify that max grade for the 3 assignments are listed (in order) as "100.00", "Mostly connected knowing (3)", and " - ". Change the category containing the 3 assignments to Sum of Grades. In the gradebook simple view, verify that max grade for the 3 assignments are listed (in order) as "100.00", "3.00", and " - ".
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            damyon 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 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
            kiddlisa 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
            kiddlisa 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 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 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
            emerrill Eric Merrill added a comment -

            Patch inbound - writing unit tests.

            Show
            emerrill Eric Merrill added a comment - Patch inbound - writing unit tests.
            Hide
            emerrill 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
            emerrill 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
            emerrill 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
            emerrill 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
            emerrill 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
            emerrill 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 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 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
            emerrill 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
            emerrill 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 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 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
            andyjdavis Andrew Davis added a comment -

            +1 from me too. Submitting for integration.

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

            Thanks Eric - this has been integrated now

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

            Passes on master, 25 and 24 - thanks!

            Show
            poltawski Dan Poltawski added a comment - Passes on master, 25 and 24 - thanks!
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  11/Nov/13