Moodle
  1. Moodle
  2. MDL-35227

error enrolling users if scorm modules present

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.4
    • Fix Version/s: 2.3.3
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      I couldn't reproduce this problem as it arises in some courses but not in other courses. It seems that the "recover grades" option has something to do with it.

      Show
      I couldn't reproduce this problem as it arises in some courses but not in other courses. It seems that the "recover grades" option has something to do with it.
    • Workaround:
      Hide

      The AJAX form calls enrol/manual/ajax.php, which (line 126) calls lib/gradelib/grade_recover_history_grades() -> lib/gradelib/grade_grab_course_grades() -> lib/gradelib/grade_update_mod_grades(). There, at last, if a SCORM module is present, the mod/scorm/lib/scorm_grade_item_update() is called and tries to create a New completion_info() without including completionlib.php.
      Workaround: use patch displayed in the Description of this issue.

      Show
      The AJAX form calls enrol/manual/ajax.php, which (line 126) calls lib/gradelib/grade_recover_history_grades() -> lib/gradelib/grade_grab_course_grades() -> lib/gradelib/grade_update_mod_grades(). There, at last, if a SCORM module is present, the mod/scorm/lib/scorm_grade_item_update() is called and tries to create a New completion_info() without including completionlib.php. Workaround: use patch displayed in the Description of this issue.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      master_MDL-35227
    • Rank:
      43875

      Description

      In a course containing one or several SCORM module instances, if one tries to enrol users manually (from the Settings Block -> Course administration -> Users -> Enrolled users), we get a "Syntax error" Javascript modal. The error logged in the Apache error log is: PHP Fatal error: Class 'completion_info' not found in /***/mod/scorm/lib.php on line 691, referer: http://***/enrol/users.php?id=514
      The following hack fixes the issue :

      --- a/mod/scorm/lib.php
      +++ b/mod/scorm/lib.php
      @@ -688,6 +688,9 @@ function scorm_grade_item_update($scorm, $grades=null, $updatecompletion=true) {
       
               $cm = get_coursemodule_from_instance('scorm', $scorm->id, $course->id);
               if (!empty($cm)) {
      +            if (!class_exists('completion_info')) { //workaround for buggy PHP versions
      +                require_once($CFG->libdir.'/completionlib.php');
      +            }
                   $completion = new completion_info($course);
                   $completion->update_state($cm, COMPLETION_COMPLETE);
               }
      
      

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          I think this makes sense to add in course/ajax.php - bouncing up for peer review. We could add it in SCORM if the integrator/peer reviewer can think of a reason why it should go there instead?

          Show
          Dan Marsden added a comment - I think this makes sense to add in course/ajax.php - bouncing up for peer review. We could add it in SCORM if the integrator/peer reviewer can think of a reason why it should go there instead?
          Hide
          Dan Poltawski added a comment -

          I agree with your assessment, +1

          Show
          Dan Poltawski added a comment - I agree with your assessment, +1
          Hide
          Dan Marsden added a comment -

          thanks Dan, rebased and pushing for integration.

          Show
          Dan Marsden added a comment - thanks Dan, rebased and pushing for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, the question is why is SCORM the only module doing completion tasks at scorm_grade_item_update(). I've looked to all the modules and none does that.

          Also, my feelings are about to think that the require_once() call must be as closer as possible to the code needing it. Just imagine I create any code calling scorm_grade_item_update(). It's that function responsibility (and not the caller) to include the needed libs.

          So, apart from the question about why SCORM is the only one doing that with completion... that perhaps will lead us to some new issue... I think the correct solution is to require_once(completionlib), immediately after the require_once(gradelib) and done.

          Reopening... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, the question is why is SCORM the only module doing completion tasks at scorm_grade_item_update(). I've looked to all the modules and none does that. Also, my feelings are about to think that the require_once() call must be as closer as possible to the code needing it. Just imagine I create any code calling scorm_grade_item_update(). It's that function responsibility (and not the caller) to include the needed libs. So, apart from the question about why SCORM is the only one doing that with completion... that perhaps will lead us to some new issue... I think the correct solution is to require_once(completionlib), immediately after the require_once(gradelib) and done. Reopening... ciao
          Hide
          Dan Marsden added a comment -

          yeah - good call - looking back at the code again myself... looks like there's a bigger issue there - enrolling a user could potentially end up flagging the module as complete? - need to look go and look back at where that was added to grade_item_update.

          Show
          Dan Marsden added a comment - yeah - good call - looking back at the code again myself... looks like there's a bigger issue there - enrolling a user could potentially end up flagging the module as complete? - need to look go and look back at where that was added to grade_item_update.
          Hide
          Dan Marsden added a comment -

          in fact... I think this might have already been fixed in MDL-33978 - so it may not be an issue? - Nicolas what Moodle version were you using when you saw this?

          Show
          Dan Marsden added a comment - in fact... I think this might have already been fixed in MDL-33978 - so it may not be an issue? - Nicolas what Moodle version were you using when you saw this?
          Hide
          Dan Marsden added a comment -

          nope - that fixes a similar issue but not the same.

          Show
          Dan Marsden added a comment - nope - that fixes a similar issue but not the same.
          Hide
          Dan Marsden added a comment -

          have updated the patch (not ready for complete peer review yet - needs phpdocs for starters) - have added Aaron/Simon to this patch as I'm keen for feedback from them before I submit this - Aaron wrote the completion code for SCORM for Totara.

          Show
          Dan Marsden added a comment - have updated the patch (not ready for complete peer review yet - needs phpdocs for starters) - have added Aaron/Simon to this patch as I'm keen for feedback from them before I submit this - Aaron wrote the completion code for SCORM for Totara.
          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
          Nicolas Dunand added a comment -

          Hi Dan, We noticed this on Moodle 2.3.1+ (Build: 20120726)

          Show
          Nicolas Dunand added a comment - Hi Dan, We noticed this on Moodle 2.3.1+ (Build: 20120726)
          Hide
          Aaron Barnes added a comment -

          Hi Dan,

          Are you able to tidy up the $updatecompletion parameter of scorm_grade_item_update() function which is no longer used?

          Also, in scorm_get_completion() could you use completion_info::is_enabled() rather than the $CFG check.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Dan, Are you able to tidy up the $updatecompletion parameter of scorm_grade_item_update() function which is no longer used? Also, in scorm_get_completion() could you use completion_info::is_enabled() rather than the $CFG check. Cheers, Aaron
          Hide
          Dan Marsden added a comment -

          Thanks Aaron - bouncing this up for peer review again.

          Show
          Dan Marsden added a comment - Thanks Aaron - bouncing this up for peer review again.
          Hide
          Aaron Barnes added a comment -

          Looks good to me!

          Show
          Aaron Barnes added a comment - Looks good to me!
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Marsden added a comment -

          rebased

          Show
          Dan Marsden added a comment - rebased
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23 & master), thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I trust you. Passing.

          Show
          Eloy Lafuente (stronk7) added a comment - I trust you. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed, many thanks for your awesome collaboration.

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your awesome collaboration.
          Hide
          Jay Knight added a comment -

          It looks like the changes from this issue are causing this bug: MDL-36107

          Show
          Jay Knight added a comment - It looks like the changes from this issue are causing this bug: MDL-36107

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: