Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41290

Improve SCORM user level report

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      This should be tested with a SCORM package that has interactions data - ping Dan for a SCORM package that provides this if you don't have one.

      Make sure multiple users have submitted multiple attempts to the same package - view the standard report then click on the attempt number column to get at the user level learning objects report - then check that you can scroll through each user attempt correctly (pagination of attempts on the user report is a new feature)
      then check that you can view the track details report for each learning object in the report.

      then check the interactions user level report and make sure you can export the report in the different formats succesfully.

      Any suggestions on improving the navigation or headers on these reports would be appreciated - it still feels a bit clunky to me.

      Show
      This should be tested with a SCORM package that has interactions data - ping Dan for a SCORM package that provides this if you don't have one. Make sure multiple users have submitted multiple attempts to the same package - view the standard report then click on the attempt number column to get at the user level learning objects report - then check that you can scroll through each user attempt correctly (pagination of attempts on the user report is a new feature) then check that you can view the track details report for each learning object in the report. then check the interactions user level report and make sure you can export the report in the different formats succesfully. Any suggestions on improving the navigation or headers on these reports would be appreciated - it still feels a bit clunky to me.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      master_MDL-41290

      Description

      The user level report page isn't great - we should improve it so that you can select different attempts once you are viewing an individual attempt and we should display the data in a more useful manner like the interactions and objectives reports. It should also allow the data to be exported.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              danmarsden Dan Marsden added a comment -

              this is a pretty significant rewrite of the user level report - few things that need a close review and I'd appreciate some help on.

              Headers/navigation between reports - I'm not sure this is quite right yet.

              use of pagination and the way I override render_paging_barto allow a different label to be used.

              I haven't implemented the objectives style report for the users yet or converted the learning objects table to a flexible table but that can come at a future time after this patch makes it in.

              Show
              danmarsden Dan Marsden added a comment - this is a pretty significant rewrite of the user level report - few things that need a close review and I'd appreciate some help on. Headers/navigation between reports - I'm not sure this is quite right yet. use of pagination and the way I override render_paging_barto allow a different label to be used. I haven't implemented the objectives style report for the users yet or converted the learning objects table to a flexible table but that can come at a future time after this patch makes it in.
              Hide
              danmarsden Dan Marsden added a comment -

              this main parts of this patch are:

              • adding pagination to the user report page to allow you to navigate through the different attempts.
              • rewrite of the way it displays the track details report - the old report showed a lot of duplicate data - the new one simplifies it a bit by just displaying all tracking information and allows export to file as well.
              • a new user level interactions report which shows a nice break down of a single users interactions and allows export to file.
              Show
              danmarsden Dan Marsden added a comment - this main parts of this patch are: adding pagination to the user report page to allow you to navigate through the different attempts. rewrite of the way it displays the track details report - the old report showed a lot of duplicate data - the new one simplifies it a bit by just displaying all tracking information and allows export to file as well. a new user level interactions report which shows a nice break down of a single users interactions and allows export to file.
              Hide
              danmarsden Dan Marsden added a comment -

              adding Matteo and Ankit here - you guys might have an interest in this patch.

              Show
              danmarsden Dan Marsden added a comment - adding Matteo and Ankit here - you guys might have an interest in this patch.
              Hide
              ankit_frenz Ankit Agarwal added a comment - - edited

              Hi Dan,
              Thanks for working on this. This is a very nice improvement. Here are a few minor things that I noticed:-

              1. $CFG is not used in scorm_get_tracks()
              2. $trackdata is of type array, phpdoc defines it as object in scorm_format_interactions()
              3. There is an is_array() check https://github.com/danmarsden/moodle/compare/moodle:master...master_MDL-41290#L1R582

                    if (is_array($usertrack)) {
                        ksort($usertrack);
                    }
                

                am not sure if that is needed anymore. Isn't $usertrack always an object of stdClass ?

              4. shouldn't we deprecate scorm_get_user_data() before removing it completely?
              5. In mod/scorm/renderer.php, the php docs says "Defines the renderer for the quiz module.", which clearly is a typo.
              6. It makes sense to me to add support for custom label in core paging_bar class. We shouldn't ideally need to override just to change the label.
              7. In userinteractionsreport.php,
                1. we should use scorm_get_tracks() instead of fetching the tracks manually and than formatting it.
                2. The first call to $table->is_downloading($download) , should also pass the filename. Other wise the download files are ending up with an empty name .
                3. May be we should consider wrapping the id column here as well?
              8. In userreporttracks.php
                1. $trackdata = stdClass(); should be $trackdata = new stdClass();
                2. The first call to $table->is_downloading($download) , should also pass the filename. Other wise the download files are ending up with an empty name .

              Some general comments

              1. @package should be mod_scorm, in page level phpdocs.
              2. We recommend having spaces around operators. Spaces are missing in a few places around =>, = , +, -
              3. add_to_log() will eventually be deprecated. We should use events. However this won't be happening in 2.6 afaik, so feel free to ignore this comment.
              4. we should use get_course() instead of get_record() to fetch the course, when possible.
              5. Are we not going to break all third party custom reports with this change?
              6. Am not sure using the pagination bar is a correct way forward. For example pagination breaks, if there are 3 attempts from a user and I delete the second attempt. In the report you see link to three pages. May be we should consider extending paging_bar class to accept a set of attempt ids?
              7. I have added a bunch of labels to the issue.
              Show
              ankit_frenz Ankit Agarwal added a comment - - edited Hi Dan, Thanks for working on this. This is a very nice improvement. Here are a few minor things that I noticed:- $CFG is not used in scorm_get_tracks() $trackdata is of type array, phpdoc defines it as object in scorm_format_interactions() There is an is_array() check https://github.com/danmarsden/moodle/compare/moodle:master...master_MDL-41290#L1R582 if (is_array($usertrack)) { ksort($usertrack); } am not sure if that is needed anymore. Isn't $usertrack always an object of stdClass ? shouldn't we deprecate scorm_get_user_data() before removing it completely? In mod/scorm/renderer.php, the php docs says "Defines the renderer for the quiz module.", which clearly is a typo. It makes sense to me to add support for custom label in core paging_bar class. We shouldn't ideally need to override just to change the label. In userinteractionsreport.php, we should use scorm_get_tracks() instead of fetching the tracks manually and than formatting it. The first call to $table->is_downloading($download) , should also pass the filename. Other wise the download files are ending up with an empty name . May be we should consider wrapping the id column here as well? In userreporttracks.php $trackdata = stdClass(); should be $trackdata = new stdClass(); The first call to $table->is_downloading($download) , should also pass the filename. Other wise the download files are ending up with an empty name . Some general comments @package should be mod_scorm, in page level phpdocs. We recommend having spaces around operators. Spaces are missing in a few places around =>, = , +, - add_to_log() will eventually be deprecated. We should use events. However this won't be happening in 2.6 afaik, so feel free to ignore this comment. we should use get_course() instead of get_record() to fetch the course, when possible. Are we not going to break all third party custom reports with this change? Am not sure using the pagination bar is a correct way forward. For example pagination breaks, if there are 3 attempts from a user and I delete the second attempt. In the report you see link to three pages. May be we should consider extending paging_bar class to accept a set of attempt ids? I have added a bunch of labels to the issue.
              Hide
              danmarsden Dan Marsden added a comment -

              Great review thanks.
              1. done.
              2. done.
              3. agree - removed.
              4. I don't think we need to - nothing else uses it and if they do it's an easy fix. (if integrators disagree that's fine.)
              5. done.
              6. we may need to write our own pagination anyway as you spotted that deleting attempts becomes problematic.
              7-1 Scorm_get_Tracks only shows data from a single sco - in the report we want to display output from all scoes in a single package.
              7-2 done.
              7-3 This isn't something that has previously been requested - would prefer to leave as-is for now and deal with that seperately if required.
              8.1 done.
              8.2 done.

              General stuff:
              1. fixed new files and a couple of others - will deal with others at some point.
              2. yeah - wish this was checked in the codechecker rules - would be easier to see these, most will be from copy/paste of old files but will try to catch most of them. (I use codechecker integrated in my IDE)
              3. yep - ignoring for now
              4. good point - was being lazy with copy/paste there.
              5. Only links to the old usereport page will break - the only custom report I'm aware of atm is the scormreport_trends which doesn't link to userreports page.
              6. Agree - I didn't like it in the first place and the issue with deleting attempts now makes it even less useful.

              Thanks for the good review - did you have any feedback on the navigation/headers - I really don't like the header that shows the pagination and user details, I'm keen to chat with Barbara on how to improve that but it can possibly come after integration.

              Have implemented those changes except for the re-think around pagination.

              Show
              danmarsden Dan Marsden added a comment - Great review thanks. 1. done. 2. done. 3. agree - removed. 4. I don't think we need to - nothing else uses it and if they do it's an easy fix. (if integrators disagree that's fine.) 5. done. 6. we may need to write our own pagination anyway as you spotted that deleting attempts becomes problematic. 7-1 Scorm_get_Tracks only shows data from a single sco - in the report we want to display output from all scoes in a single package. 7-2 done. 7-3 This isn't something that has previously been requested - would prefer to leave as-is for now and deal with that seperately if required. 8.1 done. 8.2 done. General stuff: 1. fixed new files and a couple of others - will deal with others at some point. 2. yeah - wish this was checked in the codechecker rules - would be easier to see these, most will be from copy/paste of old files but will try to catch most of them. (I use codechecker integrated in my IDE) 3. yep - ignoring for now 4. good point - was being lazy with copy/paste there. 5. Only links to the old usereport page will break - the only custom report I'm aware of atm is the scormreport_trends which doesn't link to userreports page. 6. Agree - I didn't like it in the first place and the issue with deleting attempts now makes it even less useful. Thanks for the good review - did you have any feedback on the navigation/headers - I really don't like the header that shows the pagination and user details, I'm keen to chat with Barbara on how to improve that but it can possibly come after integration. Have implemented those changes except for the re-think around pagination.
              Hide
              ankit_frenz Ankit Agarwal added a comment - - edited

              I was thinking may be we can move the pagination bar to the right end of the page, while keeping the details at center. Not sure if that is going to make it better, but is an idea to consider.
              Also FYI https://github.com/moodlehq/moodle-local_codechecker/pull/11.

              Show
              ankit_frenz Ankit Agarwal added a comment - - edited I was thinking may be we can move the pagination bar to the right end of the page, while keeping the details at center. Not sure if that is going to make it better, but is an idea to consider. Also FYI https://github.com/moodlehq/moodle-local_codechecker/pull/11 .
              Hide
              danmarsden Dan Marsden added a comment -

              thanks - have just pushed a fix that implements a custom pagination bar for SCORM - which is a better solution anyway as it is a bit cleaner and noticed a couple of other small bugs that I've fixed.

              I might try to get the code in through integration with the navigation/headers as-is because it's similar to the existing display but I'm guessing this sort of display is needed in other places (user reports) so we should be consistent across other reports if poss

              pushing back up for peer review - thanks Ankit.

              Show
              danmarsden Dan Marsden added a comment - thanks - have just pushed a fix that implements a custom pagination bar for SCORM - which is a better solution anyway as it is a bit cleaner and noticed a couple of other small bugs that I've fixed. I might try to get the code in through integration with the navigation/headers as-is because it's similar to the existing display but I'm guessing this sort of display is needed in other places (user reports) so we should be consistent across other reports if poss pushing back up for peer review - thanks Ankit.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              Hi Dan,
              Thanks for the quick update.
              The changes looks good. I haven't checked, but is it possible to simulate scorm attempts in unit tests? If yes, we might want to have some tests for scorm_get_all_attempts().

              Also may be we should consider using classes\ directory, to put in the new classes that we are writing, scorm_attempt_bar() and mod_scorm_renderer(), so that auto loading could be used. Sorry, I missed this one before.

              Feel free to push for integration, after you have considered above comments.

              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - Hi Dan, Thanks for the quick update. The changes looks good. I haven't checked, but is it possible to simulate scorm attempts in unit tests? If yes, we might want to have some tests for scorm_get_all_attempts(). Also may be we should consider using classes\ directory, to put in the new classes that we are writing, scorm_attempt_bar() and mod_scorm_renderer(), so that auto loading could be used. Sorry, I missed this one before. Feel free to push for integration, after you have considered above comments. Thanks
              Hide
              danmarsden Dan Marsden added a comment - - edited

              Thanks Ankit - have rebased and now bouncing up for integration - apparently class auto-loader isn't ready for renderers - policy still states to use renderer.php

              Hard to see a useful way of unit testing scorm_get_all_attempts - it's basically just a DB call so all we'd be testing would be dml functions which should be covered already.

              NOTE TO INTEGRATOR:
              This contains AMOS script for some of the help strings that have moved but have the same value.

              Show
              danmarsden Dan Marsden added a comment - - edited Thanks Ankit - have rebased and now bouncing up for integration - apparently class auto-loader isn't ready for renderers - policy still states to use renderer.php Hard to see a useful way of unit testing scorm_get_all_attempts - it's basically just a DB call so all we'd be testing would be dml functions which should be covered already. NOTE TO INTEGRATOR: This contains AMOS script for some of the help strings that have moved but have the same value.
              Hide
              danmarsden Dan Marsden added a comment -

              MDL-39910 MDL-28579 MDL-38489 Must be integrated before this patch - the branch above contains patches from all those issues.

              Show
              danmarsden Dan Marsden added a comment - MDL-39910 MDL-28579 MDL-38489 Must be integrated before this patch - the branch above contains patches from all those issues.
              Hide
              danmarsden Dan Marsden added a comment -

              rebased to remove reliance on MDL-39910 - (still reliant on MDL-28579 and MDL-38489)

              Show
              danmarsden Dan Marsden added a comment - rebased to remove reliance on MDL-39910 - (still reliant on MDL-28579 and MDL-38489 )
              Hide
              danmarsden Dan Marsden added a comment -

              rebased so no longer reliant on MDL-28579 and MDL-38489 - cherry picks fine before and after those patches.

              Show
              danmarsden Dan Marsden added a comment - rebased so no longer reliant on MDL-28579 and MDL-38489 - cherry picks fine before and after those patches.
              Hide
              damyon Damyon Wiese added a comment -

              Created an issue to allow renderers to be autoloaded.

              MDL-41663

              Show
              damyon Damyon Wiese added a comment - Created an issue to allow renderers to be autoloaded. MDL-41663
              Hide
              damyon Damyon Wiese added a comment -

              One suggestion for the navigation headers would be to have the activity name heading above the tabs/navigation links on each page. (can be done in a separate issue).

              Show
              damyon Damyon Wiese added a comment - One suggestion for the navigation headers would be to have the activity name heading above the tabs/navigation links on each page. (can be done in a separate issue).
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Dan,

              All looks good to me. Integrated to master only.

              Show
              damyon Damyon Wiese added a comment - Thanks Dan, All looks good to me. Integrated to master only.
              Hide
              jerome Jérôme Mouneyrac added a comment -

              I can't test "then check that you can scroll through each user attempt correctly" as I have nothing to scroll, however pagination works. All the rest is passed too. Passing.

              Show
              jerome Jérôme Mouneyrac added a comment - I can't test "then check that you can scroll through each user attempt correctly" as I have nothing to scroll, however pagination works. All the rest is passed too. Passing.
              Hide
              damyon Damyon Wiese added a comment -

              This issue along with 77 of it's friends has been sent upstream and released to the world.

              Thankyou for your contribution.

              Show
              damyon Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.
              Hide
              tsala Helen Foster added a comment -

              Hi Dan,

              Any chance you could provide a SCORM package with interactions data for use in QA testing?

              Show
              tsala Helen Foster added a comment - Hi Dan, Any chance you could provide a SCORM package with interactions data for use in QA testing?
              Hide
              tsala Helen Foster added a comment -

              I've removed the docs_required label for this issue, as the user level report seems to be already documented in https://docs.moodle.org/en/Using_SCORM#Basic_report

              If more documentation is required, please feel free to log in to Moodle Docs and edit the page, or describe what needs doing in a comment here and re-add the docs_required label.

              Show
              tsala Helen Foster added a comment - I've removed the docs_required label for this issue, as the user level report seems to be already documented in https://docs.moodle.org/en/Using_SCORM#Basic_report If more documentation is required, please feel free to log in to Moodle Docs and edit the page, or describe what needs doing in a comment here and re-add the docs_required label.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              MDLQA-88 already covers this testing, so removed qa_test_required label.

              Show
              rajeshtaneja Rajesh Taneja added a comment - MDLQA-88 already covers this testing, so removed qa_test_required label.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    18/Nov/13