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

      Gliffy Diagrams

        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: