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
    • Rank:
      16938

      Description

      A plugin to display interactions properly

        Issue Links

          Activity

          Ankit Agarwal created issue -
          Ankit Agarwal made changes -
          Field Original Value New Value
          Fix Version/s DEV backlog [ 10464 ]
          Ankit Agarwal made changes -
          Summary Design of a interraction report plugin Design of a interaction report plugin
          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
          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!
          Dan Marsden made changes -
          Status Open [ 1 ] Waiting for peer review [ 10012 ]
          Peer reviewer dongsheng
          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!
          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 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
          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!
          Dan Marsden made changes -
          Status Waiting for peer review [ 10012 ] Development in progress [ 3 ]
          Dan Marsden made changes -
          Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
          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
          Sam Hemelryk made changes -
          Currently in integration Yes
          Eloy Lafuente (stronk7) made changes -
          Integrator stronk7
          Aparup Banerjee made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator stronk7 nebgor
          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. @,@
          Aparup Banerjee made changes -
          Status Integration review in progress [ 10004 ] Waiting for integration review [ 10010 ]
          Aparup Banerjee made changes -
          Integrator nebgor stronk7
          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?
          Dan Marsden made changes -
          Link This issue has been marked as being related by MDL-30041 [ MDL-30041 ]
          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)
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Status Integration review in progress [ 10004 ] Reopened [ 4 ]
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes
          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
          Ankit Agarwal made changes -
          Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Dan Marsden made changes -
          Priority Minor [ 4 ] Major [ 3 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          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.
          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 ]
          Rossiani Wijaya made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Tester rwijaya
          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.
          Rossiani Wijaya made changes -
          Status Testing in progress [ 10011 ] Problem during testing [ 10007 ]
          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
          Eloy Lafuente (stronk7) made changes -
          Status Problem during testing [ 10007 ] Integration review in progress [ 10004 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Re-integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Re-integrated, thanks!
          Eloy Lafuente (stronk7) made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Rossiani Wijaya made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          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.
          Rossiani Wijaya made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          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
          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 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.
          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: