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

              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 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: