Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27256 Improve SCORM quiz reporting
  3. MDL-27522

separate generation of user report from main report page to allow better re-use

    Details

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

      Enter a SCORM as a student to generate an attempt.
      Login as a teacher and view the "results" tab on the SCORM page,
      click on the number in the users "attempt" column
      verify that it still works

      NOTE: No actual funcionality change has occured here - this is purely a structural change that moves the user report generation to a new file in preperation for MDL-27256

      Show
      Enter a SCORM as a student to generate an attempt. Login as a teacher and view the "results" tab on the SCORM page, click on the number in the users "attempt" column verify that it still works NOTE: No actual funcionality change has occured here - this is purely a structural change that moves the user report generation to a new file in preperation for MDL-27256
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      master_MDL-272522

      Description

      At present a single file mod/scorm/report.php handles all Scorm reporting request.It is a good idea to move the details of individual attempts to a separate file "reportuser.php" which can generically used by the current report plugin to display the attempt details and other related stuff.

        Gliffy Diagrams

          Activity

          Hide
          danmarsden Dan Marsden added a comment -

          Hi Ankit - looking good.
          couple more changes:
          can we please rename reportuser.php to userreport.php (I know I was the one to call it reportuser.php in the first place!)

          also rename the new capability "viewreports" to "viewuserreport"

          also - I don't think the new string "invaliduser" is used anymore after the MUST_EXIST flags are now used.

          Show
          danmarsden Dan Marsden added a comment - Hi Ankit - looking good. couple more changes: can we please rename reportuser.php to userreport.php (I know I was the one to call it reportuser.php in the first place!) also rename the new capability "viewreports" to "viewuserreport" also - I don't think the new string "invaliduser" is used anymore after the MUST_EXIST flags are now used.
          Hide
          danmarsden Dan Marsden added a comment -

          in report.php: $strname = get_string('name');

          is $strname actually used anywhere?

          Show
          danmarsden Dan Marsden added a comment - in report.php: $strname = get_string('name'); is $strname actually used anywhere?
          Hide
          danmarsden Dan Marsden added a comment -

          also:
          $strscorms = get_string('modulenameplural', 'scorm');
          $strscorm = get_string('modulename', 'scorm');

          are they actually used?

          Show
          danmarsden Dan Marsden added a comment - also: $strscorms = get_string('modulenameplural', 'scorm'); $strscorm = get_string('modulename', 'scorm'); are they actually used?
          Hide
          danmarsden Dan Marsden added a comment -

          grrr. had wrong view up - those vars might be used.. sorry!

          Show
          danmarsden Dan Marsden added a comment - grrr. had wrong view up - those vars might be used.. sorry!
          Hide
          danmarsden Dan Marsden added a comment -

          nope - had right view - those vars don't seem to be used in reportuser.php - are they even used in report.php? (haven't looked)

          Show
          danmarsden Dan Marsden added a comment - nope - had right view - those vars don't seem to be used in reportuser.php - are they even used in report.php? (haven't looked)
          Hide
          dongsheng Dongsheng Cai added a comment -

          Hi Dan and Ankit

          I just had a quick look, generally, code looks fine, good structure and using new APIs.

          Keep up the good works.

          Regards,
          Dongsheng

          Show
          dongsheng Dongsheng Cai added a comment - Hi Dan and Ankit I just had a quick look, generally, code looks fine, good structure and using new APIs. Keep up the good works. Regards, Dongsheng
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Yes..just rechecked..those vars not used anywhere..not even in the original report.php.. removed them from both files.

          Show
          ankit_frenz Ankit Agarwal added a comment - Yes..just rechecked..those vars not used anywhere..not even in the original report.php.. removed them from both files.
          Hide
          danmarsden Dan Marsden added a comment - - edited

          great! - was a better idea to remove that cap rather than create a new one at this stage too.

          because there isn't any change to caps for this patch we should probably remove the version.php change as well - version.php should only change when js/css or stuff in mod/scorm/db/ changes occur.

          once you remove that version.php change I'll submit this for integration review - great work!

          Show
          danmarsden Dan Marsden added a comment - - edited great! - was a better idea to remove that cap rather than create a new one at this stage too. because there isn't any change to caps for this patch we should probably remove the version.php change as well - version.php should only change when js/css or stuff in mod/scorm/db/ changes occur. once you remove that version.php change I'll submit this for integration review - great work!
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Hi Dan,
          discarded the version changes.

          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Hi Dan, discarded the version changes. Thanks
          Hide
          danmarsden Dan Marsden added a comment -

          Hi Ankit - I made a couple of changes to this and have submitted it for review - good work!

          I've squashed all your commits into a single commit in git so you might need to do a bit of merging/fixing conflicts with your other branches (don't do it yet as it's likely that the integration reviewer will squash it further - merging my commit with yours into a single commit) - thanks for all your work on this!

          Show
          danmarsden Dan Marsden added a comment - Hi Ankit - I made a couple of changes to this and have submitted it for review - good work! I've squashed all your commits into a single commit in git so you might need to do a bit of merging/fixing conflicts with your other branches (don't do it yet as it's likely that the integration reviewer will squash it further - merging my commit with yours into a single commit) - thanks for all your work on this!
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks guys this has been integrated now

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks guys this has been integrated now
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Hi,
          Thanks for the review and integration

          Show
          ankit_frenz Ankit Agarwal added a comment - Hi, Thanks for the review and integration
          Hide
          nebgor Aparup Banerjee added a comment -

          the page is there and its working fine.

          note: this may be another bug but there are some lang string errors when viewing 'Track details' in that page. (is this related to the scorm file itself?)

          Invalid get_string() identifier: 'cmi.interactions_0.correct_responses_0.pattern' or component 'scorm'
          line 5910 of /lib/moodlelib.php: call to debugging()
          line 6500 of /lib/moodlelib.php: call to core_string_manager->get_string()
          line 323 of /mod/scorm/userreport.php: call to get_string()

          Show
          nebgor Aparup Banerjee added a comment - the page is there and its working fine. note: this may be another bug but there are some lang string errors when viewing 'Track details' in that page. (is this related to the scorm file itself?) Invalid get_string() identifier: 'cmi.interactions_0.correct_responses_0.pattern' or component 'scorm' line 5910 of /lib/moodlelib.php: call to debugging() line 6500 of /lib/moodlelib.php: call to core_string_manager->get_string() line 323 of /mod/scorm/userreport.php: call to get_string()
          Hide
          danmarsden Dan Marsden added a comment -

          Thanks Apu - Ankit found that too, it's an existing bug - he's created MDL-27624 for it.

          Show
          danmarsden Dan Marsden added a comment - Thanks Apu - Ankit found that too, it's an existing bug - he's created MDL-27624 for it.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          And now this is part of the best Moodle weeklies ever, thanks!

          Closing.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - And now this is part of the best Moodle weeklies ever, thanks! Closing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                1/Jul/11