Details

    • Rank:
      44097

      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

        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: