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 2.4 Branch:
      MDL-29327_none_24
    • Pull Master Branch:
      MDL-29327_none
    • Rank:
      18861

      Description

      See the attached image

        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: