Moodle
  1. Moodle
  2. MDL-25769

some parts of gradebook dont use enrolment tables and they should

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Gradebook
    • Labels:
      None
    • Database:
      Any
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      load_users() in grade/report/grader/lib.php uses role assignments but doesn't use the enrollment tables to determine who should be displayed on the grader report. See calculate_averages() in grade/report/user/lib.php committed as part of MDL-20617 for an example.

      Grep the whole codebase looking for other instances of "gradebookroles" and update them all to use the enrollment tables in addition to the gradeable roles.

        Gliffy Diagrams

          Activity

          Hide
          Andrew Davis added a comment -

          Fixing this as part of MDL-25769 after all

          Show
          Andrew Davis added a comment - Fixing this as part of MDL-25769 after all
          Hide
          Andrew Davis added a comment - - edited

          After much more further discussion I'm reopening and fixing this as a separate item.

          This isnt close to complete but you can see it progress by going to https://github.com/andyjdavis/moodle/compare/master...MDL-25769_enrollment

          Show
          Andrew Davis added a comment - - edited After much more further discussion I'm reopening and fixing this as a separate item. This isnt close to complete but you can see it progress by going to https://github.com/andyjdavis/moodle/compare/master...MDL-25769_enrollment
          Hide
          Andrew Davis added a comment -

          Think Ive fixed them all.

          repo: git://github.com/andyjdavis/moodle.git
          branch: MDL-25769_enrollment
          diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25769_enrollment

          Raised MDL-25879 while doing this. Looks like theres some redundant code.

          Show
          Andrew Davis added a comment - Think Ive fixed them all. repo: git://github.com/andyjdavis/moodle.git branch: MDL-25769 _enrollment diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25769_enrollment Raised MDL-25879 while doing this. Looks like theres some redundant code.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Just looking through this now, the following are the things I've spotted so far:

          1. grade/import/lib.php
            • ln 170: You use $this out of scope.
            • ln 178: White space (not added by you)
            • ln 186: $params is forgotten about, but looks like its still required "WHERE giv.importcode = ?"
          2. grade/report/grader/lib.php
            • ln 1314: $params already contains courseid; I don't think this line is needed any more.

          Grader report tested fine, I'm guessing the import will fail because of the above so I didn't test that.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Just looking through this now, the following are the things I've spotted so far: grade/import/lib.php ln 170: You use $this out of scope. ln 178: White space (not added by you) ln 186: $params is forgotten about, but looks like its still required "WHERE giv.importcode = ?" grade/report/grader/lib.php ln 1314: $params already contains courseid; I don't think this line is needed any more. Grader report tested fine, I'm guessing the import will fail because of the above so I didn't test that. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Thanks for you keen eyes Sam. I've fixed that stuff although Im unable to test the changes in grade import. I've raised MDL-25887 as something is broken in grade export/import. Note that I was able to reproduce the import errors in both this branch and another branch so it isn't a problem introduced by these changes.

          Show
          Andrew Davis added a comment - Thanks for you keen eyes Sam. I've fixed that stuff although Im unable to test the changes in grade import. I've raised MDL-25887 as something is broken in grade export/import. Note that I was able to reproduce the import errors in both this branch and another branch so it isn't a problem introduced by these changes.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Thanks for making the requested changes, a couple of things though:

          1. You still have $this->context being used in grade/import/lib.php line 171
          2. Given that the database query is using named params its probably best to change giv.importcode = ? to giv.importcode = :importcode and then refactor $params as such.

          Rest looks fine, pitty we can't test the grade import code however.
          I wonder whether that area of the patch should be split out either against the issue you've created to fix the import, or to a separate issue? You may want to check with Petr and see what he thinks.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Thanks for making the requested changes, a couple of things though: You still have $this->context being used in grade/import/lib.php line 171 Given that the database query is using named params its probably best to change giv.importcode = ? to giv.importcode = :importcode and then refactor $params as such. Rest looks fine, pitty we can't test the grade import code however. I wonder whether that area of the patch should be split out either against the issue you've created to fix the import, or to a separate issue? You may want to check with Petr and see what he thinks. Cheers Sam
          Hide
          Andrew Davis added a comment - - edited

          Thats weird. My reply to this seems to have disappeared.

          Anyhow, Ive fixed those 2 issues. Ive also raised submitted a fix for MDL-25887. I'm not keen to spend anymore time shuffling changes between branches than absolutely necessary.

          Show
          Andrew Davis added a comment - - edited Thats weird. My reply to this seems to have disappeared. Anyhow, Ive fixed those 2 issues. Ive also raised submitted a fix for MDL-25887 . I'm not keen to spend anymore time shuffling changes between branches than absolutely necessary.
          Hide
          Andrew Davis added a comment -

          Submitting for consideration by the integrators. PULL-102

          Show
          Andrew Davis added a comment - Submitting for consideration by the integrators. PULL-102
          Hide
          Petr Skoda added a comment -

          committed, thanks

          Show
          Petr Skoda added a comment - committed, thanks

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: