Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35870

improving caching of SCORM JS and improve general structure.

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.3, 2.7.1
    • Fix Version/s: 2.8
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      This is a significant change to a lot of SCORM files.
      Please re-run the QA tests:
      MDLQA-6752
      MDLQA-6753
      MDLQA-6755
      MDLQA-6756
      MDLQA-6757
      MDLQA-6758
      MDLQA-7090
      MDLQA-7091
      MDLQA-7092

      Also make the following test to ensure that the student preferences are being recalled correctly:
      Enter a SCORM package with the SCORM debugger turned on.
      Make sure normal debugger entries are added to the debugger - make sure the following entry shows in the debug log:
      LMSInitialize("", "") => 0

      in the scorm debugger window select "cmi.student_preference.language" and then enter a text string into the "value to set" field - then hit the LMSSetValue() button.

      Then with the same var selected ("cmi.student_preference.language") hit the LMSGetValue() button and make sure that value is returned in the debugger log below.

      Show
      This is a significant change to a lot of SCORM files. Please re-run the QA tests: MDLQA-6752 MDLQA-6753 MDLQA-6755 MDLQA-6756 MDLQA-6757 MDLQA-6758 MDLQA-7090 MDLQA-7091 MDLQA-7092 Also make the following test to ensure that the student preferences are being recalled correctly: Enter a SCORM package with the SCORM debugger turned on. Make sure normal debugger entries are added to the debugger - make sure the following entry shows in the debug log: LMSInitialize("", "") => 0 in the scorm debugger window select "cmi.student_preference.language" and then enter a text string into the "value to set" field - then hit the LMSSetValue() button. Then with the same var selected ("cmi.student_preference.language") hit the LMSGetValue() button and make sure that value is returned in the debugger log below.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      This bug has evolved a bit - our main aim is to give proper separation of PHP and JS in SCORM and at the same time improve the way that values are retrieved and make it more consistent across all parameters that should be available to the SCORM package. such as the student_preference vars.

      The long term plan is to completely remove the .js.php files but we may still need to keep part of them in the first version of this change.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              danmarsden Dan Marsden added a comment -

              Hi Luc,

              I haven't been able to reproduce this as an error - here's the output from my scorm debugger in Moodle:

              Sun, 11 Nov 2012 23:24:48 GMT: LMSGetValue("cmi.student_preference.language") - => 0
              Sun, 11 Nov 2012 23:24:48 GMT: LMSGetErrorString("0", "No error") => 0
              Sun, 11 Nov 2012 23:24:54 GMT: LMSSetValue("cmi.student_preference.language", "US/English") => 0
              Sun, 11 Nov 2012 23:24:54 GMT: LMSGetErrorString("0", "No error") => 0
              Sun, 11 Nov 2012 23:24:57 GMT: LMSGetValue("cmi.student_preference.language") - US/English => 0
              Sun, 11 Nov 2012 23:24:57 GMT: LMSGetErrorString("0", "No error") => 0

              can you please provide more information on how you managed to reproduce this?

              Show
              danmarsden Dan Marsden added a comment - Hi Luc, I haven't been able to reproduce this as an error - here's the output from my scorm debugger in Moodle: Sun, 11 Nov 2012 23:24:48 GMT: LMSGetValue("cmi.student_preference.language") - => 0 Sun, 11 Nov 2012 23:24:48 GMT: LMSGetErrorString("0", "No error") => 0 Sun, 11 Nov 2012 23:24:54 GMT: LMSSetValue("cmi.student_preference.language", "US/English") => 0 Sun, 11 Nov 2012 23:24:54 GMT: LMSGetErrorString("0", "No error") => 0 Sun, 11 Nov 2012 23:24:57 GMT: LMSGetValue("cmi.student_preference.language") - US/English => 0 Sun, 11 Nov 2012 23:24:57 GMT: LMSGetErrorString("0", "No error") => 0 can you please provide more information on how you managed to reproduce this?
              Hide
              luxyluc luc santin added a comment -

              Hi Dan,
              Here is the process to see the bug.

              • Launch the SCO : LMSInitialize("")
              • bSet the value : LMSSetValue("cmi.student_preference.language", "US/English") =>0
              • Exit the SCO : LMSFinish("")
              • Take a look at the reporting in moodle -> the value is correct !
              • Launch the SCO again : LMSInitialize("")
              • Get the value : LMSGetValue("cmi.student_preference.language") - =>0 ->(the value is not set anymore ! ).
              • Exit the SCO : LMSFinish("")
              • Take a look at the reporting (the value is not set anymore).
              Show
              luxyluc luc santin added a comment - Hi Dan, Here is the process to see the bug. Launch the SCO : LMSInitialize("") bSet the value : LMSSetValue("cmi.student_preference.language", "US/English") =>0 Exit the SCO : LMSFinish("") Take a look at the reporting in moodle -> the value is correct ! Launch the SCO again : LMSInitialize("") Get the value : LMSGetValue("cmi.student_preference.language") - =>0 ->(the value is not set anymore ! ). Exit the SCO : LMSFinish("") Take a look at the reporting (the value is not set anymore).
              Hide
              danmarsden Dan Marsden added a comment -

              ah - of course.. Adding Matteo here as well - there are a bunch of student_preferences that we should probably take a look at here - I can't see any reason in the spec why we don't do this for all rw student_preferences - Matteo do you have any thoughts on this?

              Show
              danmarsden Dan Marsden added a comment - ah - of course.. Adding Matteo here as well - there are a bunch of student_preferences that we should probably take a look at here - I can't see any reason in the spec why we don't do this for all rw student_preferences - Matteo do you have any thoughts on this?
              Hide
              matteo Matteo Scaramuccia added a comment -

              Hi Dan,
              you're right: no reason for not applying the DB value, when available, to cmi.student_preference.*; these are simple fields - key => value, no special pattern - . This is a required improvement even if ADL Test Suite doesn't test them, definitely a bug.
              BTW, among those with no pattern and mod=rw these are the only fields missing the correct initialisation while in SCORM 1.3 implementation cmi.learner_preference is correctly initialised with the DB values.

              Show
              matteo Matteo Scaramuccia added a comment - Hi Dan, you're right: no reason for not applying the DB value, when available, to cmi.student_preference.* ; these are simple fields - key => value , no special pattern - . This is a required improvement even if ADL Test Suite doesn't test them, definitely a bug. BTW, among those with no pattern and mod=rw these are the only fields missing the correct initialisation while in SCORM 1.3 implementation cmi.learner_preference is correctly initialised with the DB values.
              Hide
              danmarsden Dan Marsden added a comment -

              yeah I might use this as an opportunity to tidy up how the vars are passed to the js - eventually I want to strip out all the php code and turn that into a js file that can be cached and pass the vars as js params using $PAGE->requires->data_for_js()

              Show
              danmarsden Dan Marsden added a comment - yeah I might use this as an opportunity to tidy up how the vars are passed to the js - eventually I want to strip out all the php code and turn that into a js file that can be cached and pass the vars as js params using $PAGE->requires->data_for_js()
              Hide
              danmarsden Dan Marsden added a comment -

              doh.... I mean js_init_call - not data_for_js....

              Show
              danmarsden Dan Marsden added a comment - doh.... I mean js_init_call - not data_for_js....
              Hide
              danmarsden Dan Marsden added a comment -

              I've started on a re-organise of the php code in scorm_12.js.php - this is an untested patch that starts to move a lot of the php code to the top of the page in preparation for converting this file into pure js.

              Show
              danmarsden Dan Marsden added a comment - I've started on a re-organise of the php code in scorm_12.js.php - this is an untested patch that starts to move a lot of the php code to the top of the page in preparation for converting this file into pure js.
              Hide
              danmarsden Dan Marsden added a comment -

              bouncing up for peer review - note to reviewer:
              The main reason for this patch is to add the extra information from the student_preferences vars that weren't previously being returned - I've taken this opportunity to shift a lot of the php code to the top of the page in preparation for converting the file into proper js at some point.

              Line lengths do not yet meet Moodle guidelines - especially on lines using ternary operators this is intentional atm - splitting these out will make it harder to read and the plan is to move them out of the js file completely anyway. These are really just moved from other areas of the code too.

              Show
              danmarsden Dan Marsden added a comment - bouncing up for peer review - note to reviewer: The main reason for this patch is to add the extra information from the student_preferences vars that weren't previously being returned - I've taken this opportunity to shift a lot of the php code to the top of the page in preparation for converting the file into proper js at some point. Line lengths do not yet meet Moodle guidelines - especially on lines using ternary operators this is intentional atm - splitting these out will make it harder to read and the plan is to move them out of the js file completely anyway. These are really just moved from other areas of the code too.
              Hide
              luxyluc luc santin added a comment - - edited

              Hi Dan
              I'm not savvy enough as a developer neither in English. So I'm not sure to understand what you can expect from people like me.
              Anyway, I tested your file in my moodle implementation.
              It seems to work fine regarding the cmi.student_preference object.
              But I think i've found another bug now.
              It's about CMIdecimal format required for fields like cmi.core.score (min, max, raw), cmi.objectives.n.score (raw, min, max) , cmi.interactions.n.weighting and cmi.interactions.n.result.
              For those data the CMIdecimal format is required. But if the SCO tries to send a more-than-2-digits-after-the-point number it fails.
              For example:
              OK : LMSSetValue("cmi.core.score.raw", "12.34") => 0
              KO : LMSSetValue("cmi.core.score.raw", "12.345") => 405
              I think the regular expression at line 101 is not conformant to the SCORM specification based on AICC specification (255 characters, positive or negative decimal ).
              I tested this regular expression. It seems to work fine.

              I hope this can help...

              Show
              luxyluc luc santin added a comment - - edited Hi Dan I'm not savvy enough as a developer neither in English. So I'm not sure to understand what you can expect from people like me. Anyway, I tested your file in my moodle implementation. It seems to work fine regarding the cmi.student_preference object. But I think i've found another bug now. It's about CMIdecimal format required for fields like cmi.core.score (min, max, raw), cmi.objectives.n.score (raw, min, max) , cmi.interactions.n.weighting and cmi.interactions.n.result. For those data the CMIdecimal format is required. But if the SCO tries to send a more-than-2-digits-after-the-point number it fails. For example: OK : LMSSetValue("cmi.core.score.raw", "12.34") => 0 KO : LMSSetValue("cmi.core.score.raw", "12.345") => 405 I think the regular expression at line 101 is not conformant to the SCORM specification based on AICC specification (255 characters, positive or negative decimal ). I tested this regular expression. It seems to work fine. I hope this can help...
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Luc - thanks for testing that - feel free to report a new issue related to CMIDecimal if you think we need to change it.

              FYI - Peer review is a formal process that is required before I can push this patch into the integration process
              here's more info on Peer Review's:
              http://docs.moodle.org/dev/Peer_reviewing_checklist

              commenting that you have tested the fix is really helpful! - obviously the more people that test something the less likely it is that we introduce any regressions.

              Show
              danmarsden Dan Marsden added a comment - Hi Luc - thanks for testing that - feel free to report a new issue related to CMIDecimal if you think we need to change it. FYI - Peer review is a formal process that is required before I can push this patch into the integration process here's more info on Peer Review's: http://docs.moodle.org/dev/Peer_reviewing_checklist commenting that you have tested the fix is really helpful! - obviously the more people that test something the less likely it is that we introduce any regressions.
              Hide
              luxyluc luc santin added a comment -

              Hi Dan

              Thank you for this comment and the link (It's clear now, that I can't perform a "peer review").

              I reported a new issue for the Regular Expression defining the CMIDecimal format (MDL_41695).

              Show
              luxyluc luc santin added a comment - Hi Dan Thank you for this comment and the link (It's clear now, that I can't perform a "peer review"). I reported a new issue for the Regular Expression defining the CMIDecimal format (MDL_41695).
              Hide
              danmarsden Dan Marsden added a comment -

              I've done some more work on this today - starting to look a lot better but I'd still like to abstract a bit more before I push this for integration.

              Show
              danmarsden Dan Marsden added a comment - I've done some more work on this today - starting to look a lot better but I'd still like to abstract a bit more before I push this for integration.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Taking this out of peer review as your last comment suggests that you wish to do some more work on this first.

              Show
              dobedobedoh Andrew Nicols added a comment - Taking this out of peer review as your last comment suggests that you wish to do some more work on this first.
              Hide
              danmarsden Dan Marsden added a comment -

              Thanks Vignesh - I've made a bunch of comments on that patch - hopefully they all make sense! - great progress so far, thanks!

              Show
              danmarsden Dan Marsden added a comment - Thanks Vignesh - I've made a bunch of comments on that patch - hopefully they all make sense! - great progress so far, thanks!
              Hide
              nobelium vignesh p added a comment -

              Hi Dan, Thanks for your time. Can you please update the tracker with the branch details.

              Show
              nobelium vignesh p added a comment - Hi Dan, Thanks for your time. Can you please update the tracker with the branch details.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Vignesh,

              I'm not sure what you mean? - I've just made comments on your existing patch via github, I haven't made any code changes - you can see the comments if you scroll to the bottom of: https://github.com/nobelium/moodle/compare/moodle:master...MDL-35870

              to see the comments in-line you will need to view the commits separately on github

              let me know if I've missed something!

              Show
              danmarsden Dan Marsden added a comment - Hi Vignesh, I'm not sure what you mean? - I've just made comments on your existing patch via github, I haven't made any code changes - you can see the comments if you scroll to the bottom of: https://github.com/nobelium/moodle/compare/moodle:master...MDL-35870 to see the comments in-line you will need to view the commits separately on github let me know if I've missed something!
              Hide
              nobelium vignesh p added a comment -

              Oops, sorry. I thought you made commits(not comments). I have seen them. I'm working on it, I'll change them and let you know.

              Show
              nobelium vignesh p added a comment - Oops, sorry. I thought you made commits(not comments). I have seen them. I'm working on it, I'll change them and let you know.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Vignesh,

              apologies - my responses/reviews have been quite slow over the past couple of weeks - I'm back to normal now so should be a lot more responsive.

              This is getting very close but still a few issues - mainly with loaddatamodel.php

              scorm_insert_track is just a php call and could be moved into the bottom of player.php'

              loaddatamodel did some handling of $userdata that is no longer being applied - we can safely ignore the addslashes_js calls as they are no longer needed due as we are now passing params via Moodle javascript functions but these 2 blocks of code need to be moved so they are still working:

              First the check for SCORM1.3 that will only pass previous datamodels on re-launch if cmi.exit is set to suspend (line 54 of loaddatamodel.php)

              and then this block of code that checks mode.

              $userdata->mode = 'normal';
              if (!empty($mode)) {
                  $userdata->mode = $mode;
              }
              if ($userdata->mode == 'normal') {
                  $userdata->credit = 'credit';
              } else {
                  $userdata->credit = 'no-credit';
              }
              

              And this block which could probably be shifted into scorm_13lib.php get_scorm_default()

              if (scorm_version_check($scorm->version, SCORM_13)) {
                  $objectives = $DB->get_records('scorm_seq_objective', array('scoid' => $scoid));
                  $index = 0;
                  foreach ($objectives as $objective) {
                      if (!empty($objective->minnormalizedmeasure)) {
                          $userdata->{'cmi.scaled_passing_score'} = $objective->minnormalizedmeasure;
                      }
                      if (!empty($objective->objectiveid)) {
                          $userdata->{'cmi.objectives.N'.$index.'.id'} = $objective->objectiveid;
                          $index++;
                      }
                  }
              }
              

              after those changes are made we should be able to remove loaddatamodel.php

              thanks!

              Show
              danmarsden Dan Marsden added a comment - Hi Vignesh, apologies - my responses/reviews have been quite slow over the past couple of weeks - I'm back to normal now so should be a lot more responsive. This is getting very close but still a few issues - mainly with loaddatamodel.php scorm_insert_track is just a php call and could be moved into the bottom of player.php' loaddatamodel did some handling of $userdata that is no longer being applied - we can safely ignore the addslashes_js calls as they are no longer needed due as we are now passing params via Moodle javascript functions but these 2 blocks of code need to be moved so they are still working: First the check for SCORM1.3 that will only pass previous datamodels on re-launch if cmi.exit is set to suspend (line 54 of loaddatamodel.php) and then this block of code that checks mode. $userdata->mode = 'normal'; if (!empty($mode)) { $userdata->mode = $mode; } if ($userdata->mode == 'normal') { $userdata->credit = 'credit'; } else { $userdata->credit = 'no-credit'; } And this block which could probably be shifted into scorm_13lib.php get_scorm_default() if (scorm_version_check($scorm->version, SCORM_13)) { $objectives = $DB->get_records('scorm_seq_objective', array('scoid' => $scoid)); $index = 0; foreach ($objectives as $objective) { if (!empty($objective->minnormalizedmeasure)) { $userdata->{'cmi.scaled_passing_score'} = $objective->minnormalizedmeasure; } if (!empty($objective->objectiveid)) { $userdata->{'cmi.objectives.N'.$index.'.id'} = $objective->objectiveid; $index++; } } } after those changes are made we should be able to remove loaddatamodel.php thanks!
              Hide
              poltawski Dan Poltawski added a comment - - edited

              [Edited away noise from me]

              Show
              poltawski Dan Poltawski added a comment - - edited [Edited away noise from me]
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Dan we know it will fail generally as there are many existing issues with scorm - does cibot not work at the moment? I was hoping to see a report.

              Show
              danmarsden Dan Marsden added a comment - Hi Dan we know it will fail generally as there are many existing issues with scorm - does cibot not work at the moment? I was hoping to see a report.
              Hide
              cibot CiBoT added a comment -

              Results for MDL-35870

              • Remote repository: git://github.com/nobelium/moodle.git
              Show
              cibot CiBoT added a comment - Results for MDL-35870 Remote repository: git://github.com/nobelium/moodle.git Remote branch MDL-35870 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4638 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4638/artifact/work/smurf.html
              Hide
              danmarsden Dan Marsden added a comment -

              NOTE TO INTEGRATOR:
              The coding guideline issues reported by CiBot will be addressed in the linked bug and are related to pre-existing code. We have not addressed them all as part of this patch as it makes it a lot harder to review the patch and this patch is hard enough to review as it stands!

              There is also further work that could be done to further abstract many areas but the main aim of this bug (getting rid of the nasty js files built using php) has been achieved by this patch. Further restructuring and abstraction should be managed on new tracker issues (feel free to create any new issues to cover work you think should still be done!)

              Thanks to Vignesh for working on this (and to the QA team/integrators who will review/test this work!)

              Show
              danmarsden Dan Marsden added a comment - NOTE TO INTEGRATOR: The coding guideline issues reported by CiBot will be addressed in the linked bug and are related to pre-existing code. We have not addressed them all as part of this patch as it makes it a lot harder to review the patch and this patch is hard enough to review as it stands! There is also further work that could be done to further abstract many areas but the main aim of this bug (getting rid of the nasty js files built using php) has been achieved by this patch. Further restructuring and abstraction should be managed on new tracker issues (feel free to create any new issues to cover work you think should still be done!) Thanks to Vignesh for working on this (and to the QA team/integrators who will review/test this work!)
              Hide
              poltawski Dan Poltawski added a comment -

              Apologies for the previous comment from me! It was actually me testing improvements to the cibot using my own tracker account.

              Show
              poltawski Dan Poltawski added a comment - Apologies for the previous comment from me! It was actually me testing improvements to the cibot using my own tracker account.
              Hide
              danmarsden Dan Marsden added a comment -

              Hehe - unleash the DanBot...... next he'll be integrating for you while you 'work remotely'

              Show
              danmarsden Dan Marsden added a comment - Hehe - unleash the DanBot...... next he'll be integrating for you while you 'work remotely'
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Oki, ignoring code-style issues and accepting octopus as a pet animal... :-P

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Oki, ignoring code-style issues and accepting octopus as a pet animal... :-P
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              For testing this.... I'd recommend (adding Rajesh Taneja coz we have been discussing this some hours ago):

              Create a testing session involving:

              1) Running all the SCORM QA Tests.
              2) Running all the @mod_scorm acceptance tests against multiple DBs and with browsers.
              3) Any other test specified in the testing instructions.

              All this about testing, looking to the patch now...

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - For testing this.... I'd recommend (adding Rajesh Taneja coz we have been discussing this some hours ago): Create a testing session involving: 1) Running all the SCORM QA Tests . 2) Running all the @mod_scorm acceptance tests against multiple DBs and with browsers. 3) Any other test specified in the testing instructions. All this about testing, looking to the patch now...
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (master only).

              I've added a couple of commits.

              1) Fixing wrong whitespace ocurrences that would be leading to failed CI tests.
              2) Bump mod_scorm version just to ensure any js caching is prevented.

              Testing will direct the designs of this issue, let's cross the toes!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only). I've added a couple of commits. 1) Fixing wrong whitespace ocurrences that would be leading to failed CI tests. 2) Bump mod_scorm version just to ensure any js caching is prevented. Testing will direct the designs of this issue, let's cross the toes!
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Vignesh for fixing this issue and Eloy for the head-up on testing.

              1. Test session has been created to test this thoroughly tomorrow.
              2. Acceptance test passed on all 4 db's, Running them on all browsers.
              3. Manual test resulted in following error and no debug window popped-up

                Coding error detected, it must be fixed by a programmer: Attempt to require a JavaScript file that does not exist
                 
                Debug info: [dirroot]/mod/scorm/datamodels/debug.js.php
                Error code: codingerror
                Stack trace:
                line 665 of /lib/outputrequirementslib.php: coding_exception thrown
                line 390 of /lib/outputrequirementslib.php: call to page_requirements_manager->js_fix_url()
                line 46 of /mod/scorm/datamodels/scorm_12.php: call to page_requirements_manager->js()
                line 258 of /mod/scorm/player.php: call to include_once()
                

              Steps to reproduce:

              1. Enable allowapidebug
              2. Create SCORM activity with attached package
              3. Enter in normal or preview mode and you will see above error

              FYI: No error without allowapidebug

              More results on it's way ....

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Vignesh for fixing this issue and Eloy for the head-up on testing. Test session has been created to test this thoroughly tomorrow. Acceptance test passed on all 4 db's, Running them on all browsers. Manual test resulted in following error and no debug window popped-up Coding error detected, it must be fixed by a programmer: Attempt to require a JavaScript file that does not exist   Debug info: [dirroot]/mod/scorm/datamodels/debug.js.php Error code: codingerror Stack trace: line 665 of /lib/outputrequirementslib.php: coding_exception thrown line 390 of /lib/outputrequirementslib.php: call to page_requirements_manager->js_fix_url() line 46 of /mod/scorm/datamodels/scorm_12.php: call to page_requirements_manager->js() line 258 of /mod/scorm/player.php: call to include_once() Steps to reproduce: Enable allowapidebug Create SCORM activity with attached package Enter in normal or preview mode and you will see above error FYI: No error without allowapidebug More results on it's way ....
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Failing this for now, as it failed manual test.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Failing this for now, as it failed manual test.
              Hide
              nobelium vignesh p added a comment -

              Hi Rajesh, I have pushed a commit to my repo on branch MDL-35870 to fix the debug.js.php not found problem, but debug.js.php it self has a lot of problems it seems. It doesn't load the scorm if apidebugging is enabled. I'm working on it.

              Show
              nobelium vignesh p added a comment - Hi Rajesh, I have pushed a commit to my repo on branch MDL-35870 to fix the debug.js.php not found problem, but debug.js.php it self has a lot of problems it seems. It doesn't load the scorm if apidebugging is enabled. I'm working on it.
              Hide
              nobelium vignesh p added a comment -

              Hi Rajesh, Fixed the bug. Some one had forgot to close the html comment in debug.js.php and it was causing all the issue. Every thing works now. Did anything else fail in manual testing?

              Show
              nobelium vignesh p added a comment - Hi Rajesh, Fixed the bug. Some one had forgot to close the html comment in debug.js.php and it was causing all the issue. Every thing works now. Did anything else fail in manual testing?
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              I've applied last vignesh commit:

              http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=76c8fa4190594b362d467fc138f7741be72bfb1d

              So I'm sending this back to testing.

              The Test session should reveal any other problem, let's see.

              Thanks for the promptly reply, V!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - I've applied last vignesh commit: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=76c8fa4190594b362d467fc138f7741be72bfb1d So I'm sending this back to testing. The Test session should reveal any other problem, let's see. Thanks for the promptly reply, V!
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for the quick fix Vignesh,

              Testing this again...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for the quick fix Vignesh, Testing this again...
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Sorry Vignesh,

              There are few fails from this. Test is still going on, but I am failing this to reflect the status.

              More information about failures are available on https://tracker.moodle.org/secure/ViewSession.jspa?testSessionId=10747

              FYI:

              1. Manual test is now passing.
              2. Behat passing
              Show
              rajeshtaneja Rajesh Taneja added a comment - Sorry Vignesh, There are few fails from this. Test is still going on, but I am failing this to reflect the status. More information about failures are available on https://tracker.moodle.org/secure/ViewSession.jspa?testSessionId=10747 FYI: Manual test is now passing. Behat passing
              Hide
              nobelium vignesh p added a comment -

              Hi Rajesh, I have fixed the scoid missing in popup mode issue. This solves MDL-46394 and MDL-46395. Please cherry pick the commit from my branch.

              I'm yet to look at MDL-46391 and MDL-46390. Will be looking at them in some time.

              Show
              nobelium vignesh p added a comment - Hi Rajesh, I have fixed the scoid missing in popup mode issue. This solves MDL-46394 and MDL-46395 . Please cherry pick the commit from my branch. I'm yet to look at MDL-46391 and MDL-46390 . Will be looking at them in some time.
              Hide
              nobelium vignesh p added a comment -

              Oops, MDL-46391 is not related to scorm and MDL-46390 got fixed by my last commit.

              Show
              nobelium vignesh p added a comment - Oops, MDL-46391 is not related to scorm and MDL-46390 got fixed by my last commit.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for the quick fix Vignesh,

              Requesting Eloy to look at it now

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for the quick fix Vignesh, Requesting Eloy to look at it now
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              Patch fixing scoid applied. Sending back to testing, thanks guys!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited Patch fixing scoid applied. Sending back to testing, thanks guys!
              Hide
              dmonllao David Monllaó added a comment -

              Hi,

              I'm still experiencing MDL-46390, even after pulling latest integration (9a363a1cd276ac66df5cad49c9469802ccb2bff1)

              This one https://tracker.moodle.org/secure/attachment/39788/PuzzleQuizSCORMExport%20%281%29.zip is the package I've used.

              Show
              dmonllao David Monllaó added a comment - Hi, I'm still experiencing MDL-46390 , even after pulling latest integration (9a363a1cd276ac66df5cad49c9469802ccb2bff1) This one https://tracker.moodle.org/secure/attachment/39788/PuzzleQuizSCORMExport%20%281%29.zip is the package I've used.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Vignesh,

              1. You are right, MDL-46391 is not related. Moved it out of test session.
              2. Requested David to re-test MDL-46390
              3. Testing MDL-46395 and MDL-46394 now.
              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Vignesh, You are right, MDL-46391 is not related. Moved it out of test session. Requested David to re-test MDL-46390 Testing MDL-46395 and MDL-46394 now.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              MDL-46395 and MDL-46394 passing now...
              Thanks Vignesh.

              Show
              rajeshtaneja Rajesh Taneja added a comment - MDL-46395 and MDL-46394 passing now... Thanks Vignesh.
              Hide
              rajeshtaneja Rajesh Taneja added a comment - - edited

              MDLQA-87 is failing, please let me know if you want me to create an issue for same.

              I am using 211 facts package from MDLQA-4687

              Show
              rajeshtaneja Rajesh Taneja added a comment - - edited MDLQA-87 is failing, please let me know if you want me to create an issue for same. I am using 211 facts package from MDLQA-4687
              Hide
              danmarsden Dan Marsden added a comment -

              I probably don't need to mention here as well... but MDL-46390 is a bug with existing code so not related to this patch. Thanks everyone for the great testing!

              Show
              danmarsden Dan Marsden added a comment - I probably don't need to mention here as well... but MDL-46390 is a bug with existing code so not related to this patch. Thanks everyone for the great testing!
              Hide
              danmarsden Dan Marsden added a comment -

              hmmm - MDLQA-87 probably needs a specific SCORM package to test with - not all SCORM packages will have been authored to support those instructions - I'll take a look at it here now. Great to have HQ team doing this and picking up issues that were missed during normal QA cycle!

              Show
              danmarsden Dan Marsden added a comment - hmmm - MDLQA-87 probably needs a specific SCORM package to test with - not all SCORM packages will have been authored to support those instructions - I'll take a look at it here now. Great to have HQ team doing this and picking up issues that were missed during normal QA cycle!
              Hide
              nobelium vignesh p added a comment -

              Hi David, Yeah, I'm able to reproduce it now. I'll look into it.
              Rajesh - can you create a new issue for MDLQA-87. Its taking longer than expected.

              Show
              nobelium vignesh p added a comment - Hi David, Yeah, I'm able to reproduce it now. I'll look into it. Rajesh - can you create a new issue for MDLQA-87 . Its taking longer than expected.
              Hide
              danmarsden Dan Marsden added a comment -

              MDLQA-87 is a regression with this patch (really good catch with an edge-case related to the SCORM package, looks like an issue related to the assets in the SCORM package still trying to initiate the SCORM API)
              usually the integration team would reject based on a regression - if we can't fix it this week we can try again during next weeks integration review. (not a big issue if we delay this until next week)

              Show
              danmarsden Dan Marsden added a comment - MDLQA-87 is a regression with this patch (really good catch with an edge-case related to the SCORM package, looks like an issue related to the assets in the SCORM package still trying to initiate the SCORM API) usually the integration team would reject based on a regression - if we can't fix it this week we can try again during next weeks integration review. (not a big issue if we delay this until next week)
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Vignesh,

              Created MDL-46397 and failing this issue for now.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Vignesh, Created MDL-46397 and failing this issue for now.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              So... is there anything pending here:

              So it seems that the later is the only one really "pending", do you think we should:

              A) revert everything and try again next week the whole thing.
              B) keep current code upstream and wait for the fix to land next/week(s).

              I'd say that being master only, well isolated, and after the intensive testing and given it does not break unit/acceptance test... we could keep it upstream till the pending fix of MDL-46397 lands. And forget about to revert and reopen everything.

              Thoughts?

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - So... is there anything pending here: MDL-46390 : Existing bug. Unrelated. MDL-46391 : Unrelated. Others have been fixed/deferred ( MDL-46394 and MDL-46395 ). MDL-46397 : True regression caused by this, will fix MDLQA-87 So it seems that the later is the only one really "pending", do you think we should: A) revert everything and try again next week the whole thing. B) keep current code upstream and wait for the fix to land next/week(s). I'd say that being master only, well isolated, and after the intensive testing and given it does not break unit/acceptance test... we could keep it upstream till the pending fix of MDL-46397 lands. And forget about to revert and reopen everything. Thoughts?
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              I just tested the last MDLQA-83 and it's failing as well. Seems to be related to MDLQA-87 (MDL-46397).
              Have used same package as in MDLQA-87 and it works fine in master stable.

              Seems to be a big regression for being on master till MDL-46397 get fixed.

              FYI: I am not creating another issue, assuming it should be fixed by MDL-46397. Updating it...

              Show
              rajeshtaneja Rajesh Taneja added a comment - I just tested the last MDLQA-83 and it's failing as well. Seems to be related to MDLQA-87 ( MDL-46397 ). Have used same package as in MDLQA-87 and it works fine in master stable. Seems to be a big regression for being on master till MDL-46397 get fixed. FYI: I am not creating another issue, assuming it should be fixed by MDL-46397 . Updating it...
              Hide
              danmarsden Dan Marsden added a comment -

              allowing this code to land with a regression (MDL-46397) would go against normal policies IMO but I suppose it will make it easier to review the fix when it lands next week. It's up to you guys whether you reject/accept. I'm going to take a look at the regression tonight and will have a bit more time tomorrow so it's possible we might still come up with a fix before the end of the week but if this is the only thing holding up integration you should probably make the call on it today.

              thanks everyone for the help here - it's thrown up some great edge case bugs that we missed and some new existing bugs have been reported.

              Show
              danmarsden Dan Marsden added a comment - allowing this code to land with a regression ( MDL-46397 ) would go against normal policies IMO but I suppose it will make it easier to review the fix when it lands next week. It's up to you guys whether you reject/accept. I'm going to take a look at the regression tonight and will have a bit more time tomorrow so it's possible we might still come up with a fix before the end of the week but if this is the only thing holding up integration you should probably make the call on it today. thanks everyone for the help here - it's thrown up some great edge case bugs that we missed and some new existing bugs have been reported.
              Hide
              nobelium vignesh p added a comment -

              Hi Rajesh, Yeah you are right, MDLQA-83 failure is related to scorm not tracking data. Anyway, I have a fix for MDL-46397. I have pushed the fix to my branch. The fix only solves the issue for scorm v1.2.

              "211 facts" is not a valid package and contains error in imsmanifest.xml file. Use the package attached to MDL-42812 for testing.

              Having a small problem for now(the first sco is repeated twice - some DOM issue). I'll fix the DOM issue first and replicate the fix for scorm v1.3 and aicc packages and let you know. I'll try to complete the patch by tonight.

              Dan - This is the preloading solution that we discussed about in the mail. Take a look when ever you find time https://github.com/nobelium/moodle/commit/338aef4c75d7ea9aa73ce8708ce1007dffddbfc4.

              Show
              nobelium vignesh p added a comment - Hi Rajesh, Yeah you are right, MDLQA-83 failure is related to scorm not tracking data. Anyway, I have a fix for MDL-46397 . I have pushed the fix to my branch. The fix only solves the issue for scorm v1.2. "211 facts" is not a valid package and contains error in imsmanifest.xml file. Use the package attached to MDL-42812 for testing. Having a small problem for now(the first sco is repeated twice - some DOM issue). I'll fix the DOM issue first and replicate the fix for scorm v1.3 and aicc packages and let you know. I'll try to complete the patch by tonight. Dan - This is the preloading solution that we discussed about in the mail. Take a look when ever you find time https://github.com/nobelium/moodle/commit/338aef4c75d7ea9aa73ce8708ce1007dffddbfc4 .
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for the Quick fix again

              I don't know much about 211 facts pacakge, but it has been used earlier and seems to be working fine in stable master.
              I am sure you have seen deeper in this, and if Dan and integrators are happy for not supporting that then it's fine.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for the Quick fix again I don't know much about 211 facts pacakge, but it has been used earlier and seems to be working fine in stable master. I am sure you have seen deeper in this, and if Dan and integrators are happy for not supporting that then it's fine.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for all the awesome work Vignesh,

              As this is master only issue, integrators gave +1 for passing this.

              I have raised priority of MDL-46397 to blocker, so it can get fixed/reviewed later.

              Passing ...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for all the awesome work Vignesh, As this is master only issue, integrators gave +1 for passing this. I have raised priority of MDL-46397 to blocker, so it can get fixed/reviewed later. Passing ...
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Thanks guys, let's continue @ the follow-up issue, yay!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Thanks guys, let's continue @ the follow-up issue, yay!
              Hide
              nobelium vignesh p added a comment - - edited

              @rajesh, have you integrated my previous commit to master? I have the fix for the DOM issue. I'll be replicating the fix for scorm v1.3 and aicc in some time. Should I create a new commit for those?

              Show
              nobelium vignesh p added a comment - - edited @rajesh, have you integrated my previous commit to master? I have the fix for the DOM issue. I'll be replicating the fix for scorm v1.3 and aicc in some time. Should I create a new commit for those?
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Hello Vignesh,

              I am not integrator.
              As mentioned by Eloy, any further patch (Including your last patch), should now go in MDL-46397 and looked at as a separate issue.

              Cheers
              Rajesh

              Show
              rajeshtaneja Rajesh Taneja added a comment - Hello Vignesh, I am not integrator. As mentioned by Eloy, any further patch (Including your last patch), should now go in MDL-46397 and looked at as a separate issue. Cheers Rajesh
              Hide
              nobelium vignesh p added a comment -

              Thanks Rajesh and Eloy, Let's continue @ the new issue.

              Show
              nobelium vignesh p added a comment - Thanks Rajesh and Eloy, Let's continue @ the new issue.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for your hard work - this issue is now part of Moodle!

              A good decision is based on knowledge and not on numbers.

              – Plato

              Show
              poltawski Dan Poltawski added a comment - Thanks for your hard work - this issue is now part of Moodle! A good decision is based on knowledge and not on numbers. – Plato

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Nov/14