Moodle
  1. Moodle
  2. MDL-33822

SCORM report empty cells not displaying correctly.

    Details

    • Testing Instructions:
      Hide

      Make sure you have a course with multiple users enrolled
      Create a SCORM that generates interactions - here's one:
      http://moodle.org/mod/data/view.php?d=50&rid=1655
      Enter as a student and submit some responses
      View the interactions report and users that have not submitted will have cells in the report with no borders.

      Show
      Make sure you have a course with multiple users enrolled Create a SCORM that generates interactions - here's one: http://moodle.org/mod/data/view.php?d=50&rid=1655 Enter as a student and submit some responses View the interactions report and users that have not submitted will have cells in the report with no borders.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      master_MDL-33822
    • Rank:
      41909

      Description

      1. Go to an existing SCORM activity with answers
      2. Go to the report of that SCORM activity

      Result:

      • Some cells are missing in the table

      See screenshot for details

      1. MDL-33822_v221.patch
        2 kB
        Gilles-Philippe Leblanc
      1. MDL-33822_fixed.png
        17 kB
      2. screenshot-1.jpg
        55 kB

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for reporting that.

        I've put that on the backlog.

        In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

        Show
        Michael de Raadt added a comment - Thanks for reporting that. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
        Hide
        Gilles-Philippe Leblanc added a comment - - edited

        I've putted a patch for our 2.2.1 version but it may be easy to apply the modification to the latest versions.
        I have also provide modification to include the missing html cells.

        Show
        Gilles-Philippe Leblanc added a comment - - edited I've putted a patch for our 2.2.1 version but it may be easy to apply the modification to the latest versions. I have also provide modification to include the missing html cells.
        Hide
        Dan Poltawski added a comment -

        Thanks Gilles-Phillipe,

        Pinging Dan Marsden who is the component maintainer of SCORM.

        Show
        Dan Poltawski added a comment - Thanks Gilles-Phillipe, Pinging Dan Marsden who is the component maintainer of SCORM.
        Hide
        Gilles-Philippe Leblanc added a comment -

        Thanks Dan. I'm waiting for its comments.

        Show
        Gilles-Philippe Leblanc added a comment - Thanks Dan. I'm waiting for its comments.
        Hide
        Dan Marsden added a comment -

        thanks for the report - the missing cell issue sounds like something that could be browser related - which browser were you using?

        • adding Ankit here too as he wrote the interactions report.
        Show
        Dan Marsden added a comment - thanks for the report - the missing cell issue sounds like something that could be browser related - which browser were you using? adding Ankit here too as he wrote the interactions report.
        Hide
        Ankit Agarwal added a comment -

        Hi Gilles,
        I am not really sure using "urldecode" is a good idea, specially on the id field. I am just wondering if we should use html_entity_decode() instead, or use nothing at all, as it is at the moment.
        I can see the missing cells issue but $questioncount may not be the number of missing cells all the time. It also depends on $displayoptions['qtext'], $displayoptions['resp'], and $displayoptions['right'].

        Thanks

        Show
        Ankit Agarwal added a comment - Hi Gilles, I am not really sure using "urldecode" is a good idea, specially on the id field. I am just wondering if we should use html_entity_decode() instead, or use nothing at all, as it is at the moment. I can see the missing cells issue but $questioncount may not be the number of missing cells all the time. It also depends on $displayoptions ['qtext'] , $displayoptions ['resp'] , and $displayoptions ['right'] . Thanks
        Hide
        Dan Marsden added a comment -

        Ankit - I have similar reservations about using urldecode esp as it converts all + to spaces which could interfere with expected data
        If we decide to do anything then using html_entity_decode sounds like a much better idea..

        Show
        Dan Marsden added a comment - Ankit - I have similar reservations about using urldecode esp as it converts all + to spaces which could interfere with expected data If we decide to do anything then using html_entity_decode sounds like a much better idea..
        Hide
        Gilles-Philippe Leblanc added a comment -

        Hi,

        first, thanks you for all your quick answers.

        Dan, I was using firefox 12 but I have also tested with chrome 19, Internet Explorer 9 and safari 5.1.7 and I still have the missing cell issues so It's not browser related.

        Concern the use of "urldecode" I question myself that its should not be the best way to remove the entities because the fields are not actually URLs but using html_entity_decode do not work at all. The entities displayed in this issue are not html entities but url entities. See this page for explanations:
        http://stackoverflow.com/questions/1812473/difference-between-url-encode-and-html-encode

        Maybe some help should be appreciated if you still think there's a better way to correct it.

        Finally, its right that the use of $questioncount for the numbers of actual columns was incorrect. We forgot to test where more than one of $displayoptions['qtext'], $displayoptions['resp'], and $displayoptions['right'] was checked simultaneously. So thank you for finding this problem. I have updated the file that fixes it.

        Show
        Gilles-Philippe Leblanc added a comment - Hi, first, thanks you for all your quick answers. Dan, I was using firefox 12 but I have also tested with chrome 19, Internet Explorer 9 and safari 5.1.7 and I still have the missing cell issues so It's not browser related. Concern the use of "urldecode" I question myself that its should not be the best way to remove the entities because the fields are not actually URLs but using html_entity_decode do not work at all. The entities displayed in this issue are not html entities but url entities. See this page for explanations: http://stackoverflow.com/questions/1812473/difference-between-url-encode-and-html-encode Maybe some help should be appreciated if you still think there's a better way to correct it. Finally, its right that the use of $questioncount for the numbers of actual columns was incorrect. We forgot to test where more than one of $displayoptions ['qtext'] , $displayoptions ['resp'] , and $displayoptions ['right'] was checked simultaneously. So thank you for finding this problem. I have updated the file that fixes it.
        Hide
        Dan Marsden added a comment -

        I'm well aware of the differences between urldecode and entity_decode..

        As Ankit mentions, the problem is that if text is not urlencoded in the first place - running urldecode causes unintended consequences - esp around changing + to a space.
        and afaik there is no reliable/safe way of checking to see if a string was urlencoded - I'm also unsure why your SCORM package even does this? - I can't remember seeing anything in the SCORM spec about urlencoding strings for storage.

        We "could" use html_entity_decode as it has less chance of causing unintended changes to text but it sounds like that won't help your situation so the better option is just to display the text as it is received from the scorm. - the fix for the cell display looks useful though - thanks!

        Show
        Dan Marsden added a comment - I'm well aware of the differences between urldecode and entity_decode.. As Ankit mentions, the problem is that if text is not urlencoded in the first place - running urldecode causes unintended consequences - esp around changing + to a space. and afaik there is no reliable/safe way of checking to see if a string was urlencoded - I'm also unsure why your SCORM package even does this? - I can't remember seeing anything in the SCORM spec about urlencoding strings for storage. We "could" use html_entity_decode as it has less chance of causing unintended changes to text but it sounds like that won't help your situation so the better option is just to display the text as it is received from the scorm. - the fix for the cell display looks useful though - thanks!
        Hide
        Gilles-Philippe Leblanc added a comment - - edited

        I now understand better!

        I understood that the problem always occurs, regardless of the imported SCORM contents. That is why I insisted so much. So I pushed my search further by generating content with many tools and the problem is only one of a particular tool: e-learning maker. So to avoid for the quiz with SCORM. We decide on our side not to fix since the problem is not unique to Moodle.

        I am sorry to have alerted about it too quickly.
        It therefore remains to correct the problem of missing cells. The patch was again updated on this.

        Do we need to rename this task to reflect the cells problem or we close this one and create another one ?

        Show
        Gilles-Philippe Leblanc added a comment - - edited I now understand better! I understood that the problem always occurs, regardless of the imported SCORM contents. That is why I insisted so much. So I pushed my search further by generating content with many tools and the problem is only one of a particular tool: e-learning maker. So to avoid for the quiz with SCORM. We decide on our side not to fix since the problem is not unique to Moodle. I am sorry to have alerted about it too quickly. It therefore remains to correct the problem of missing cells. The patch was again updated on this. Do we need to rename this task to reflect the cells problem or we close this one and create another one ?
        Hide
        Dan Marsden added a comment -

        Thanks - it's been useful to hear how different authoring packages manage the data - thanks for updating the patch too, I'll format it as a git oatch and get Ankit to review.

        Show
        Dan Marsden added a comment - Thanks - it's been useful to hear how different authoring packages manage the data - thanks for updating the patch too, I'll format it as a git oatch and get Ankit to review.
        Hide
        Dan Marsden added a comment -

        Ankit - can you please take a look at the git patch and let me know if it's ok to sumbit for integration?

        Show
        Dan Marsden added a comment - Ankit - can you please take a look at the git patch and let me know if it's ok to sumbit for integration?
        Hide
        Ankit Agarwal added a comment -

        Hi Guys,
        The patch looks good to me. Submitting for integration.

        @integrator
        This should cleanly pick to 23 and 22.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Guys, The patch looks good to me. Submitting for integration. @integrator This should cleanly pick to 23 and 22. Thanks
        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
        Rossiani Wijaya added a comment -

        This looks good.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This looks good. Test passed.
        Hide
        Dan Poltawski added a comment -

        Congratulations!

        You've made it into the weekly release!

        Thanks for your contribution - here are some random drummers to keep you inspired for the next week!
        http://www.youtube.com/watch?v=_QhpHUmVCmY

        Show
        Dan Poltawski added a comment - Congratulations! You've made it into the weekly release! Thanks for your contribution - here are some random drummers to keep you inspired for the next week! http://www.youtube.com/watch?v=_QhpHUmVCmY

          People

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

            Dates

            • Created:
              Updated:
              Resolved: