Moodle
  1. Moodle
  2. MDL-26880

SCORM 2004 tracking broken due to a missing parameter, the activity id, on submitting to datamodel.php

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: STABLE backlog
    • Component/s: SCORM
    • Labels:
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      16503

      Activity

      Hide
      Matteo Scaramuccia added a comment -
      Show
      Matteo Scaramuccia added a comment - Details to replicate the issue in http://moodle.org/mod/forum/discuss.php?d=171195#p751293
      Hide
      Matteo Scaramuccia added a comment -

      Patch proposal: https://github.com/scara/moodle/commit/007f562c428e3d3645c6d8085f3933af04bc2d1c
      It also contains other fixes, see the commit message, plus cleanup of commented out code supposed for debug purposes.

      Now the issue has been fixed but... PHP notices about Undefined property: stdClass::$id seems to prevent me to see the content when there is some tracking data. IMHO unrelated with this bug and the patch proposal.

      Test Case for the patch: http://moodle.org/mod/forum/discuss.php?d=171195#p751293

      Show
      Matteo Scaramuccia added a comment - Patch proposal: https://github.com/scara/moodle/commit/007f562c428e3d3645c6d8085f3933af04bc2d1c It also contains other fixes, see the commit message, plus cleanup of commented out code supposed for debug purposes. Now the issue has been fixed but... PHP notices about Undefined property: stdClass::$id seems to prevent me to see the content when there is some tracking data. IMHO unrelated with this bug and the patch proposal. Test Case for the patch: http://moodle.org/mod/forum/discuss.php?d=171195#p751293
      Hide
      Dan Marsden added a comment -

      looks good to me - thanks Matteo!

      Show
      Dan Marsden added a comment - looks good to me - thanks Matteo!
      Hide
      Eloy Lafuente (stronk7) added a comment -

      Reopening this because some problems detected @ PULL-498. Seem easy to fix.

      Thanks and ciao

      Show
      Eloy Lafuente (stronk7) added a comment - Reopening this because some problems detected @ PULL-498. Seem easy to fix. Thanks and ciao
      Hide
      Dan Marsden added a comment -

      Thanks Eloy! - good spotting!

      Matteo - do you have time to fix this today? - just commit the change on top of the existing branch if you can (ping me on jabber/gchat if Eloys comment on PULL-498 doesn't make sense)

      thanks!

      Show
      Dan Marsden added a comment - Thanks Eloy! - good spotting! Matteo - do you have time to fix this today? - just commit the change on top of the existing branch if you can (ping me on jabber/gchat if Eloys comment on PULL-498 doesn't make sense) thanks!
      Hide
      Matteo Scaramuccia added a comment -

      Hi Eloy,
      just read the reasons for the rejection: IMHO it is actually not useful to log something available only with debugging.
      While UTF-8 sounds reasonable (my fault!), multilang was already an issue before touching that code with my proposal, 'cause my Activity name was longer then VARCHAR(40) i.e. discovered by accident. What about removing that logging instead of addressing a correct substring?

      Waiting for guidelines,
      Matteo

      Show
      Matteo Scaramuccia added a comment - Hi Eloy, just read the reasons for the rejection: IMHO it is actually not useful to log something available only with debugging. While UTF-8 sounds reasonable (my fault!), multilang was already an issue before touching that code with my proposal, 'cause my Activity name was longer then VARCHAR(40) i.e. discovered by accident. What about removing that logging instead of addressing a correct substring? Waiting for guidelines, Matteo
      Hide
      Dan Marsden added a comment -

      my vote - keep the logging but remove the scorm->name as it isn't needed.

      Show
      Dan Marsden added a comment - my vote - keep the logging but remove the scorm->name as it isn't needed.
      Hide
      Matteo Scaramuccia added a comment -

      OK
      Just looking at the possibility to use the identifier since at that level we're logging at SCO level and not generically at Activity level... I'm checking the data type for identifiers in the IMSManifest, if ASCII it will solve anything keeping the code simple

      Show
      Matteo Scaramuccia added a comment - OK Just looking at the possibility to use the identifier since at that level we're logging at SCO level and not generically at Activity level... I'm checking the data type for identifiers in the IMSManifest, if ASCII it will solve anything keeping the code simple
      Hide
      Matteo Scaramuccia added a comment -

      Fix proposal for PULL-498 rejection: https://github.com/scara/moodle/compare/007f562...3c86719
      Tracking data is at SCO level and not at Activity level so $scorm->name is actually not relevant to identify the scope of the tracking. That being said, IMS Manifest item identifiers cannot be used being of datatype string and potentially UTF-8: my proposal is to use plain numbers i.e. the SCO PK.

      Note: as part of the discussion with Dan, I think it is useful to keep track that such logging was added at the time of MDL-13431.

      Show
      Matteo Scaramuccia added a comment - Fix proposal for PULL-498 rejection: https://github.com/scara/moodle/compare/007f562...3c86719 Tracking data is at SCO level and not at Activity level so $scorm->name is actually not relevant to identify the scope of the tracking. That being said, IMS Manifest item identifiers cannot be used being of datatype string and potentially UTF-8: my proposal is to use plain numbers i.e. the SCO PK. Note: as part of the discussion with Dan, I think it is useful to keep track that such logging was added at the time of MDL-13431 .
      Hide
      Dan Marsden added a comment -

      looks good to me - thanks Matteo, Eloy can you please review the Pull request again? - thanks!

      Show
      Dan Marsden added a comment - looks good to me - thanks Matteo, Eloy can you please review the Pull request again? - thanks!
      Hide
      Matteo Scaramuccia added a comment -

      Hi Eloy, any update about the integration based on my proposal about your rejection? Ref: comments above and https://github.com/scara/moodle/compare/MOODLE_20_STABLE...3c86719 or https://github.com/scara/moodle/compare/MOODLE_20_STABLE...m20_MDL-26880_SCORM13_AJAX_activityid_missing for a quick view.

      Show
      Matteo Scaramuccia added a comment - Hi Eloy, any update about the integration based on my proposal about your rejection? Ref: comments above and https://github.com/scara/moodle/compare/MOODLE_20_STABLE...3c86719 or https://github.com/scara/moodle/compare/MOODLE_20_STABLE...m20_MDL-26880_SCORM13_AJAX_activityid_missing for a quick view.
      Hide
      Dan Marsden added a comment -

      I probably need to create a new Pull request - will do that later this week.

      Show
      Dan Marsden added a comment - I probably need to create a new Pull request - will do that later this week.
      Hide
      Helen Foster added a comment -

      Reopening - please see PULL-498 for reasons.

      Matteo and Dan, thanks for your work on this issue. Looking forward to a new pull request

      Show
      Helen Foster added a comment - Reopening - please see PULL-498 for reasons. Matteo and Dan, thanks for your work on this issue. Looking forward to a new pull request
      Hide
      Dan Marsden added a comment -

      pull request resubmitted.

      Show
      Dan Marsden added a comment - pull request resubmitted.

        People

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

          Dates

          • Created:
            Updated:
            Resolved: