Moodle
  1. Moodle
  2. MDL-29392

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      19097

      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.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

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

          Show
          Tim Hunt added a comment - This fix works for us. Mahmoud's work, but I transferred it from OU moodle.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

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

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

          Cheers
          Sam

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

          Works Great
          Thanks for fixing this Tim

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

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

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

          Ciao

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