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:
    • Rank:
      42588

      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.

        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: