Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: Administration
    • Labels:
    • Rank:
      18564

      Description

      The problem with current reports is that they are spread over several areas (course, system, user) and are not context aware.

      IMHO we should have only one /report/ directory where all plugins live, the navigation would simply ask all report plugins if they want to add any links in the given page context. Some plugins could work in specific contexts only, others might work everywhere.

      See http://docs.moodle.org/dev/General_report_plugins for more details

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Sounds good to me.

          Show
          Michael de Raadt added a comment - Sounds good to me.
          Hide
          Petr Škoda added a comment -

          Sending for peer review, the last missing bit should be the page types for blocks, I will look at that tomorrow...

          Show
          Petr Škoda added a comment - Sending for peer review, the last missing bit should be the page types for blocks, I will look at that tomorrow...
          Show
          Martin Dougiamas added a comment - - edited Is this going to break backward compatibility with old reports? Was it really truly necessary? Admin: http://moodle.org/mod/data/view.php?d=13&perpage=20&search=&sort=0&order=DESC&advanced=0&filter=1&advanced=1&f_44=&f_45=&f_46=Admin+Report&f_47=&f_48=&f_49=&f_51=&f_96=&f_97=&f_98=&f_99=&f_100=&f_101=&f_195=&f_213=&f_214=&f_215=&f_216=&u_fn=&u_ln= Course: http://moodle.org/mod/data/view.php?d=13&perpage=20&search=Admin+Report&sort=0&order=DESC&advanced=0&filter=1&advanced=1&f_44=&f_45=&f_46=Course+Report&f_47=&f_48=&f_49=&f_51=&f_96=&f_97=&f_98=&f_99=&f_100=&f_101=&f_195=&f_213=&f_214=&f_215=&f_216=&u_fn=&u_ln= What is the upgrade procedure for old reports?
          Hide
          Petr Škoda added a comment - - edited

          1/ upgrade procedure is described in /report/upgrade.txt
          2/ admin report migration is simple - you move and and update the url links, alternatively they may be migrated to admin/tool/ plugin type
          3/ migration of course reports is a bit more complex, but it is optional for now

          Benefits:

          • user reports are not hardcoded any more - devs may finally create new user reports without hacking core
          • we can easily add reports for course categories (one we decide where the reports should be, the category navigation is problematic at the moment)
          • access control fixes and improvements - each user report now decides independently
          • you can delete reports from your installation now - please note there is no way to disable them at the moment; previously you could not even delete them
          • no future incompatible API changes - everything is solved via plugin callbacks (settings, navigation, pagetypes, etc.)
          • full frankenstyle compliance
          • we can finally clean up lang packs
          Show
          Petr Škoda added a comment - - edited 1/ upgrade procedure is described in /report/upgrade.txt 2/ admin report migration is simple - you move and and update the url links, alternatively they may be migrated to admin/tool/ plugin type 3/ migration of course reports is a bit more complex, but it is optional for now Benefits: user reports are not hardcoded any more - devs may finally create new user reports without hacking core we can easily add reports for course categories (one we decide where the reports should be, the category navigation is problematic at the moment) access control fixes and improvements - each user report now decides independently you can delete reports from your installation now - please note there is no way to disable them at the moment; previously you could not even delete them no future incompatible API changes - everything is solved via plugin callbacks (settings, navigation, pagetypes, etc.) full frankenstyle compliance we can finally clean up lang packs
          Hide
          Sam Hemelryk added a comment -

          Hi Petr,

          The changes look nearly 100% spot on.
          I have a couple of questions however all relating to absolutely trivial things that I've noticed while looking through these changes.

          • course/user.php now only displays the grade profile report… do we still need course/user.php or could re redirect to something else here as well?
          • In outputrenderers.php line 510 first checks log/index.php exists then checks the log view cap… this is really hardcoded stuff - do we still want that log link there? Personally looking over at it myself I think we could be add a help icon here instead instructing people to look at the log report or talk to there site administrator.. same for every user.
          • Do we have in mind to one day drop support for the coursereport plugin type altogether?
          • configlog/settings.php double up of MOODLE_INTERNAL
          • Functions in course/lib.php print_log, print_log_csv, print_log_ods, and print_log_xls, build_logs_array, print_mnet_log are now only used by the log report, should we look to move those to a locallib.php or is there a reason to keep them there?
          • Do we need to require course/lib.php within report/outline/locallib.php
          • Have any docs been created about these changes yet? (If not done no probs I'm just keen to have a read myself)

          The rest looks spot on to me, I'll leave this is in review in progress so that Eloy has a chance to look at it if he likes and then I'll look to integrate this tomorrow.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr, The changes look nearly 100% spot on. I have a couple of questions however all relating to absolutely trivial things that I've noticed while looking through these changes. course/user.php now only displays the grade profile report… do we still need course/user.php or could re redirect to something else here as well? In outputrenderers.php line 510 first checks log/index.php exists then checks the log view cap… this is really hardcoded stuff - do we still want that log link there? Personally looking over at it myself I think we could be add a help icon here instead instructing people to look at the log report or talk to there site administrator.. same for every user. Do we have in mind to one day drop support for the coursereport plugin type altogether? configlog/settings.php double up of MOODLE_INTERNAL Functions in course/lib.php print_log, print_log_csv, print_log_ods, and print_log_xls, build_logs_array, print_mnet_log are now only used by the log report, should we look to move those to a locallib.php or is there a reason to keep them there? Do we need to require course/lib.php within report/outline/locallib.php Have any docs been created about these changes yet? (If not done no probs I'm just keen to have a read myself) The rest looks spot on to me, I'll leave this is in review in progress so that Eloy has a chance to look at it if he likes and then I'll look to integrate this tomorrow. Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          +1 from me but we really do need some docs about reports before 2.2 release.

          Show
          Martin Dougiamas added a comment - +1 from me but we really do need some docs about reports before 2.2 release.
          Hide
          Martin Dougiamas added a comment -

          course/user.php now only shows grade report? How has the UI changed for user reports then?

          Show
          Martin Dougiamas added a comment - course/user.php now only shows grade report? How has the UI changed for user reports then?
          Hide
          Petr Škoda added a comment -
          • course/user.php - yeah, I did not know what to do with that - we could add support for gradebook reports which would mean to remove the current setting from gradebook and use something else because you could have multiple reports there, or create a new /report/grade/ that just redirects - I do not want to make any gradebook related decisions myself
          • yes, I do not like hardcoding either but I suppose we should solve this later when we work on general logging support (there is a separate issue for that)
          • my +1 to drop course/report in 2.3 - I was not sure if it should be dropped now or later, but Martin's questions persuaded me that it might be better to do it later
          • yes, the log stuff in course/lib.php could be improved - again I thin it should be done together with major log cleanup
          • the course/lib.php in outline - not sure, in any case locallib.php can include whatever they want because it is used only when you view the actual report, I concentrated on moving stuff away from lib.php; this can be improved later once somebody starts improving this report
          • no docs changes yet, I added necessary steps to upgrade.txt's; I worked on other things because I did not know if this lands in 2.2dev - tell me when to start writing docs
          • I have added one more commit doing the page type migration in the block_instances table, please pull it too

          thanks

          Show
          Petr Škoda added a comment - course/user.php - yeah, I did not know what to do with that - we could add support for gradebook reports which would mean to remove the current setting from gradebook and use something else because you could have multiple reports there, or create a new /report/grade/ that just redirects - I do not want to make any gradebook related decisions myself yes, I do not like hardcoding either but I suppose we should solve this later when we work on general logging support (there is a separate issue for that) my +1 to drop course/report in 2.3 - I was not sure if it should be dropped now or later, but Martin's questions persuaded me that it might be better to do it later yes, the log stuff in course/lib.php could be improved - again I thin it should be done together with major log cleanup the course/lib.php in outline - not sure, in any case locallib.php can include whatever they want because it is used only when you view the actual report, I concentrated on moving stuff away from lib.php; this can be improved later once somebody starts improving this report no docs changes yet, I added necessary steps to upgrade.txt's; I worked on other things because I did not know if this lands in 2.2dev - tell me when to start writing docs I have added one more commit doing the page type migration in the block_instances table, please pull it too thanks
          Hide
          Petr Škoda added a comment -

          to Martin: the course/user.php was linked from navigation only, the reports inject own links there now via callback, the course/user.php contains redirection logic in case somebody copy/pasted the links somewhere.

          Show
          Petr Škoda added a comment - to Martin: the course/user.php was linked from navigation only, the reports inject own links there now via callback, the course/user.php contains redirection logic in case somebody copy/pasted the links somewhere.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          My +0.95, it looks great (just overviewed it, sorry, no way to go deeper). Great (and huge) work Petr. I agree this will need some documentation (for devs).

          Just one comment for the integrator: I've integrated MDL-29803 some minutes ago, introducing one change in course/report/log/lib.php (http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=c1733742c6594e0512ec19f169b0b73b80ecbcfd). So, surely you will get conflict when merging this (easily fixable). Of course the fix should be reapplied (on merge or later commit). FYC.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - My +0.95, it looks great (just overviewed it, sorry, no way to go deeper). Great (and huge) work Petr. I agree this will need some documentation (for devs). Just one comment for the integrator: I've integrated MDL-29803 some minutes ago, introducing one change in course/report/log/lib.php ( http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=c1733742c6594e0512ec19f169b0b73b80ecbcfd ). So, surely you will get conflict when merging this (easily fixable). Of course the fix should be reapplied (on merge or later commit). FYC. Ciao
          Hide
          Sam Hemelryk added a comment -

          Just noting that during merge I had to resolve conflicts on the above noted issue, and on MDL-29812.

          Show
          Sam Hemelryk added a comment - Just noting that during merge I had to resolve conflicts on the above noted issue, and on MDL-29812 .
          Hide
          Sam Hemelryk added a comment -

          Hoorah this has been integrated now!

          I'll review this issue again this afternoon and create following issues and comment on existing issues where required.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hoorah this has been integrated now! I'll review this issue again this afternoon and create following issues and comment on existing issues where required. Cheers Sam
          Hide
          Andrew Davis added a comment -

          the testing instructions are rather vague. I've checked the following reports. Those with no comment worked as expected.

          course reports:
          logs
          live logs
          activity report
          course participation (the "Look back" drop down never has anything but "Choose.." in it. Not sure if this is because of this issue)
          statistics (gives an error "Sorry, there is no available data to display". Turning ON enablestats in advanced features causes the statistics course report menu item to disappear. Turning OFF enablestats causes the menu to appear. This seems backwards. Is this because of this issue?)

          site reports:
          logs
          live logs
          activity report (never seems to show any data. /report/outline/index.php?id=1 I added a label to the front page, viewed it but nothing on the activity report)
          course participation (a course participation report at Home > Site pages > Reports > Course participation seems odd. "Activity module" never has anything but "Choose..." in it. I cant get the report to ever show any data.
          statistics (has the same problems as the course statistics report)

          Show
          Andrew Davis added a comment - the testing instructions are rather vague. I've checked the following reports. Those with no comment worked as expected. course reports: logs live logs activity report course participation (the "Look back" drop down never has anything but "Choose.." in it. Not sure if this is because of this issue) statistics (gives an error "Sorry, there is no available data to display". Turning ON enablestats in advanced features causes the statistics course report menu item to disappear. Turning OFF enablestats causes the menu to appear. This seems backwards. Is this because of this issue?) site reports: logs live logs activity report (never seems to show any data. /report/outline/index.php?id=1 I added a label to the front page, viewed it but nothing on the activity report) course participation (a course participation report at Home > Site pages > Reports > Course participation seems odd. "Activity module" never has anything but "Choose..." in it. I cant get the report to ever show any data. statistics (has the same problems as the course statistics report)
          Hide
          Martin Dougiamas added a comment -

          I'm passing these just to let 2.2 beta close.

          Some (most) of the problems look like config/setup problems to me. Some may actually be regressions though.

          Andrew: could you please file new bugs for these issues and link them to this one?

          Show
          Martin Dougiamas added a comment - I'm passing these just to let 2.2 beta close. Some (most) of the problems look like config/setup problems to me. Some may actually be regressions though. Andrew: could you please file new bugs for these issues and link them to this one?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao
          Hide
          Andrew Davis added a comment -

          4 new Moodle 2.2 MDLs created and linked to this issue

          Show
          Andrew Davis added a comment - 4 new Moodle 2.2 MDLs created and linked to this issue

            People

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

              Dates

              • Created:
                Updated:
                Resolved: