Details

      Description

      Mayank has developed a large patch that includes fixes for a wide range of SCORM 2004 issues - it includes a rewrite of scorm_get_toc which may affect SCORM 1.2 packages - We will submit the code as part of this sub-task

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Marsden added a comment -

            would be good to get someone else's eyes on this code again.

            NOTE: most of the functions in the new file scorm13_lib.php have been moved from the old sequencinglib.php file - most of these functions are a mess and this patch only tidies up some of the issues - just because we've moved the functions we shouldn't have to fix everything that's wrong with them (yet)

            Show
            Dan Marsden added a comment - would be good to get someone else's eyes on this code again. NOTE: most of the functions in the new file scorm13_lib.php have been moved from the old sequencinglib.php file - most of these functions are a mess and this patch only tidies up some of the issues - just because we've moved the functions we shouldn't have to fix everything that's wrong with them (yet)
            Hide
            Dan Marsden added a comment -

            as component maintainer I have peer reviewed Mayanks code and made a few changes - passing this up for integration review now as it needs to land before freeze and I'm taking some time off over the next few weeks.

            Note to integrator - some of the functions that Mayank has shifted in this patch are old/nasty functions that still need further work - before failing due to nasty/bad code please check old code to see if the issue already exists.

            I also think there's still further work to be done here - I'd still like to see further refactoring that could be good with scorm_get_toc but this is a good step forward and a good improvement and allows us to pass some more ADL tests so should be allowed for integration.

            thanks!

            Show
            Dan Marsden added a comment - as component maintainer I have peer reviewed Mayanks code and made a few changes - passing this up for integration review now as it needs to land before freeze and I'm taking some time off over the next few weeks. Note to integrator - some of the functions that Mayank has shifted in this patch are old/nasty functions that still need further work - before failing due to nasty/bad code please check old code to see if the issue already exists. I also think there's still further work to be done here - I'd still like to see further refactoring that could be good with scorm_get_toc but this is a good step forward and a good improvement and allows us to pass some more ADL tests so should be allowed for integration. thanks!
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            I've found this issue after reading a comment of yours posted in the Community: really a big work, kudos to the GSOC and Mayank

            I'll try to test it, a quick look (no run just read) at the commit diff:

            1. function declaration: it should be a_function(params) and not a_function (params). Maybe a run of code checker will help in finding some other issues I've seen with my eyes
            2. https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L10R456: 6 lines of code for debugging. It could be useful for the main stream too but not written that way. Suggestion: deletion
            3. https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L10R1604: not sure if an asset should miss the "status icon", being here more "untracked" than reporting that kind of "status". Need a run to look at the overall graphic effect to see if there will be a missing about info at the user side
            4. https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L10R1604: identation. Many "issues" in mod/scorm/module.js (e.g.: https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L11R294, ...)
            5. https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L11R514: identation

            HTH,
            Matteo

            Show
            Matteo Scaramuccia added a comment - Hi Dan, I've found this issue after reading a comment of yours posted in the Community : really a big work, kudos to the GSOC and Mayank I'll try to test it, a quick look (no run just read) at the commit diff: function declaration: it should be a_function(params) and not a_function (params) . Maybe a run of code checker will help in finding some other issues I've seen with my eyes https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L10R456: 6 lines of code for debugging. It could be useful for the main stream too but not written that way. Suggestion: deletion https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L10R1604: not sure if an asset should miss the "status icon", being here more "untracked" than reporting that kind of "status". Need a run to look at the overall graphic effect to see if there will be a missing about info at the user side https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L10R1604: identation. Many "issues" in mod/scorm/module.js (e.g.: https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L11R294 , ...) https://github.com/danmarsden/moodle/compare/master...master_MDL-35418#L11R514: identation HTH, Matteo
            Hide
            Dan Marsden added a comment -

            thanks Matteo - re 1/4/5 I'd avoided running codechecker on all this code as most SCORM code just doesn't generally comply and I didn't want to re-work all the code (but all new code should comply) - I'll take another look at module.js and may just go through the whole file

            missed the code you saw in point 2 - good point, it should be removed.

            re point 3 - what do you think we should display for assets as the status icon?

            Show
            Dan Marsden added a comment - thanks Matteo - re 1/4/5 I'd avoided running codechecker on all this code as most SCORM code just doesn't generally comply and I didn't want to re-work all the code (but all new code should comply) - I'll take another look at module.js and may just go through the whole file missed the code you saw in point 2 - good point, it should be removed. re point 3 - what do you think we should display for assets as the status icon?
            Hide
            Dan Marsden added a comment -

            1 - haven't found the specific function you meant? - is this an existing function that is wrong or a new one that was added? -
            2 - code deleted, thanks.
            3 - This is really expected behaviour at this stage - if you think we need to improve it further lets do that on a separate tracker issue so we don't prevent this from being integrated.

            4/5 were naughty tabs that weren't picked up by codechecker - have converted those to spaces.

            thanks!

            Show
            Dan Marsden added a comment - 1 - haven't found the specific function you meant? - is this an existing function that is wrong or a new one that was added? - 2 - code deleted, thanks. 3 - This is really expected behaviour at this stage - if you think we need to improve it further lets do that on a separate tracker issue so we don't prevent this from being integrated. 4/5 were naughty tabs that weren't picked up by codechecker - have converted those to spaces. thanks!
            Hide
            Dan Marsden added a comment -

            re point 1 - I think these may just be pre-existing functions that were shifted - see my comment above "some of the functions that Mayank has shifted in this patch are old/nasty functions that still need further work - before failing due to nasty/bad code please check old code to see if the issue already exists."

            Show
            Dan Marsden added a comment - re point 1 - I think these may just be pre-existing functions that were shifted - see my comment above "some of the functions that Mayank has shifted in this patch are old/nasty functions that still need further work - before failing due to nasty/bad code please check old code to see if the issue already exists."
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            1. I mean: the extra space between the name of the fn and the params declaration, nothing more than coding style. That means: real code is fine
            3. Something like a greyed icon miming that the tracking couldn't be there being that item declared as an "asset": I think that this icon has been coded there as completed also because of in the past course vendors "sometimes" flagged a real sco as an asset. BTW, I totally agree: something that could be eventually done as an optional improvement, ex-post: asset.gif is fine

            Show
            Matteo Scaramuccia added a comment - Hi Dan, 1. I mean: the extra space between the name of the fn and the params declaration, nothing more than coding style. That means: real code is fine 3. Something like a greyed icon miming that the tracking couldn't be there being that item declared as an " asset ": I think that this icon has been coded there as completed also because of in the past course vendors "sometimes" flagged a real sco as an asset . BTW, I totally agree: something that could be eventually done as an optional improvement, ex-post : asset.gif is fine
            Hide
            Dan Marsden added a comment -

            cool - as far as I could see - (1) is from existing code that has been shifted - I definately want to clean this up but running through all coding guidelines in this patch for all files/functions in existing code is likely to make it harder for the integrators to review the patch.

            feel free to open a new bug to cover (3) - thanks!

            Show
            Dan Marsden added a comment - cool - as far as I could see - (1) is from existing code that has been shifted - I definately want to clean this up but running through all coding guidelines in this patch for all files/functions in existing code is likely to make it harder for the integrators to review the patch. feel free to open a new bug to cover (3) - thanks!
            Hide
            Aparup Banerjee added a comment -

            Hi all,
            i've been looking through this and couldn't help but wonder about adding phpunit tests here with this change. This would really help with the integration of this seeing that this is a lot of refactoring and would prevent many possible regressions (if any).

            so i think its wise here to add in some tests, perhaps in time for code freeze.

            ps: i hope phpunit tests make sense in the context of scorm and the changes here. i see there are some in mod/scorm/tests/format_duration_test.php.

            Show
            Aparup Banerjee added a comment - Hi all, i've been looking through this and couldn't help but wonder about adding phpunit tests here with this change. This would really help with the integration of this seeing that this is a lot of refactoring and would prevent many possible regressions (if any). so i think its wise here to add in some tests, perhaps in time for code freeze. ps: i hope phpunit tests make sense in the context of scorm and the changes here. i see there are some in mod/scorm/tests/format_duration_test.php.
            Hide
            Dan Marsden added a comment -

            Hi Aparup - SCORM is already coverered really well by tests - the ADL tests. I'm not going to have time to work on unit tests before code freeze - (nor do I think I should have to.) much of my time on SCORM is spent as a volunteer. If HQ wants to fund the time for extra work on unit tests that's fine, but it should not prevent this from being integrated IMO.

            Show
            Dan Marsden added a comment - Hi Aparup - SCORM is already coverered really well by tests - the ADL tests. I'm not going to have time to work on unit tests before code freeze - (nor do I think I should have to.) much of my time on SCORM is spent as a volunteer. If HQ wants to fund the time for extra work on unit tests that's fine, but it should not prevent this from being integrated IMO.
            Hide
            Aparup Banerjee added a comment -

            Thanks for the feedback Dan, now that i think about it, We've actually got David Monallo who's working with tests and selenium here at HQ, i'll ping him tomorrow about trying to get the ADL tests integrated into those tests so that we can run them more easily.

            ps: actually probably best if he runs the ADL tests for this MDL too

            pps: setting David as tester (MDL-26912 has a test harness that uses selenium wink )

            Show
            Aparup Banerjee added a comment - Thanks for the feedback Dan, now that i think about it, We've actually got David Monallo who's working with tests and selenium here at HQ, i'll ping him tomorrow about trying to get the ADL tests integrated into those tests so that we can run them more easily. ps: actually probably best if he runs the ADL tests for this MDL too pps: setting David as tester ( MDL-26912 has a test harness that uses selenium wink )
            Hide
            Aparup Banerjee added a comment -

            i swear i saw some doc pointing to selenium + test harness... can't find it now.

            Show
            Aparup Banerjee added a comment - i swear i saw some doc pointing to selenium + test harness... can't find it now.
            Hide
            Dan Marsden added a comment -

            selenium/test harness is probably not likely to work straight off the bat... especially as the majority of the SCORM 2004 tests fail and this only allows us to pass 2.
            but here's the link:
            http://docs.moodle.org/dev/SCORM_Test_Harness

            I'd suggest at this stage it would be better to run the tests manually as per docs I've linked above, once you have a browser set up that will run them manually you can start experimenting with the selenium tests (which may need some updating)

            Show
            Dan Marsden added a comment - selenium/test harness is probably not likely to work straight off the bat... especially as the majority of the SCORM 2004 tests fail and this only allows us to pass 2. but here's the link: http://docs.moodle.org/dev/SCORM_Test_Harness I'd suggest at this stage it would be better to run the tests manually as per docs I've linked above, once you have a browser set up that will run them manually you can start experimenting with the selenium tests (which may need some updating)
            Hide
            Dan Marsden added a comment -

            probably a good idea to point out that the ADL test packages loaded on qa.moodle.net might not be quite right either - best to use fresh versions of those files (links to download packages should be in the docs I mentioned.)

            Show
            Dan Marsden added a comment - probably a good idea to point out that the ADL test packages loaded on qa.moodle.net might not be quite right either - best to use fresh versions of those files (links to download packages should be in the docs I mentioned.)
            Hide
            Aparup Banerjee added a comment -

            thanks. those are the ones i used (docs) .. using.. will integrate this once i test this.

            Show
            Aparup Banerjee added a comment - thanks. those are the ones i used (docs) .. using.. will integrate this once i test this.
            Hide
            Aparup Banerjee added a comment -

            Hi Dan,
            From http://docs.moodle.org/dev/SCORM_1.2_compliance_test_instructions :-
            "Upload the 2 SCORM test objects
            The SCORM objects LMSTestCourse01.zip and LMSTestCourse02.zip are contained within the 1.2 Test suite available from http://www.adlnet.gov"

            i couldn't find any '1.2' Test suite available at www.adlnet.gov while searching the software downloads section. Perhaps if you could make those specific zips available here it would be easier to test the ADL 1.2 tests.

            Regarding the ADL 2004 tests CM-01 and CM-02a, got them from the 'test' packages' link in http://docs.moodle.org/dev/SCORM_2004_compliance_test_instructions :
            on IE9 (windows VM) with patch applied i just get a list of three activities when entering the scorm activity. (both CM-01 and CM-02a).
            Prior to this i was getting pop ups about 'browser not tested' in ie until i upgraded java plugin that browser requested.
            i tried on Safari (osx) just to see what was up and both the CM-01 and CM-02a popup with a certificate that is expired (so i couldn't continue from there). The scorn menu and navigation buttons did only render on the safari trial (not on i.e.)

            After all this i'm really wondering what can be done to make testing scorm easier in general.

            aside from testing difficulties, i couldn't find anything wrong with the code, which is why i tried testing it.

            on the commits, i noticed a commit for MDL-7068 ( meta of MDL-28740 and MDL-28741) so i'm thinking the tests aren't working perhaps because i'm actually testing if the test works.

            anyway, we really need to sort out

            • testing this and testing the test works too.
            • commit MDL prefixes here, MDL-7068 is a meta and seems unlikely that that will be closed once this patch is integrated.

            ps: added rosie here as she seems involved in testing other MDLs.

            Show
            Aparup Banerjee added a comment - Hi Dan, From http://docs.moodle.org/dev/SCORM_1.2_compliance_test_instructions :- "Upload the 2 SCORM test objects The SCORM objects LMSTestCourse01.zip and LMSTestCourse02.zip are contained within the 1.2 Test suite available from http://www.adlnet.gov " i couldn't find any '1.2' Test suite available at www.adlnet.gov while searching the software downloads section. Perhaps if you could make those specific zips available here it would be easier to test the ADL 1.2 tests. Regarding the ADL 2004 tests CM-01 and CM-02a, got them from the 'test' packages' link in http://docs.moodle.org/dev/SCORM_2004_compliance_test_instructions : on IE9 (windows VM) with patch applied i just get a list of three activities when entering the scorm activity. (both CM-01 and CM-02a). Prior to this i was getting pop ups about 'browser not tested' in ie until i upgraded java plugin that browser requested. i tried on Safari (osx) just to see what was up and both the CM-01 and CM-02a popup with a certificate that is expired (so i couldn't continue from there). The scorn menu and navigation buttons did only render on the safari trial (not on i.e.) After all this i'm really wondering what can be done to make testing scorm easier in general. aside from testing difficulties, i couldn't find anything wrong with the code, which is why i tried testing it. on the commits, i noticed a commit for MDL-7068 ( meta of MDL-28740 and MDL-28741 ) so i'm thinking the tests aren't working perhaps because i'm actually testing if the test works. anyway, we really need to sort out testing this and testing the test works too. commit MDL prefixes here, MDL-7068 is a meta and seems unlikely that that will be closed once this patch is integrated. ps: added rosie here as she seems involved in testing other MDLs.
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Dan Marsden added a comment -

            testing SCORM with ADL tests is hard. - I'm not sure what I can do to improve this.

            you need to run the tests with the test harness(as covered in the docs I linked) - simply loading them in the browser isn't sufficient.

            not sure why this has been bumped out of integration... perhaps someone could attempt this at the start of the week rather than leaving it untill the end of the integration cycle?

            Show
            Dan Marsden added a comment - testing SCORM with ADL tests is hard. - I'm not sure what I can do to improve this. you need to run the tests with the test harness(as covered in the docs I linked) - simply loading them in the browser isn't sufficient. not sure why this has been bumped out of integration... perhaps someone could attempt this at the start of the week rather than leaving it untill the end of the integration cycle?
            Hide
            Dan Marsden added a comment -
            • I do want to talk with HQ about adding the selenium tests to some form of regular cycle - but that is a separate issue - it shouldn't be a per-requisite to integrating this code.
            Show
            Dan Marsden added a comment - I do want to talk with HQ about adding the selenium tests to some form of regular cycle - but that is a separate issue - it shouldn't be a per-requisite to integrating this code.
            Hide
            Dan Marsden added a comment -

            the 1.2 tests are also included in the Moodle 1.9 backup linked here:
            http://docs.moodle.org/dev/SCORM_1.2_compliance_test_instructions#Moodle_install.2Fsetup

            that course should restore fine into a Moodle 2.3 site and contains the packages needed.

            Show
            Dan Marsden added a comment - the 1.2 tests are also included in the Moodle 1.9 backup linked here: http://docs.moodle.org/dev/SCORM_1.2_compliance_test_instructions#Moodle_install.2Fsetup that course should restore fine into a Moodle 2.3 site and contains the packages needed.
            Hide
            Dan Marsden added a comment -

            SCORM 1.2 test suite is available here:
            http://www.adlnet.gov/capabilities/scorm#tab-learn
            under the resources tab hit "ADL conformance test suite" - both 1.2 and 2004 test suites are available there.

            Show
            Dan Marsden added a comment - SCORM 1.2 test suite is available here: http://www.adlnet.gov/capabilities/scorm#tab-learn under the resources tab hit "ADL conformance test suite" - both 1.2 and 2004 test suites are available there.
            Hide
            Aparup Banerjee added a comment -

            Hi Dan,
            thanks for the extra notes for testing this , i'll see this goes into integration in time for integration testing.

            i had set this to reopened as there is still the one commit from MDL-7068 in this patch which i thought could be cleaned up.

            Show
            Aparup Banerjee added a comment - Hi Dan, thanks for the extra notes for testing this , i'll see this goes into integration in time for integration testing. i had set this to reopened as there is still the one commit from MDL-7068 in this patch which i thought could be cleaned up.
            Hide
            Dan Marsden added a comment -

            Hi Aparup - fair enough, have edited that commit message to use this bug instead of the global meta one (and rebased on this weeks master)

            I don't think I know anyone who has managed to get the ADL tests running on first go... I wish it was a lot easier! - hopefully we'll be able to set up the GSOC test harness stuff and run it on some weekly/nightly process against qa.moodle.net at some point soon.

            Show
            Dan Marsden added a comment - Hi Aparup - fair enough, have edited that commit message to use this bug instead of the global meta one (and rebased on this weeks master) I don't think I know anyone who has managed to get the ADL tests running on first go... I wish it was a lot easier! - hopefully we'll be able to set up the GSOC test harness stuff and run it on some weekly/nightly process against qa.moodle.net at some point soon.
            Hide
            Aparup Banerjee added a comment -

            Thanks this has been integrated into master.

            ps: i've cleaned up a whole bunch of whitespace and tabing (e4f7742) right before integrating.

            Show
            Aparup Banerjee added a comment - Thanks this has been integrated into master. ps: i've cleaned up a whole bunch of whitespace and tabing (e4f7742) right before integrating.
            Hide
            Dan Marsden added a comment -

            cool - thanks for the whitespace cleanup! - I'd avoided some of that as I thought it might make the git patch harder to review - am still planning to go through all SCORM code at some point and submit a whitespace/codechecker patch that doesn't make any code changes.

            Show
            Dan Marsden added a comment - cool - thanks for the whitespace cleanup! - I'd avoided some of that as I thought it might make the git patch harder to review - am still planning to go through all SCORM code at some point and submit a whitespace/codechecker patch that doesn't make any code changes.
            Hide
            Dan Marsden added a comment -

            David - this week my NZ hours are 9am->3pm (mon-Fri) and 7pm->late (wed,thurs) - so in the morning Perth time is probably best to get hold of me if you're having issues with the ADL tests (they are a pain to run)

            Show
            Dan Marsden added a comment - David - this week my NZ hours are 9am->3pm (mon-Fri) and 7pm->late (wed,thurs) - so in the morning Perth time is probably best to get hold of me if you're having issues with the ADL tests (they are a pain to run)
            Hide
            David Monllaó added a comment -

            Hi,

            I executed the 1.2 packages tests and I was able to finish both ADL SCORM Test Courses using IE 8 in a Win7, but I was experiencing a crash in SCO #6 of the first course so I completed from #6 to #9 without the ADL tool. In the ADL tool I've seen no error messages during the execution. I talked with Dan about it and he said that is OK to pass the 1.2 tests.

            As commented with Dan Marsden, this problems can be caused by the Java version used (1.7 and 1.6)

            About the CM-01 and CM-02a tests of the 2004 compliance, I got stuck in an eternal "Testing in progress..." but probably this can be tested with the whole 2004 suite when Moodle is ready

            Show
            David Monllaó added a comment - Hi, I executed the 1.2 packages tests and I was able to finish both ADL SCORM Test Courses using IE 8 in a Win7, but I was experiencing a crash in SCO #6 of the first course so I completed from #6 to #9 without the ADL tool. In the ADL tool I've seen no error messages during the execution. I talked with Dan about it and he said that is OK to pass the 1.2 tests. As commented with Dan Marsden, this problems can be caused by the Java version used (1.7 and 1.6) About the CM-01 and CM-02a tests of the 2004 compliance, I got stuck in an eternal "Testing in progress..." but probably this can be tested with the whole 2004 suite when Moodle is ready
            Hide
            Aparup Banerjee added a comment -

            Your issue has dug up some gold.
            It works great i've been told.
            Go forth, be brave, be bold.

            yay! "All your thoughts are belong to everyone."

            Thanks and ciao!

            Show
            Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!
            Hide
            Dan Marsden added a comment -

            awesome! - thanks to everyone who helped to test/review/integrate this patch and thanks to Mayank for his work on GSOC this year!

            Show
            Dan Marsden added a comment - awesome! - thanks to everyone who helped to test/review/integrate this patch and thanks to Mayank for his work on GSOC this year!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: