Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: SCORM
    • Testing Instructions:
      Hide

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

      log in as a student and submit some attempts.
      log back in as a teacher
      Visit Scorm package>report>Interaction report
      make sure the Reports page operates as expected - try downloading Excel, ODS files, try deleting attempts,updating form options.
      The specific addition of this plugin is to display student's response and correct answers.Please make sure those columns are properly populated.

      Show
      Please install this Scorm package http://moodle.org/mod/data/view.php?d=50&rid=1655&filter=1 log in as a student and submit some attempts. log back in as a teacher Visit Scorm package>report>Interaction report make sure the Reports page operates as expected - try downloading Excel, ODS files, try deleting attempts,updating form options. The specific addition of this plugin is to display student's response and correct answers.Please make sure those columns are properly populated.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-28277-master

      Description

      A plugin to display interactions properly

        Gliffy Diagrams

          Issue Links

            Activity

            ankit_frenz Ankit Agarwal created issue -
            ankit_frenz Ankit Agarwal made changes -
            Field Original Value New Value
            Fix Version/s DEV backlog [ 10464 ]
            ankit_frenz Ankit Agarwal made changes -
            Summary Design of a interraction report plugin Design of a interaction report plugin
            danmarsden Dan Marsden made changes -
            Pull Master Diff URL https://github.com/ankitagarwal/moodle/compare/master...MDL28277
            Pull Master Branch MDL28277
            Pull from Repository git://github.com/ankitagarwal/moodle.git
            Hide
            danmarsden Dan Marsden added a comment -

            Dongsheng - any chance you have time to review this as well? - thanks!

            Show
            danmarsden Dan Marsden added a comment - Dongsheng - any chance you have time to review this as well? - thanks!
            danmarsden Dan Marsden made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            Peer reviewer dongsheng
            Hide
            danmarsden Dan Marsden added a comment -

            Ankit - looking good so far - just a quick think - we don't use "IN" directly in SQL - we use a helper function - see G7 on this page for an example:
            http://docs.moodle.org/dev/DB_layer_2.0_migration_docs#The_golden_changes

            Show
            danmarsden Dan Marsden added a comment - Ankit - looking good so far - just a quick think - we don't use "IN" directly in SQL - we use a helper function - see G7 on this page for an example: http://docs.moodle.org/dev/DB_layer_2.0_migration_docs#The_golden_changes
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            removed the IN from sql statments

            Show
            ankit_frenz Ankit Agarwal added a comment - removed the IN from sql statments
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Ankit

            I just learned today that the mform function "set_data" must always come before a get_data call for the same form - (probably another bug in the other code too) - can you please make the change to this patch and check the other scorm report plugin to see if you need to do something similar there and open a new bug to fix it there as well?

            also - something else I learned today.... it would be nice to add course context to the 3rd param of all uses of format_string - new code is "supposed" to do this to help with performance etc eg:
            $formattextoptions = array('context'=>get_context_instance(CONTEXT_COURSE, $courseid));
            $headers[]= format_string($sco->title, true, $formattextoptions);
            (don't do this for any old code - just for the new interactions plugin for now)

            Then can you please add testing instructions to this bug for the QA team - linking to a SCORM package that produces interactions that can be shown by this report!

            thanks heaps!

            Show
            danmarsden Dan Marsden added a comment - thanks Ankit I just learned today that the mform function "set_data" must always come before a get_data call for the same form - (probably another bug in the other code too) - can you please make the change to this patch and check the other scorm report plugin to see if you need to do something similar there and open a new bug to fix it there as well? also - something else I learned today.... it would be nice to add course context to the 3rd param of all uses of format_string - new code is "supposed" to do this to help with performance etc eg: $formattextoptions = array('context'=>get_context_instance(CONTEXT_COURSE, $courseid)); $headers[]= format_string($sco->title, true, $formattextoptions); (don't do this for any old code - just for the new interactions plugin for now) Then can you please add testing instructions to this bug for the QA team - linking to a SCORM package that produces interactions that can be shown by this report! thanks heaps!
            ankit_frenz Ankit Agarwal made changes -
            Testing Instructions Please install this Scorm package
            http://moodle.org/mod/data/view.php?d=50&rid=1655&filter=1

            log in as a student and submit some attempts.
            log back in as a teacher
            Visit Scorm package>report>Interaction report
            make sure the Reports page operates as expected - try downloading Excel, ODS files, try deleting attempts,updating form options.


            ankit_frenz Ankit Agarwal made changes -
            Testing Instructions Please install this Scorm package
            http://moodle.org/mod/data/view.php?d=50&rid=1655&filter=1

            log in as a student and submit some attempts.
            log back in as a teacher
            Visit Scorm package>report>Interaction report
            make sure the Reports page operates as expected - try downloading Excel, ODS files, try deleting attempts,updating form options.


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

            log in as a student and submit some attempts.
            log back in as a teacher
            Visit Scorm package>report>Interaction report
            make sure the Reports page operates as expected - try downloading Excel, ODS files, try deleting attempts,updating form options.
            The specific addition of this plugin is to display student's response and correct answers.Please make sure those columns are properly populated.



            Hide
            danmarsden Dan Marsden added a comment -

            Hi Ankit,

            just noticed this on the interactions report:
            Notice: Undefined variable: i in /mod/scorm/report/reportlib.php on line 63

            also - I can't remember where the "Preferences for this report" settings were in the main lib or in the plugin - can we strip out the setting "display tracks" and make it always display tracks?

            thanks!

            Show
            danmarsden Dan Marsden added a comment - Hi Ankit, just noticed this on the interactions report: Notice: Undefined variable: i in /mod/scorm/report/reportlib.php on line 63 also - I can't remember where the "Preferences for this report" settings were in the main lib or in the plugin - can we strip out the setting "display tracks" and make it always display tracks? thanks!
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Dan,
            Thanks for the feedback.
            Set the interaction plugin to always display tracks.
            Fixed the undefined variable issue.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Dan, Thanks for the feedback. Set the interaction plugin to always display tracks. Fixed the undefined variable issue. Thanks
            Hide
            danmarsden Dan Marsden added a comment -

            Hi Ankit - looking good!

            can you please add "completion date" as one of the fields in the report - just after the "Last accessed on" field?

            thanks!

            Show
            danmarsden Dan Marsden added a comment - Hi Ankit - looking good! can you please add "completion date" as one of the fields in the report - just after the "Last accessed on" field? thanks!
            danmarsden Dan Marsden made changes -
            Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
            danmarsden Dan Marsden made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Hide
            danmarsden Dan Marsden added a comment -

            Note to integrator - this is a new feature so for master ONLY

            Show
            danmarsden Dan Marsden added a comment - Note to integrator - this is a new feature so for master ONLY
            samhemelryk Sam Hemelryk made changes -
            Currently in integration Yes
            stronk7 Eloy Lafuente (stronk7) made changes -
            Integrator stronk7
            nebgor Aparup Banerjee made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator stronk7 nebgor
            Hide
            nebgor Aparup Banerjee added a comment -

            hm noting something that doesn't seem to make sense here. interaction report : user answered EVERY answer wrong === status 'incomplete' but took 24 seconds :-D

            Show
            nebgor Aparup Banerjee added a comment - hm noting something that doesn't seem to make sense here. interaction report : user answered EVERY answer wrong === status 'incomplete' but took 24 seconds :-D
            Hide
            nebgor Aparup Banerjee added a comment - - edited

            noticed a few minor things: white space @ mod/scorm/report/interactions/report.php lines 77 & 514 and exported files had 'attempt' column --> 'Attempt'

            • setting Eloy back as integrator. @,@
            Show
            nebgor Aparup Banerjee added a comment - - edited noticed a few minor things: white space @ mod/scorm/report/interactions/report.php lines 77 & 514 and exported files had 'attempt' column --> 'Attempt' setting Eloy back as integrator. @,@
            nebgor Aparup Banerjee made changes -
            Status Integration review in progress [ 10004 ] Waiting for integration review [ 10010 ]
            nebgor Aparup Banerjee made changes -
            Integrator nebgor stronk7
            Hide
            danmarsden Dan Marsden added a comment -

            Thanks Aparup,

            Whitespace fixed and pushed into my repo.

            Case of "attempt" can be classed as "new" bug as this is copied from standard "basic" report and is a problem there too - I'll check existing uses of the 'attempt' string in SCORM and change case of existing string or create a new one if lower case is needed in some places.

            Show
            danmarsden Dan Marsden added a comment - Thanks Aparup, Whitespace fixed and pushed into my repo. Case of "attempt" can be classed as "new" bug as this is copied from standard "basic" report and is a problem there too - I'll check existing uses of the 'attempt' string in SCORM and change case of existing string or create a new one if lower case is needed in some places.
            Hide
            danmarsden Dan Marsden added a comment -

            .although not sure about your comment about 24 seconds - did it take you longer/shorter to complete the SCORM?

            Show
            danmarsden Dan Marsden added a comment - .although not sure about your comment about 24 seconds - did it take you longer/shorter to complete the SCORM?
            danmarsden Dan Marsden made changes -
            Link This issue has been marked as being related by MDL-30041 [ MDL-30041 ]
            Hide
            danmarsden Dan Marsden added a comment -

            MDL-30041 created for string update and integration request filed with change (won't be picked up until next week I guess)

            Show
            danmarsden Dan Marsden added a comment - MDL-30041 created for string update and integration request filed with change (won't be picked up until next week I guess)
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            It looks, great, hope it works, lol (sure it does!). Some really minor-minor comments:

            • I think we need to use proper component name (plugin type + plugin name) in some places. If I'm not wrong this is 'scormreport_interactions' and as such should be used in @package / @subpackage docs, surely also in the settings form name (not need to be path-based, but component-based) and surely other bits.
            • There are a lot of incorrectly spaced operators here and there, note that coding style says we need to add one whitespace char both before and after any comparison/assignment operator. Same applies to "for" and "if" (space after them).
            • versions @ version.php are way too old, given current date... it would be better to raise them a bit.
            • get_scorm_question_count() needs documentation. Also, note it's a double-nested for loop, fetching fields from DB 1 by 1, and I guess it's a clear candidate to scale really, really bad, perhaps we should be accessing to that information in an alternative (optimized) way ? Once again, think about thousands...

            So, given that, I think this requires a bit of polishing (simple coding standards) and some more thinking about how the report will behave under load (big numbers). Otherwise, it's looking really promising, thanks!

            Reopening as rejected, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - It looks, great, hope it works, lol (sure it does!). Some really minor-minor comments: I think we need to use proper component name (plugin type + plugin name) in some places. If I'm not wrong this is 'scormreport_interactions' and as such should be used in @package / @subpackage docs, surely also in the settings form name (not need to be path-based, but component-based) and surely other bits. There are a lot of incorrectly spaced operators here and there, note that coding style says we need to add one whitespace char both before and after any comparison/assignment operator. Same applies to "for" and "if" (space after them). Double levels like: https://github.com/danmarsden/moodle/compare/master...master_MDL-28277#L1R385 (first one "if" and later one "foreach" are, usually unnecessary. Also, if they can be, say, more than 1000, I'd be using get_recordset() instead of get_records(), to avoid being memory-bound problems. Some lines seems incorrectly aligned (tabs there yet?): https://github.com/danmarsden/moodle/compare/master...master_MDL-28277#L1R441 versions @ version.php are way too old, given current date... it would be better to raise them a bit. get_scorm_question_count() needs documentation. Also, note it's a double-nested for loop, fetching fields from DB 1 by 1, and I guess it's a clear candidate to scale really, really bad, perhaps we should be accessing to that information in an alternative (optimized) way ? Once again, think about thousands... So, given that, I think this requires a bit of polishing (simple coding standards) and some more thinking about how the report will behave under load (big numbers). Otherwise, it's looking really promising, thanks! Reopening as rejected, ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Integration review in progress [ 10004 ] Reopened [ 4 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes
            Hide
            danmarsden Dan Marsden added a comment -

            Hi Eloy - thanks for the great review - Ankit do you have time to sort these out - feel free to bounce back to me if not.

            Ankit - FYI the double levels Eloy mentions were needed in 1.9 (where you copied the code from) but in 2.0 as Eloy mentions all get_records style calls return an empty object - in 1.9 some returned false, null etc and a foreach would throw an undefined var error.

            Show
            danmarsden Dan Marsden added a comment - Hi Eloy - thanks for the great review - Ankit do you have time to sort these out - feel free to bounce back to me if not. Ankit - FYI the double levels Eloy mentions were needed in 1.9 (where you copied the code from) but in 2.0 as Eloy mentions all get_records style calls return an empty object - in 1.9 some returned false, null etc and a foreach would throw an undefined var error.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Eloy & Dan,
            Thanks for the review!
            I will try to fix those issues today!
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Eloy & Dan, Thanks for the review! I will try to fix those issues today! Thanks
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Following Changes made

            • Adjusted to Moodle coding style
            • Removed double levels
            • Alignment fixed
            • version.php Updated
            • get_Scorm_question_count function optimized (now it makes only one call to DB!)

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Following Changes made Adjusted to Moodle coding style Removed double levels Alignment fixed version.php Updated get_Scorm_question_count function optimized (now it makes only one call to DB!) Thanks
            ankit_frenz Ankit Agarwal made changes -
            Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            danmarsden Dan Marsden made changes -
            Priority Minor [ 4 ] Major [ 3 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This has been integrated, thanks!

            I've added one extra commit fixing 1 wrong whitespace remaining + some MOODLE_INTERNAL uses/misses.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This has been integrated, thanks! I've added one extra commit fixing 1 wrong whitespace remaining + some MOODLE_INTERNAL uses/misses.
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Fix Version/s 2.2 [ 10656 ]
            Fix Version/s DEV backlog [ 10464 ]
            rwijaya Rossiani Wijaya made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester rwijaya
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Deleting attempt on interaction report produce this error:
            Invalid array parameter detected in required_param(): attemptid
            line 581 of /lib/moodlelib.php: call to debugging()
            line 40 of /mod/scorm/report/interactions/report.php: call to optional_param()
            line 85 of /mod/scorm/report.php: call to scorm_interactions_report->display()

            I also notice that, attempts can't be greater than 3. In other words, when student has 3 attempts, student won't be able to add another attempt, instead it will set attempt 3 as incomplete and overwrite the attempt.

            Failing test.

            Show
            rwijaya Rossiani Wijaya added a comment - Deleting attempt on interaction report produce this error: Invalid array parameter detected in required_param(): attemptid line 581 of /lib/moodlelib.php: call to debugging() line 40 of /mod/scorm/report/interactions/report.php: call to optional_param() line 85 of /mod/scorm/report.php: call to scorm_interactions_report->display() I also notice that, attempts can't be greater than 3. In other words, when student has 3 attempts, student won't be able to add another attempt, instead it will set attempt 3 as incomplete and overwrite the attempt. Failing test.
            rwijaya Rossiani Wijaya made changes -
            Status Testing in progress [ 10011 ] Problem during testing [ 10007 ]
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Rosie - that's the change to optional_param_array - forgot about that one - Ankit can you please fix that up?

            number of attempts allowed can be controlled by a number of different processes - in any case this is not relevant to the display of the attempts but either the SCORM package itself or other settings in the SCORM.

            Show
            danmarsden Dan Marsden added a comment - thanks Rosie - that's the change to optional_param_array - forgot about that one - Ankit can you please fix that up? number of attempts allowed can be controlled by a number of different processes - in any case this is not relevant to the display of the attempts but either the SCORM package itself or other settings in the SCORM.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Rosie,
            As Dan said the second part is dependent on Scorm and other settings
            And here is a fix for the first part
            https://github.com/ankitagarwal/moodle/commit/f9ab70a9084820b4168396185d8590e49fef1fd9

            I will see if that can pulled up
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Rosie, As Dan said the second part is dependent on Scorm and other settings And here is a fix for the first part https://github.com/ankitagarwal/moodle/commit/f9ab70a9084820b4168396185d8590e49fef1fd9 I will see if that can pulled up Thanks
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Problem during testing [ 10007 ] Integration review in progress [ 10004 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Re-integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Re-integrated, thanks!
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            rwijaya Rossiani Wijaya made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Hide
            rwijaya Rossiani Wijaya added a comment -

            This is working great.

            Thanks Dan and Ankit.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - This is working great. Thanks Dan and Ankit. Test passed.
            rwijaya Rossiani Wijaya made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FYI, just detected this was not declared as "core" plugin yet so I did it, also adding $plugin->component to version.php (8f1a0d21cbe8ad8868ff483022966d3516acbd7f)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FYI, just detected this was not declared as "core" plugin yet so I did it, also adding $plugin->component to version.php (8f1a0d21cbe8ad8868ff483022966d3516acbd7f)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks!

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 15/Nov/11
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            adding qa_test_required tag as this needs QA tests.

            Show
            ankit_frenz Ankit Agarwal added a comment - adding qa_test_required tag as this needs QA tests.
            ankit_frenz Ankit Agarwal made changes -
            Labels qa_test_required

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/Dec/11