Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-29392

grade/report/index.php ignores $CFG->grade_profilereport

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      1. To to Admin -> Grades -> User profile report and change that to something else. - that it not easy because standard Moodle only comes with the User report. We have an OU user report here. Let us assume you managed to get an 'XXX user' report from somewhere.
      2. Change the capabilities so that students can access the XXX User report, and cannot access the standard User report.
      3. Log in as a student, and try to go to the grade/report/index.php?id=

      {courseid}

      URL. Verify you are sent to the 'XXX user' report.

      Note that the most recent report you looked at in each course is stored in the session, and that takes priority over the default, which complicates testing.

      Show
      1. To to Admin -> Grades -> User profile report and change that to something else. - that it not easy because standard Moodle only comes with the User report. We have an OU user report here. Let us assume you managed to get an 'XXX user' report from somewhere. 2. Change the capabilities so that students can access the XXX User report, and cannot access the standard User report. 3. Log in as a student, and try to go to the grade/report/index.php?id= {courseid} URL. Verify you are sent to the 'XXX user' report. Note that the most recent report you looked at in each course is stored in the session, and that takes priority over the default, which complicates testing.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      The default grade report for users is set by $CFG->grade_profilereport. grade/report/index.php is a script that redirects users to the most appropriate gradebook report for them. It currently hard-codes 'users' rather than using that config setting.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            This fix works for us. Mahmoud's work, but I transferred it from OU moodle.

            Show
            timhunt Tim Hunt added a comment - This fix works for us. Mahmoud's work, but I transferred it from OU moodle.
            Hide
            timhunt Tim Hunt added a comment -

            Although I have already submitted it for integration, it would be great if you could take a look Andrew.

            Show
            timhunt Tim Hunt added a comment - Although I have already submitted it for integration, it would be great if you could take a look Andrew.
            Hide
            andyjdavis Andrew Davis added a comment -

            Looks good. The only query I have is with the removal of this

            } else if (array_key_exists('user', $reports)) {
               $last = 'user';

            which has been replaced by this

            } else if (array_key_exists($CFG->grade_profilereport, $reports)) {
               $last = $CFG->grade_profilereport;

            If $CFG->grade_profilereport is not set at all will it still default to the user report? It looks like it will fall through to $last = key(reset($reports)); and Im not sure what that will return. Maybe the if clause that explicitly defaults to the user report wants to be retained but should appear after the $CFG->grade_profilereport clause.

            Show
            andyjdavis Andrew Davis added a comment - Looks good. The only query I have is with the removal of this } else if (array_key_exists('user', $reports)) { $last = 'user'; which has been replaced by this } else if (array_key_exists($CFG->grade_profilereport, $reports)) { $last = $CFG->grade_profilereport; If $CFG->grade_profilereport is not set at all will it still default to the user report? It looks like it will fall through to $last = key(reset($reports)); and Im not sure what that will return. Maybe the if clause that explicitly defaults to the user report wants to be retained but should appear after the $CFG->grade_profilereport clause.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi guys,

            I'm failing this because there is a coding error, reset returns the first element of an array so calling key(reset($reports)) is certainly going to lead to notices because you're calling key what ever the first element is.

            In regards to the default I think you are right as well Andrew, I'd think ideally the default should remain the user report although hardcoding that is horrid so perhaps only using user is if it is available. What are your thoughts on that Tim?

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi guys, I'm failing this because there is a coding error, reset returns the first element of an array so calling key(reset($reports)) is certainly going to lead to notices because you're calling key what ever the first element is. In regards to the default I think you are right as well Andrew, I'd think ideally the default should remain the user report although hardcoding that is horrid so perhaps only using user is if it is available. What are your thoughts on that Tim? Cheers Sam
            Hide
            timhunt Tim Hunt added a comment -

            $CFG->grade_profilereport is set to its default value ('user') on install. On the admin screens, this value is set using a select menu of available reports, so there is no way it can be blank. So, no point in keeping the explicit 'user' clause in the if.

            I will fix the coding error. Sorry.

            Show
            timhunt Tim Hunt added a comment - $CFG->grade_profilereport is set to its default value ('user') on install. On the admin screens, this value is set using a select menu of available reports, so there is no way it can be blank. So, no point in keeping the explicit 'user' clause in the if. I will fix the coding error. Sorry.
            Hide
            timhunt Tim Hunt added a comment -

            All three branches amended. Note that the key(reset($reports)); problem has been there since forever. We did not change it in our patch. Still, fixed now.

            Show
            timhunt Tim Hunt added a comment - All three branches amended. Note that the key(reset($reports)); problem has been there since forever. We did not change it in our patch. Still, fixed now.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (oops, apologizes, I got this by mistake, sending back to Sam)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (oops, apologizes, I got this by mistake, sending back to Sam)
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks for clearing that all up Tim, this has been integrated now

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks for clearing that all up Tim, this has been integrated now Cheers Sam
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Works Great
            Thanks for fixing this Tim

            FYI:
            Tested by adding a dummy grade report, replicating user report.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Works Great Thanks for fixing this Tim FYI: Tested by adding a dummy grade report, replicating user report.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Oct/11