Moodle
  1. Moodle
  2. MDL-27256 Improve SCORM quiz reporting
  3. MDL-27523

Design of a pluggable plugin container and a basic report plugin

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.2
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      This shouldn't affect the display to users very much this just duplicates existing functionality but changes the code structure.

      Create a SCORM based on one from the SCORM repo:
      log in as a student and submit some attempts.
      log back in as a teacher and make sure the Reports page operates as expected - try downloading Excel, ODS files, try deleting attempts.

      NOTE: In my review of this code I found previous patches to the SCORM reports pages have broken support for groups/groupings - see MDL-28282 - so don't bother to test this!

      Show
      This shouldn't affect the display to users very much this just duplicates existing functionality but changes the code structure. Create a SCORM based on one from the SCORM repo: log in as a student and submit some attempts. log back in as a teacher and make sure the Reports page operates as expected - try downloading Excel, ODS files, try deleting attempts. NOTE: In my review of this code I found previous patches to the SCORM reports pages have broken support for groups/groupings - see MDL-28282 - so don't bother to test this!
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      master_MDL-27523
    • Rank:
      16935

      Description

      The current reporting feature consists of single file handling the complete reporting.Its better to convert that to a pluggable architecture , allowing developers to develop there own reporting plugins ,which can be easily plugged into the container.
      Thus a plugin container needs to be developed.
      Along with the container , a basic plugin which displays the very basic reporting should also be included with the container by default.

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          Hi Ankit,

          have had a really quick look at this - good start!

          it's missing the code from mod/scorm/db/upgrade.php to create the new table - install.xml is only used for fresh installs.

          the mod.php file isn't really needed - all SCORM reports should use a standard method for display - by calling: /mod/scorm/report.php with the mode set to the report plugin name - like you've done for the basic plugin.

          If we want to allow plugins to override this and use a different url we could add a function to the class that allows a plugin to override the location - but lets leave that out for now.

          in report.php:
          foreach ($reportlist as $reportobj)

          should be:
          foreach ($reportlist as $reportobj) {

          if($mode!=NULL)

          should be:
          if (!empty($mode)) {

          few more comments to come later.

          Show
          Dan Marsden added a comment - Hi Ankit, have had a really quick look at this - good start! it's missing the code from mod/scorm/db/upgrade.php to create the new table - install.xml is only used for fresh installs. the mod.php file isn't really needed - all SCORM reports should use a standard method for display - by calling: /mod/scorm/report.php with the mode set to the report plugin name - like you've done for the basic plugin. If we want to allow plugins to override this and use a different url we could add a function to the class that allows a plugin to override the location - but lets leave that out for now. in report.php: foreach ($reportlist as $reportobj) should be: foreach ($reportlist as $reportobj) { if($mode!=NULL) should be: if (!empty($mode)) { few more comments to come later.
          Hide
          Ankit Agarwal added a comment -

          Hi Dan,
          Mod.php is also responsible for the display of the Anchor text..and also it can allow plugins to be displayed as per their custom capability.For example a plugin can provide its own "capability" and based on that the tab representing that plugin might be visible to some users and invisible to others while scorm/report.php is loaded..So will it be okay to keep the mod.php ? or still its better to remove it?

          I will also look on the other stuff you mentioned and make changes accordingly.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Dan, Mod.php is also responsible for the display of the Anchor text..and also it can allow plugins to be displayed as per their custom capability.For example a plugin can provide its own "capability" and based on that the tab representing that plugin might be visible to some users and invisible to others while scorm/report.php is loaded..So will it be okay to keep the mod.php ? or still its better to remove it? I will also look on the other stuff you mentioned and make changes accordingly. Thanks
          Hide
          Dan Marsden added a comment -

          I thought the capability was going to be managed in the scorm_report table? - like done in quiz_reports - I think it uses the scorm/reports/plugin/install.php to manage that in quiz - see quiz/report/statistics for an example.

          my vote - remove mod.php - thanks!

          Show
          Dan Marsden added a comment - I thought the capability was going to be managed in the scorm_report table? - like done in quiz_reports - I think it uses the scorm/reports/plugin/install.php to manage that in quiz - see quiz/report/statistics for an example. my vote - remove mod.php - thanks!
          Hide
          Dan Marsden added a comment -

          NOTES FOR INTEGRATORS:
          MASTER only please - this should not go in stable branches.
          The basic plugin is basically a copy of the existing report in Moodle - any bugs with the code structure that are present in existing code can be classed as "new" bugs - (feel free to create them if you spot issues)

          I'd like to move the plugin away from using "echo" style statements and use it's own renderer functions but this can come later and shouldn't hold up this patch from being integrated.

          Show
          Dan Marsden added a comment - NOTES FOR INTEGRATORS: MASTER only please - this should not go in stable branches. The basic plugin is basically a copy of the existing report in Moodle - any bugs with the code structure that are present in existing code can be classed as "new" bugs - (feel free to create them if you spot issues) I'd like to move the plugin away from using "echo" style statements and use it's own renderer functions but this can come later and shouldn't hold up this patch from being integrated.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Dan, Ankit,

          nice to have proper report subplugins here, super!

          Anyway, I think this needs a bit more of work before landing, hence I'm reopening:

          0) One recommendation: Please don't use quiz as reference, lol, it was one of the very first implementations and I'm not sure it's the best one. I'd look to workshop instead.

          1) the subplugin name. "scorm" is NOT a good name at all (sure it's because you copied from quiz, where that name (the module one) was picked. Instead use "scormreport" or something unique. Imagine tomorrow you'll want to create another subplugin for displaying one scorm, it will be "scormdisplay" (just one silly example). Simply don't use the name of the module to name on subplugin. See workshop.

          2) The get_reports_list() function. Please don't transverse directories there at all, instead use the get_plugin_list('subpluginame') function that will return you a better list. Then apply for cap checks or whatever you need.

          3) Capabilities needed for each report?

          4) Add them in the list of "core" plugins, so they will show properly (and not as 3rd part) on upgrade.

          These are the most important points I've detected looking to this. Please fix them, I'll be happy to take a more deeper review then.

          Great work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Dan, Ankit, nice to have proper report subplugins here, super! Anyway, I think this needs a bit more of work before landing, hence I'm reopening: 0) One recommendation: Please don't use quiz as reference, lol, it was one of the very first implementations and I'm not sure it's the best one. I'd look to workshop instead. 1) the subplugin name. "scorm" is NOT a good name at all (sure it's because you copied from quiz, where that name (the module one) was picked. Instead use "scormreport" or something unique. Imagine tomorrow you'll want to create another subplugin for displaying one scorm, it will be "scormdisplay" (just one silly example). Simply don't use the name of the module to name on subplugin. See workshop. 2) The get_reports_list() function. Please don't transverse directories there at all, instead use the get_plugin_list('subpluginame') function that will return you a better list. Then apply for cap checks or whatever you need. 3) Capabilities needed for each report? 4) Add them in the list of "core" plugins, so they will show properly (and not as 3rd part) on upgrade. These are the most important points I've detected looking to this. Please fix them, I'll be happy to take a more deeper review then. Great work, ciao
          Hide
          Dan Marsden added a comment -

          Thanks Eloy -
          1 good point about the plugin name
          2. whoops - we were using get_plugin_list at some point but that function has been through a few changes and I forgot about it.
          3. yeah - each report should be able to define it's own capabilities and restrict access if required. (quiz reports do this using the nasty mod.php file.) - but, I've just taken another look and /scorm/report.php requires mod/scorm:viewreport so we don't need to implement that check in the initial "Basic" plugin - Ankit we should just remove the canview function from the basic plugin class - the default class returns true and we know the user already has scorm:viewreport - hope that makes sense, ping me on jabber if not.
          4. isn't this what the subplugins.php file is for? - or do we need to put them into the main core list?

          Ankit - can you please fix points 1-3 above (make sure you grab the latest code from my repo) - thanks!

          Show
          Dan Marsden added a comment - Thanks Eloy - 1 good point about the plugin name 2. whoops - we were using get_plugin_list at some point but that function has been through a few changes and I forgot about it. 3. yeah - each report should be able to define it's own capabilities and restrict access if required. (quiz reports do this using the nasty mod.php file.) - but, I've just taken another look and /scorm/report.php requires mod/scorm:viewreport so we don't need to implement that check in the initial "Basic" plugin - Ankit we should just remove the canview function from the basic plugin class - the default class returns true and we know the user already has scorm:viewreport - hope that makes sense, ping me on jabber if not. 4. isn't this what the subplugins.php file is for? - or do we need to put them into the main core list? Ankit - can you please fix points 1-3 above (make sure you grab the latest code from my repo) - thanks!
          Hide
          Petr Škoda added a comment -

          There is a tiny cosmetic problem with phpdocs - your new plugins should use
          @package scormreport
          @subpackage basic

          I am going to fix it myself.

          Show
          Petr Škoda added a comment - There is a tiny cosmetic problem with phpdocs - your new plugins should use @package scormreport @subpackage basic I am going to fix it myself.
          Hide
          Petr Škoda added a comment -

          4/ yes, the subplugins.php is the right place to add module subplugins - if you are not sure always look into mod/workshop...

          Show
          Petr Škoda added a comment - 4/ yes, the subplugins.php is the right place to add module subplugins - if you are not sure always look into mod/workshop...
          Hide
          Petr Škoda added a comment -

          Integrated, thanks!

          Show
          Petr Škoda added a comment - Integrated, thanks!
          Hide
          Petr Škoda added a comment -

          I was getting some noise from the userreport.php:
          Invalid get_string() identifier: 'cmi.core.lesson_location' or component 'scorm'
          line 5949 of /lib/moodlelib.php: call to debugging()
          line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string()
          line 322 of /mod/scorm/userreport.php: call to get_string()
          Invalid get_string() identifier: 'cmi.interactions_0.id' or component 'scorm'
          line 5949 of /lib/moodlelib.php: call to debugging()
          line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string()
          line 322 of /mod/scorm/userreport.php: call to get_string()
          Invalid get_string() identifier: 'cmi.interactions_1.id' or component 'scorm'
          line 5949 of /lib/moodlelib.php: call to debugging()
          line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string()
          line 322 of /mod/scorm/userreport.php: call to get_string()

          but I guess that would be a separate issue.

          Show
          Petr Škoda added a comment - I was getting some noise from the userreport.php: Invalid get_string() identifier: 'cmi.core.lesson_location' or component 'scorm' line 5949 of /lib/moodlelib.php: call to debugging() line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string() line 322 of /mod/scorm/userreport.php: call to get_string() Invalid get_string() identifier: 'cmi.interactions_0.id' or component 'scorm' line 5949 of /lib/moodlelib.php: call to debugging() line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string() line 322 of /mod/scorm/userreport.php: call to get_string() Invalid get_string() identifier: 'cmi.interactions_1.id' or component 'scorm' line 5949 of /lib/moodlelib.php: call to debugging() line 6539 of /lib/moodlelib.php: call to core_string_manager->get_string() line 322 of /mod/scorm/userreport.php: call to get_string() but I guess that would be a separate issue.
          Hide
          Petr Škoda added a comment -

          Tested, works as described. Thanks.

          Show
          Petr Škoda added a comment - Tested, works as described. Thanks.
          Hide
          Ankit Agarwal added a comment -

          Hi Skodak,
          Thanks for the integrations and comments.
          About the errors you mentioned..I have already created MDL-27624 for that.
          Will be patching that soon.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Skodak, Thanks for the integrations and comments. About the errors you mentioned..I have already created MDL-27624 for that. Will be patching that soon. Thanks
          Hide
          Petr Škoda added a comment -

          wow, that was fast! thanks

          Show
          Petr Škoda added a comment - wow, that was fast! thanks
          Hide
          Petr Škoda added a comment -

          There is one more issue, the new basic report is not included in the list of standard plugins yet (it is displayed as non-standard plugin during install/upgrade). There is a list of standard plugins somewhere in moodellib.php (or is it admin lib?)

          Show
          Petr Škoda added a comment - There is one more issue, the new basic report is not included in the list of standard plugins yet (it is displayed as non-standard plugin during install/upgrade). There is a list of standard plugins somewhere in moodellib.php (or is it admin lib?)
          Hide
          Dan Marsden added a comment -

          I guess you mean standard_plugins_list in pluginlib.php - I'll create a new bug for that and push through a fix - thanks.

          Show
          Dan Marsden added a comment - I guess you mean standard_plugins_list in pluginlib.php - I'll create a new bug for that and push through a fix - thanks.
          Hide
          Dan Marsden added a comment -

          have created MDL-28441 to track this - probably won't get to it in time for this weeks review.

          Show
          Dan Marsden added a comment - have created MDL-28441 to track this - probably won't get to it in time for this weeks review.
          Hide
          Petr Škoda added a comment -

          Ah, right, that one. It is only a cosmetic issue in dev branch, no worries.

          Show
          Petr Škoda added a comment - Ah, right, that one. It is only a cosmetic issue in dev branch, no worries.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: