Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major 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

            Hide
            Dan Marsden added a comment -

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

            Show
            Dan Marsden added a comment - Dongsheng - any chance you have time to review this as well? - thanks!
            Hide
            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
            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 Agarwal added a comment -

            removed the IN from sql statments

            Show
            Ankit Agarwal added a comment - removed the IN from sql statments
            Hide
            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
            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!
            Hide
            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
            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 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 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
            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
            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!
            Hide
            Dan Marsden added a comment -

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

            Show
            Dan Marsden added a comment - Note to integrator - this is a new feature so for master ONLY
            Hide
            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
            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
            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
            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. @,@
            Hide
            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
            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
            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
            Dan Marsden added a comment - .although not sure about your comment about 24 seconds - did it take you longer/shorter to complete the SCORM?
            Hide
            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
            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)
            Hide
            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
            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
            Hide
            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
            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 Agarwal added a comment -

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

            Show
            Ankit Agarwal added a comment - Hi Eloy & Dan, Thanks for the review! I will try to fix those issues today! Thanks
            Hide
            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 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
            Hide
            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
            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.
            Hide
            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
            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.
            Hide
            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
            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 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 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
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Re-integrated, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Re-integrated, thanks!
            Hide
            Rossiani Wijaya added a comment -

            This is working great.

            Thanks Dan and Ankit.

            Test passed.

            Show
            Rossiani Wijaya added a comment - This is working great. Thanks Dan and Ankit. Test passed.
            Hide
            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
            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
            Eloy Lafuente (stronk7) added a comment -

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

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao
            Hide
            Ankit Agarwal added a comment -

            adding qa_test_required tag as this needs QA tests.

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

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: