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
    • Rank:
      15522

      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.

        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 Škoda added a comment -

        committed, thanks

        Show
        Petr Škoda added a comment - committed, thanks

          People

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

            Dates

            • Created:
              Updated:
              Resolved: