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

      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.

        Gliffy Diagrams

          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: