Moodle
  1. Moodle
  2. MDL-29327

mess grade categories and items table when categories' grade type is set as none

    Details

    • Testing Instructions:
      Hide

      You'll need a course with a handful of activities.

      This test has 2 parts:

      Part 1 - testing the scenario originally reported.

      1. goto gradebook.
      2. click tab 'categories and items'.
      3. add a category and set its 'Grade type' to 'value'.
      4. move an activity into the category
      5. change the category grade type to 'None'

      Make sure the table does not appear broken as in the screenshot.

      Part 2 - double checking that nothing was broken by the fix.

      You'll need some activities, one category with grade type set to none and at least one other category.

      Using the arrow move grade item icons to shift around the activities. Try some combinations. empty categories, an activity in each category, a category within another category.

      Make sure the table does not appear broken as in the screenshot.

      Be aware that bulk moving grade items is known to be broken and will be fixed in MDL-35058.

      Show
      You'll need a course with a handful of activities. This test has 2 parts: Part 1 - testing the scenario originally reported. 1. goto gradebook. 2. click tab 'categories and items'. 3. add a category and set its 'Grade type' to 'value'. 4. move an activity into the category 5. change the category grade type to 'None' Make sure the table does not appear broken as in the screenshot. Part 2 - double checking that nothing was broken by the fix. You'll need some activities, one category with grade type set to none and at least one other category. Using the arrow move grade item icons to shift around the activities. Try some combinations. empty categories, an activity in each category, a category within another category. Make sure the table does not appear broken as in the screenshot. Be aware that bulk moving grade items is known to be broken and will be fixed in MDL-35058 .
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-29327_none

      Description

      See the attached image

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Do you have any hidden grade items in your list of grades?

            Show
            Michael de Raadt added a comment - Do you have any hidden grade items in your list of grades?
            Hide
            Sunner Sun added a comment -

            No hidden items. And I also tested with hidden item, the problem is still there.

            Show
            Sunner Sun added a comment - No hidden items. And I also tested with hidden item, the problem is still there.
            Hide
            Minh-Tam Nguyen added a comment -

            I have been able to reproduce this on qa.moodle.net today 22/02/2012.
            If the grade type for the Course grade category is set to "None" the Course Total column is not displayed as designed, however in the Categories and Items screen the last row displayed is indented incorrectly due to the "rowspan" arguent of the header row counting the number of grade items incorrectly.

            Show
            Minh-Tam Nguyen added a comment - I have been able to reproduce this on qa.moodle.net today 22/02/2012. If the grade type for the Course grade category is set to "None" the Course Total column is not displayed as designed, however in the Categories and Items screen the last row displayed is indented incorrectly due to the "rowspan" arguent of the header row counting the number of grade items incorrectly.
            Hide
            Dan Poltawski added a comment -

            I can reproduce this.

            Show
            Dan Poltawski added a comment - I can reproduce this.
            Hide
            Dan Poltawski added a comment -
            1. Create an empty course
            2. Go to Settings -> Grades > Categories and items > Full View
            3. Add a category
            4. Set the grade type to 'none'
            5. See screenshot'd table
            Show
            Dan Poltawski added a comment - Create an empty course Go to Settings -> Grades > Categories and items > Full View Add a category Set the grade type to 'none' See screenshot'd table
            Hide
            Andrew Davis added a comment -

            I had a duplicate report of this issue assigned to the next stable sprint. As this bug report is older and has more votes I have closed that issue (MDL-34646) and will focus on this one.

            Show
            Andrew Davis added a comment - I had a duplicate report of this issue assigned to the next stable sprint. As this bug report is older and has more votes I have closed that issue ( MDL-34646 ) and will focus on this one.
            Hide
            Andrew Davis added a comment -

            Just waiting for MDL-34883 to be integrated before starting on this.

            Show
            Andrew Davis added a comment - Just waiting for MDL-34883 to be integrated before starting on this.
            Hide
            Sam Chaffee added a comment - - edited

            Hi Andrew, I've been taking a peek at code that I think may causing this bug and I think it may be regression from MDL-23543.

            I took out the following from lib/grade/grade_category.php's grade_category::_fetch_course_tree_recursion() and it fixes the problem addressed in this ticket as well as a related issue I am working on in a custom grader report:

            if (isset($category_array['object']->gradetype) && $category_array['object']->gradetype==GRADE_TYPE_NONE) {
                return null;
            }
            

            I am wondering how we might change this check (aside from removing it since that would then re-break MDL-23543) so that categories with GRADE_TYPE_NONE still show up in the grader report.

            EDIT: Possibly something like this:

            if ($category_array['type'] != 'categoryitem' && isset($category_array['object']->gradetype) && $category_array['object']->gradetype==GRADE_TYPE_NONE) {
                return null;
            }
            

            Thanks,

            Sam

            Show
            Sam Chaffee added a comment - - edited Hi Andrew, I've been taking a peek at code that I think may causing this bug and I think it may be regression from MDL-23543 . I took out the following from lib/grade/grade_category.php's grade_category::_fetch_course_tree_recursion() and it fixes the problem addressed in this ticket as well as a related issue I am working on in a custom grader report: if (isset($category_array['object']->gradetype) && $category_array['object']->gradetype==GRADE_TYPE_NONE) { return null; } I am wondering how we might change this check (aside from removing it since that would then re-break MDL-23543 ) so that categories with GRADE_TYPE_NONE still show up in the grader report. EDIT: Possibly something like this: if ($category_array['type'] != 'categoryitem' && isset($category_array['object']->gradetype) && $category_array['object']->gradetype==GRADE_TYPE_NONE) { return null; } Thanks, Sam
            Hide
            Andrew Davis added a comment - - edited

            Ive added a potential fix as well as expanding the testing instructions. All I've done in the fix is remove some code. It's several years old and its anyone's guess as to what the original intention was. I could be wrong but all it appears to do now is to cause categories with grade type "none" to have a rowspan of 0 which breaks the table.

            Show
            Andrew Davis added a comment - - edited Ive added a potential fix as well as expanding the testing instructions. All I've done in the fix is remove some code. It's several years old and its anyone's guess as to what the original intention was. I could be wrong but all it appears to do now is to cause categories with grade type "none" to have a rowspan of 0 which breaks the table.
            Hide
            Damyon Wiese added a comment -

            Thanks Andrew, this change looks good. I amended the test instructions as I found a way to reliably reproduce the error.

            Can you provide backported branches? Then it should be ready to send to integration.

            [Y] Syntax
            [-] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [Y] Testing
            [-] Security
            [-] Documentation
            [N] Git - Just needs rebasing.
            [Y] Sanity check

            Show
            Damyon Wiese added a comment - Thanks Andrew, this change looks good. I amended the test instructions as I found a way to reliably reproduce the error. Can you provide backported branches? Then it should be ready to send to integration. [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [N] Git - Just needs rebasing. [Y] Sanity check
            Hide
            Andrew Davis added a comment -

            Ive added branches for 2.4 and 2.3. Submitting for integration.

            Show
            Andrew Davis added a comment - Ive added branches for 2.4 and 2.3. Submitting for integration.
            Hide
            Sam Hemelryk added a comment -

            Thanks Andrew, this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now.
            Hide
            David Monllaó added a comment -

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

            Show
            David Monllaó added a comment - It passes, all works as expected, tested in master and 23
            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:
                13 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: