Details

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

      Please install this Scorm package
      http://moodle.org/mod/data/view.php?d=50&rid=1655&filter=1

      Login as a student Attempt and complete the quiz
      Logout and logback as teacher
      Visit Scorm package>report>basic report
      Set show track details to yes.
      click on any link in the track data (last) column.
      You should be taken to userreport.php on the bottom of which you should be able to see a table
      consisting of Scorm elements , their definitions and values.
      The definition part is added by this patch.

      Show
      Please install this Scorm package http://moodle.org/mod/data/view.php?d=50&rid=1655&filter=1 Login as a student Attempt and complete the quiz Logout and logback as teacher Visit Scorm package>report>basic report Set show track details to yes. click on any link in the track data (last) column. You should be taken to userreport.php on the bottom of which you should be able to see a table consisting of Scorm elements , their definitions and values. The definition part is added by this patch.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      master_MDL-27624
    • Rank:
      16937

      Description

      When a user chooses to see track details , a bunch of data elements name are show. Which doesn't make any sense to normal user. The code to convert this data element into a string is present in the reporting as shown below:-

      $row[] = get_string($element, 'scorm') != '[['.$element.']]' ? get_string($element, 'scorm') : $element;

      {/quote}

      But those elements are not defined in the language files thus ,get_String is actually of no use here.

      This should be handled in a better way and if possible a description of common data elements should be provided to the user as well.

        Activity

        Hide
        Ankit Agarwal added a comment -

        As per changes in MDL-27522, the effected file now is userreport.php

        Show
        Ankit Agarwal added a comment - As per changes in MDL-27522 , the effected file now is userreport.php
        Hide
        Dan Marsden added a comment -

        Hi Ankit - looks great to me! - can you please rebase against latest master and add some testing instructions for the QA team? - point to a SCORM object that generates tracks like the ones you've translated above?

        thanks!

        Show
        Dan Marsden added a comment - Hi Ankit - looks great to me! - can you please rebase against latest master and add some testing instructions for the QA team? - point to a SCORM object that generates tracks like the ones you've translated above? thanks!
        Hide
        Dan Marsden added a comment -

        NOTE for Integrator - this is in Ankit's github not mine and this is for master only. - thanks!

        Show
        Dan Marsden added a comment - NOTE for Integrator - this is in Ankit's github not mine and this is for master only. - thanks!
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        I'm reopening this issue at the moment as unfortunately we are running out of time for this weeks integration due to Eloy being on holiday at the start of the integration and myself getting sick.
        We'll endeavour to look at this properly next Monday.

        I did have a quick look at the code just then however, the changes look like a positive change however the couldn't all of those if's be if..else's to avoid the numerous stristr calls once we have found the correct one? also whitespace needs to be cleaned up as per the coding guidelines.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, I'm reopening this issue at the moment as unfortunately we are running out of time for this weeks integration due to Eloy being on holiday at the start of the integration and myself getting sick. We'll endeavour to look at this properly next Monday. I did have a quick look at the code just then however, the changes look like a positive change however the couldn't all of those if's be if..else's to avoid the numerous stristr calls once we have found the correct one? also whitespace needs to be cleaned up as per the coding guidelines. Cheers Sam
        Hide
        Dan Marsden added a comment -

        Thanks Sam - I wondered how you were going to get through the integration this week!

        good point about if statements - they should definately be else if - Ankit can you please sort this out and check for spacing issues?

        thanks!

        Show
        Dan Marsden added a comment - Thanks Sam - I wondered how you were going to get through the integration this week! good point about if statements - they should definately be else if - Ankit can you please sort this out and check for spacing issues? thanks!
        Hide
        Ankit Agarwal added a comment -

        Thanks Dan and sam,
        I have made changes and pushed the code.
        Thanks

        Show
        Ankit Agarwal added a comment - Thanks Dan and sam, I have made changes and pushed the code. Thanks
        Hide
        Dan Marsden added a comment -

        Hi Ankit - can you please rebase on this weeks master and I will resubmit - thanks!

        Show
        Dan Marsden added a comment - Hi Ankit - can you please rebase on this weeks master and I will resubmit - thanks!
        Hide
        Ankit Agarwal added a comment -

        Hi Dan,
        Rebase done.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Dan, Rebase done. Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi, I'm rejecting this based on:

        1.- Missing whitespace around all =, ==, != operators for improved readability (coding guidelines).
        2.- stristr() returns match or false (not null)

        So:

        $string=false;
        if (stristr($element, '.id')!=null) {
            $string="interactionsid";
        ....
        ....
        

        Should look like:

        $string = false;
        if (stristr($element, '.id') !== false) {
            $string = 'interactionsid';
        ....
        ....
        

        Also, I'm not sure if it's a good idea to repeat the element into the 1st and 2nd columns if the string is not available, perhaps it's better to leave the 2nd column (description) blank? Please, discuss that with Dan.

        Thanks for the effort and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi, I'm rejecting this based on: 1.- Missing whitespace around all =, ==, != operators for improved readability (coding guidelines). 2.- stristr() returns match or false (not null) So: $string= false ; if (stristr($element, '.id')!= null ) { $string= "interactionsid" ; .... .... Should look like: $string = false ; if (stristr($element, '.id') !== false ) { $string = 'interactionsid'; .... .... Also, I'm not sure if it's a good idea to repeat the element into the 1st and 2nd columns if the string is not available, perhaps it's better to leave the 2nd column (description) blank? Please, discuss that with Dan. Thanks for the effort and ciao
        Hide
        Rossiani Wijaya added a comment -

        Hi Dan and Ankit,

        This issue also occurs in m2.0 and m2.1, under report.php file for 2.0. Perhaps you could also provide patches for 2.0 and 2.1 versions.

        Thanks.

        Show
        Rossiani Wijaya added a comment - Hi Dan and Ankit, This issue also occurs in m2.0 and m2.1, under report.php file for 2.0. Perhaps you could also provide patches for 2.0 and 2.1 versions. Thanks.
        Hide
        Dan Marsden added a comment -

        Thanks Rossiani - this dates back even further to 1.6 I think! - it's just that 2.0 now displays big debugging messages for missing strings.

        I class this as part of the reporting improvements that Ankit has been working on - not really a bug - more an improvement and we are only pushing improvements/features into master.

        happy for someone else to do the work and submit for integration themselves but I won't be.

        thanks!

        Show
        Dan Marsden added a comment - Thanks Rossiani - this dates back even further to 1.6 I think! - it's just that 2.0 now displays big debugging messages for missing strings. I class this as part of the reporting improvements that Ankit has been working on - not really a bug - more an improvement and we are only pushing improvements/features into master. happy for someone else to do the work and submit for integration themselves but I won't be. thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (master only). Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (master only). Thanks!
        Hide
        Rossiani Wijaya added a comment -

        Tested and it works great.

        Thanks Dan and Ankit.

        Test passed.

        Just some side note: in this week integration, we are also updating YUI version that might create an issue to upload scorm file through file picker. The upload issue is not related to this patch.

        Rosie

        Show
        Rossiani Wijaya added a comment - Tested and it works great. Thanks Dan and Ankit. Test passed. Just some side note: in this week integration, we are also updating YUI version that might create an issue to upload scorm file through file picker. The upload issue is not related to this patch. Rosie
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: