Moodle

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

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

removed the IN from sql statments

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!

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!

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

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!

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

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

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. @,@

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.

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

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

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

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.

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

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

This has been integrated, thanks!

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

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.

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.

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

Re-integrated, thanks!

This is working great.

Thanks Dan and Ankit.

Test passed.

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

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

Ciao