Moodle
  1. Moodle
  2. MDL-35336

Process for enabling statistics should be made clearer

    Details

    • Testing Instructions:
      Hide
      1. Go to Settings > Site admin > advanced features
      2. Make sure statistics (enablestats) is disable.
      3. Go to Settings > Site admin > Reports, make sure 'Course overview' is not listed.
      4. Access the report course overview page (eg: http://mymoodletestsite/report/courseoverview/index.php). Make sure the page redirect to search result page that contents the setting for enabling statistics.
      5. Login as Manager, make sure 'course overview' is not listed (Settings > Site admin > Reports).
      Show
      Go to Settings > Site admin > advanced features Make sure statistics (enablestats) is disable. Go to Settings > Site admin > Reports, make sure 'Course overview' is not listed. Access the report course overview page (eg: http://mymoodletestsite/report/courseoverview/index.php ). Make sure the page redirect to search result page that contents the setting for enabling statistics. Login as Manager, make sure 'course overview' is not listed (Settings > Site admin > Reports).
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-35336_m24
    • Pull Master Branch:
    • Rank:
      44008

      Description

      In order to enable statistics, you have to turn on the Advanced features setting (enablestats). However, when Statistics not enabled, the Statistics settings page still appears at Settings > Site admin > Server > Statistics and the Course overview report appears in Settings > Site admin > Reports > Course overview.

      We should consider hiding these two pages while Statistics is disabled.

      Alternately we should consider making the following changes.

      1. When you visit the Course overview report, while statistics is disabled, you are redirected to the Statistics settings page, you should instead be redirected to the Advanced features page, perhaps through a search for "enablestats".
      2. When you visit the Statistics settings page, while statistics is disabled, you should see a notice that Statistics are disabled and provided with a link to the Advanced features page, perhaps through a search for "enablestats".

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Created patch to fix the link for redirect.

          In order to keep the setting page consistent with the rest, I didn't add any notification to the statistics setting page.

          I could add it easily if it is really needed/require.

          Show
          Rossiani Wijaya added a comment - Created patch to fix the link for redirect. In order to keep the setting page consistent with the rest, I didn't add any notification to the statistics setting page. I could add it easily if it is really needed/require.
          Hide
          Rajesh Taneja added a comment -

          Thanks Rossie,

          Patch looks spot-on, pushing for integration review.
          [y] Syntax
          [y] Output
          [y] Whitespace
          [-] Language
          [-] Databases
          [y] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Show
          Rajesh Taneja added a comment - Thanks Rossie, Patch looks spot-on, pushing for integration review. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Cheers!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Cheers!
          Hide
          Dan Poltawski added a comment -

          Hi Rosie,

          The change is fine, but it doesn't improve the situation that much as we're just again presented with a long list of options. Therefore i'm reopening.

          Why not instead redirect to "admin/search.php?query=enablestats" as Michael suggested?

          thanks
          Dan

          Show
          Dan Poltawski added a comment - Hi Rosie, The change is fine, but it doesn't improve the situation that much as we're just again presented with a long list of options. Therefore i'm reopening. Why not instead redirect to "admin/search.php?query=enablestats" as Michael suggested? thanks Dan
          Hide
          Rossiani Wijaya added a comment -

          Hi Dan,

          Thanks for the feedback.

          I updated the patch to re-direct to admin/search.php?query=enablestats page.

          Re-submitting for integration reivew.

          Show
          Rossiani Wijaya added a comment - Hi Dan, Thanks for the feedback. I updated the patch to re-direct to admin/search.php?query=enablestats page. Re-submitting for integration reivew.
          Hide
          Rajesh Taneja added a comment -

          Hello Dan and Rossie,

          I am not sure if this will work.
          If you are logged in as manager, you can't configure admin settings but can see course overview report.
          So redirecting to a page where user doesn't have permission is probably not helpful.

          We might have to look at this with different roles and there capabilities, before redirecting.

          Show
          Rajesh Taneja added a comment - Hello Dan and Rossie, I am not sure if this will work. If you are logged in as manager, you can't configure admin settings but can see course overview report. So redirecting to a page where user doesn't have permission is probably not helpful. We might have to look at this with different roles and there capabilities, before redirecting.
          Hide
          Dan Poltawski added a comment -

          Good catch, Raj

          Show
          Dan Poltawski added a comment - Good catch, Raj
          Hide
          Rajesh Taneja added a comment -

          Thanks to you Dan, for making me think about roles.
          Few recent issues have made me think how different roles behave and have been facing issues.

          Show
          Rajesh Taneja added a comment - Thanks to you Dan, for making me think about roles. Few recent issues have made me think how different roles behave and have been facing issues.
          Hide
          Rossiani Wijaya added a comment -

          Updating the patch to hide the course overview link when stats is disabled.

          sending for peer review.

          Show
          Rossiani Wijaya added a comment - Updating the patch to hide the course overview link when stats is disabled. sending for peer review.
          Hide
          Rajesh Taneja added a comment -

          Thanks Rossie,

          Patch looks good to me, pushing for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Rossie, Patch looks good to me, pushing for integration review.
          Hide
          Dan Poltawski added a comment -

          Hi Rosie,

          This isn't quite right, it causes problems if the the page isn't added to the admin tree all the time (I made this mistake myself, see the bugfix in MDL-36743).

          The solution is to use the $hidden flag of admin_externalpage which will hide it if necessary

          Show
          Dan Poltawski added a comment - Hi Rosie, This isn't quite right, it causes problems if the the page isn't added to the admin tree all the time (I made this mistake myself, see the bugfix in MDL-36743 ). The solution is to use the $hidden flag of admin_externalpage which will hide it if necessary
          Hide
          Rossiani Wijaya added a comment -

          Hi Dan,

          Thank you for the suggestion.

          I made the changes and re-submitting for integration review.

          Show
          Rossiani Wijaya added a comment - Hi Dan, Thank you for the suggestion. I made the changes and re-submitting for integration review.
          Hide
          Rajesh Taneja added a comment -

          Hello Rossie,

          Not sure, but I think you should use empty($CFG->enablestats), to be on safer side.
          As we have seen $CFG->enablestats might not be set at times.

          Show
          Rajesh Taneja added a comment - Hello Rossie, Not sure, but I think you should use empty($CFG->enablestats), to be on safer side. As we have seen $CFG->enablestats might not be set at times.
          Hide
          Rossiani Wijaya added a comment -

          Arghh... It was right in front of me to check for empty $CFG and yet I still missed it.

          Thank you Raj for catching that.

          Updated the patch.

          Show
          Rossiani Wijaya added a comment - Arghh... It was right in front of me to check for empty $CFG and yet I still missed it. Thank you Raj for catching that. Updated the patch.
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Thanks!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
          Hide
          Rossiani Wijaya added a comment -

          Patch rebased.

          Show
          Rossiani Wijaya added a comment - Patch rebased.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks a lot Rosie!

          Show
          Dan Poltawski added a comment - Integrated, thanks a lot Rosie!
          Hide
          Mark Nelson added a comment -

          Works as expected, passing.

          Show
          Mark Nelson added a comment - Works as expected, passing.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon
          Hide
          Michael de Raadt added a comment -

          I've added a ui_change and docs_required labels to this issue.

          I suspect the docs might need minor changes after this change.

          Show
          Michael de Raadt added a comment - I've added a ui_change and docs_required labels to this issue. I suspect the docs might need minor changes after this change.
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as I have made a note of this in the 2.3 and 2.4 docs http://docs.moodle.org/24/en/Statistics#Enabling_statistics

          Show
          Mary Cooch added a comment - Removing docs_required label as I have made a note of this in the 2.3 and 2.4 docs http://docs.moodle.org/24/en/Statistics#Enabling_statistics

            People

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

              Dates

              • Created:
                Updated:
                Resolved: