Moodle
  1. Moodle
  2. MDL-34243

completion_criteria_completion can trigger a coding_exception

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.1
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Course completion
    • Labels:
      None
    • Testing Instructions:
      Hide

      Enable course completion site-wide in the advanced settings page.
      Create a course with completion enabled.
      Edit the course's completion tracking and enable manual self completion.
      Add the completion status block to the course.
      Enrol a user in the course.
      View the course page as that user.

      No errors should occur.

      Show
      Enable course completion site-wide in the advanced settings page. Create a course with completion enabled. Edit the course's completion tracking and enable manual self completion. Add the completion status block to the course. Enrol a user in the course. View the course page as that user. No errors should occur.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:

      Description

      If the get_criteria() method is called on the completion_criteria_completion object and it attempts to load the criteria the exception will be triggered.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Aaron Barnes added a comment -

            This doesn't occurring during normal use of course completion, but could be a problem if people have customised completion.

            Show
            Aaron Barnes added a comment - This doesn't occurring during normal use of course completion, but could be a problem if people have customised completion.
            Hide
            Sam Hemelryk added a comment -

            Hi Aaron,

            I was just looking at this change (checking over issues for the next integration run) and ran a quick grep on master (grep -Frn 'completion_criteria::factory' .).
            I see there are 3 calls to that static factory method, 2 of which appear to pass objects and 1 that passes and array.

            ./lib/completionlib.php:366:                $this->criteria[$record->id] = completion_criteria::factory((array)$record);
            ./lib/completion/completion_criteria_completion.php:152:            $this->attach_criteria(completion_criteria::factory($record));
            ./course/togglecompletion.php:56:        completion_criteria::factory((object) array('id'=>$rolec, 'criteriatype'=>COMPLETION_CRITERIA_TYPE_ROLE)); //TODO: this is dumb, because it does not fetch the data?!?!
            

            Looks like the ./course/togglecompletion.php script needs to be updated as well.
            Perhaps its worth producing separate stable branches and adding an array type hint to the factory method in master only to ensure future calls are made correctly.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Aaron, I was just looking at this change (checking over issues for the next integration run) and ran a quick grep on master ( grep -Frn 'completion_criteria::factory' . ). I see there are 3 calls to that static factory method, 2 of which appear to pass objects and 1 that passes and array. ./lib/completionlib.php:366: $this->criteria[$record->id] = completion_criteria::factory((array)$record); ./lib/completion/completion_criteria_completion.php:152: $this->attach_criteria(completion_criteria::factory($record)); ./course/togglecompletion.php:56: completion_criteria::factory((object) array('id'=>$rolec, 'criteriatype'=>COMPLETION_CRITERIA_TYPE_ROLE)); //TODO: this is dumb, because it does not fetch the data?!?! Looks like the ./course/togglecompletion.php script needs to be updated as well. Perhaps its worth producing separate stable branches and adding an array type hint to the factory method in master only to ensure future calls are made correctly. Cheers Sam
            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
            Sam Hemelryk added a comment -

            Thanks Aaron, this has been integrated now.

            I had a think about togglecompletion.php and decided it would be easiest to fix it during integration.
            As such I made an additional commit (9df8150) and then backported both your commit and my own to 23, and 22.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Thanks Aaron, this has been integrated now. I had a think about togglecompletion.php and decided it would be easiest to fix it during integration. As such I made an additional commit (9df8150) and then backported both your commit and my own to 23, and 22. Cheers Sam
            Hide
            Rossiani Wijaya added a comment -

            Hi Aaron,

            Could you provide testing instructions for this issue?

            Thanks.

            Show
            Rossiani Wijaya added a comment - Hi Aaron, Could you provide testing instructions for this issue? Thanks.
            Hide
            Aaron Barnes added a comment -

            Hi,

            Added the best instructions I can, these will make sure that this patch doesn't cause any regressions.

            Cheers,
            Aaron

            Show
            Aaron Barnes added a comment - Hi, Added the best instructions I can, these will make sure that this patch doesn't cause any regressions. Cheers, Aaron
            Hide
            Rossiani Wijaya added a comment -

            This looks good.

            Thanks Aaron.

            Test passed.

            Show
            Rossiani Wijaya added a comment - This looks good. Thanks Aaron. Test passed.
            Hide
            Aparup Banerjee added a comment -

            yay, it works!

            This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

            Thank you all for taking the time to get us here.

            cheers!

            Show
            Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: