Moodle

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

Details

  • Type: Bug Bug
  • Status: Closed 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:
  • Pull Master Diff URL:

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

is duplicated by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-23143 Allow default grade report to be configurable

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-24326 Gradebook plugins not modular -- user report hardcoded

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: