Moodle

completion_criteria_completion can trigger a coding_exception

Details

  • Type: Bug Bug
  • Status: Closed 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 from Repository:
  • Pull Master Branch:
  • Pull Master Diff URL:

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

is a regression caused by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-32203 Course completion data object misses records

  • Major - Major loss of function, incorrect output
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
is duplicated by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-34642 Error when trying to mark an activity completed on Course Completion Report

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
will help resolve

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-34409 Teacher cannot mark course complete

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: