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

      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);
               }
       

        Gliffy Diagrams

          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: