Moodle

pretty sure this is because the params passed aren't encoded correctly - most browsers don't care, but it seems IE does!

updated code - using full player.php page to load popup instead which appears to fix issue.

Linking to display issues that need to be fixed before we can push this into core.

I wonder if we should check if SCORM TOC is disabled and if so - don't use the full player but use a standard object/frame include.

note: the patch for master on this also includes the patch for MDL-28493 to help avoid conflicts during integration. MDL-28493 must be merged first.

Note: I'm only pushing in a fix for 2.X branches at this stage - If someone else wants to take my patch and develop a 1.9 version I'd be happy to review it for inclusion but I have no plans to do it myself (unless someone pays me for the time to do it!)

This patch also disables the TOC and navbar for existing SCORMs that open in a new window as if these appeared for existing scorms in a new window it would probably cause great confusion!

note to integrator - I've just updated those branches with a savepoint function typo fix like the attached bug - whoops!

rebased against latest branches

Ops, failing integration of this because:

1) Critical: I get some obvious duplicate code, for example @ mod/scorm/player.php - all the optional_param() seem duplicate. Haven't verified the rest.
2) Not critical: Upgrade script, perhaps better if limiting the fields retrieved from DB to the used ones and/or use recordset? Imagine site with 20.000 scorms in popups and big summaries or so. I know it's an extreme situation, but...
3) Trivial: Not knowing much about CSS... it surprised me the use of "font-size: medium", I thought we were using always em or friends.
4) Question: 20_STABLE and 21_STABLE have the same version? Does that mean that are interchangeable? In other words... does the 21_STABLE version work if installed into 20_STABLE site?
5) Side comment: While reviewing the code, it seems that the master version is 99% about to fix the popup thingy. Seems 100% unrelated with the title of the issue and that annoyed me a bit.
6) Testing instructions of some type would be great if possible (I know, some things are hardly testable... but the popup thing in code seems to be).

Thanks and ciao

1 - not sure what duplicate you mean here eloy the optional_params in player.php are all for different things.
2 - Update script - sure, will tidy this up a bit.
3 - ok will update
4 - 20_stable and 21_stable branches are both very close so the patch should be interchangeable if you want - not sure why you're asking this.
5 - the popup thing has changed because of the IE 7 madness - we can't load the SCO in a new window and keep the api in the parent window as for some stupid reason IE 7 fails intermittently.
6 - fair enough - the IE 7 issue is a real pain to reproduce but the pop-ups can be tested... any chance of getting this fixed this week if I make the above changes today?

player.php
$mode == mode the SCORM should be loaded in - normal, review, browse etc. (as per SCORM Spec)
$newattempt == the user has selected to start a new attempt.
it shuold be obvious what the other optional_params are used for.

updated as requested and have re-submitted. (still not sure about your duplicate optional_param comment)

thanks.

Take a look to: https://github.com/danmarsden/moodle/blob/f9ce4a21c06d50711c7ecf00568be76c4dc74259/mod/scorm/player.php

or I'm blind or I'm seeing a lot of optional/required_param() dupe lines there. Sure it's my eyes getting tired lol... so better I go to sleep some hours... see you tomorrow, and thanks!

And yes, I'll re-try this tomorrow...ciao

whoops! - I was only looking at the master branch just now - yeah that's a bad copy/paste... I'll check the full patches for 2.0Stable and 2.1Stable again! - sorry bout that!

pushed updated code for 2.0 and 2.1 stable branches - I've also removed a lot of the whitespace clean up stuff to hopefully make it easier to review! - sorry for pushing such a messy commit!

thanks!

Thanks Dan, re-looking to this now.

About 4) in my original list... what I was asking is that, as far as 20_STABLE, 21_STABLE and master branches, all them have "$module->requires = 2010080300;" then, potentially, it's possible to run the 21_STABLE module into one 20_STABLE site (and also one master module into one 20 or 21_STABLE site).

And I was thinking that perhaps, although improbable... that was not a good thing. Just that. As far as you keep 20_STABLE and 21_STABLE fully in sync, np here, but if they diverge, using some new stuff only present in 21_STABLE... you should update that "$module->requires" to point to newer release of Moodle.

Not critical at all, it just surprised me you were using the same version numbers in the 2 branches. Ciao

reviewing this again....

cool - thanks Eloy - 2.0 and 2.1 should be in sync - anything different will only be in master, anything pushed to 2.1stable will also go to 2.0stable... I figure anything that can't be pushed to 2.0stable shouldn't go to 2.1stable either so I'll probably keep them both in sync for a while.

the only stuff I've been pushing into stable branches for SCORM are bug fixes excluding any SCORM 2004 stuff which is only going in master.

thanks heaps!

Integrated, thanks!

Note for tester: Plz, test this under some STABLE branch.

thanks Eloy!

Thanks guys, passing this test now.

Cheers
Sam

Hi Dan,

Sorry for double-posting (here and in the forums) - I should rather have posted this only here, probably. I applied your patch to 2.0.4, and it seems to fix IE7 - awesome! However, the same popup still freezes in IE8. Do you have any insight? Maybe I applied the path wrongly...? Here's what I did:

1. Clone the git repo at git://github.com/danmarsden/moodle.git as a standalone set of files (not on top of my Moodle instance)
2. Checkout MOODLE_20_STABLE branch and use it to upgrade my Moodle instance, by replacing everything with the new set of files, then overlaying my custom auth and theme folders, and config.php
3. Checkout the m20_MDL-28295 branch (your fix) to the standalone location, and copy the entire mod/scorm/ directory to my Moodle instance

This appears to have fixed the popup issue in IE7, but in IE8, the popup window still hangs at load time, crashing the browser.

Thanks for any insight you can provide!

Hi Gabe,

good to hear! - I haven't done any testing of IE 8 yet but we do have this:
http://tracker.moodle.org/browse/MDL-28551

try the fix posted in the forum and see if that resolves it for you? - if not we'll need lots of information on how to reproduce it - including SCORM object, IIS/Apache (what versions) php version, what php extensions are installed etc etc.

thanks!

Sent upstream and closing, many thanks!