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

          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: