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

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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: