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 Sub-task
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      16915

      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.

        Activity

        Hide
        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
        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
        Dan Marsden added a comment -

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

        is $strname actually used anywhere?

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

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

        are they actually used?

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

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

        Show
        Dan Marsden added a comment - grrr. had wrong view up - those vars might be used.. sorry!
        Hide
        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
        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 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 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 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 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
        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
        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 Agarwal added a comment -

        Hi Dan,
        discarded the version changes.

        Thanks

        Show
        Ankit Agarwal added a comment - Hi Dan, discarded the version changes. Thanks
        Hide
        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
        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
        Sam Hemelryk added a comment -

        Thanks guys this has been integrated now

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

        Hi,
        Thanks for the review and integration

        Show
        Ankit Agarwal added a comment - Hi, Thanks for the review and integration
        Hide
        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
        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
        Dan Marsden added a comment -

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

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

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

        Closing.

        Show
        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: