From ed1a2e8a5499288c9d860b45ebc853f467ce128f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Monlla=C3=B3?= Date: Fri, 17 May 2019 12:23:12 +0200 Subject: [PATCH] MDL-65634 analytics: Discard late and early student enrolments --- .../analytics/target/course_competencies.php | 5 ++ .../analytics/target/course_completion.php | 4 ++ .../analytics/target/course_dropout.php | 5 ++ .../analytics/target/course_enrolments.php | 44 ++++++++++++++++-- .../analytics/target/course_gradetopass.php | 5 ++ lib/tests/targets_test.php | 46 ++++++++++++++++--- 6 files changed, 99 insertions(+), 10 deletions(-) diff --git a/course/classes/analytics/target/course_competencies.php b/course/classes/analytics/target/course_competencies.php index 0aea6268db6..046013b54bf 100644 --- a/course/classes/analytics/target/course_competencies.php +++ b/course/classes/analytics/target/course_competencies.php @@ -118,6 +118,11 @@ class course_competencies extends course_enrolments { */ protected function calculate_sample($sampleid, \core_analytics\analysable $course, $starttime = false, $endtime = false) { + if ($this->enrolment_starts_after_calculation_start($sampleid, $starttime)) { + // Discard user enrolments whose start date is after $starttime. + return null; + } + $userenrol = $this->retrieve('user_enrolments', $sampleid); $key = $course->get_id(); diff --git a/course/classes/analytics/target/course_completion.php b/course/classes/analytics/target/course_completion.php index 9c187ca76b7..75f0d82c1c3 100644 --- a/course/classes/analytics/target/course_completion.php +++ b/course/classes/analytics/target/course_completion.php @@ -96,6 +96,10 @@ class course_completion extends course_enrolments { */ protected function calculate_sample($sampleid, \core_analytics\analysable $course, $starttime = false, $endtime = false) { + if ($this->enrolment_starts_after_calculation_start($sampleid, $starttime)) { + // Discard user enrolments whose start date is after $starttime. + return null; + } $userenrol = $this->retrieve('user_enrolments', $sampleid); // We use completion as a success metric. diff --git a/course/classes/analytics/target/course_dropout.php b/course/classes/analytics/target/course_dropout.php index 694b0581079..4cb43c7a82a 100644 --- a/course/classes/analytics/target/course_dropout.php +++ b/course/classes/analytics/target/course_dropout.php @@ -118,6 +118,11 @@ class course_dropout extends course_enrolments { */ protected function calculate_sample($sampleid, \core_analytics\analysable $course, $starttime = false, $endtime = false) { + if ($this->enrolment_starts_after_calculation_start($sampleid, $starttime)) { + // Discard user enrolments whose start date is after $starttime. + return null; + } + $userenrol = $this->retrieve('user_enrolments', $sampleid); // We use completion as a success metric only when it is enabled. diff --git a/course/classes/analytics/target/course_enrolments.php b/course/classes/analytics/target/course_enrolments.php index e97112a4172..e3384132438 100644 --- a/course/classes/analytics/target/course_enrolments.php +++ b/course/classes/analytics/target/course_enrolments.php @@ -122,6 +122,8 @@ abstract class course_enrolments extends \core_analytics\local\target\binary { */ public function is_valid_sample($sampleid, \core_analytics\analysable $course, $fortraining = true) { + $now = time(); + $userenrol = $this->retrieve('user_enrolments', $sampleid); if ($userenrol->timeend && $course->get_start() > $userenrol->timeend) { // Discard enrolments which time end is prior to the course start. This should get rid of @@ -139,9 +141,22 @@ abstract class course_enrolments extends \core_analytics\local\target\binary { return false; } - if (($userenrol->timestart && $userenrol->timestart > $course->get_end()) || - (!$userenrol->timestart && $userenrol->timecreated > $course->get_end())) { - // Discard user enrolments that starts after the analysable official end. + if ($course->get_end()) { + if (($userenrol->timestart && $userenrol->timestart > $course->get_end()) || + (!$userenrol->timestart && $userenrol->timecreated > $course->get_end())) { + // Discard user enrolments that starts after the analysable official end. + return false; + } + + } + + if ($now < $userenrol->timestart && $userenrol->timestart) { + // Discard enrolments whose start date is after now (no need to check timecreated > $now :P). + return false; + } + + if (!$fortraining && $userenrol->timeend && $userenrol->timeend < $now) { + // We don't want to generate predictions for finished enrolments. return false; } @@ -182,4 +197,27 @@ abstract class course_enrolments extends \core_analytics\local\target\binary { return array_merge($actions, parent::prediction_actions($prediction, $includedetailsaction)); } + + /** + * Does the user enrolment created after this time range start time or starts after it? + * + * We need to identify these enrolments because the indicators can not be calculated properly + * if the student enrolment started half way through this time range. + * + * User enrolments whose end date is before time() have already been discarded in + * course_enrolments::is_valid_sample. + * + * @param int $sampleid + * @param int $starttime + * @return bool + */ + protected function enrolment_starts_after_calculation_start(int $sampleid, int $starttime) { + + $userenrol = $this->retrieve('user_enrolments', $sampleid); + if ($userenrol->timestart && $userenrol->timestart > $starttime) { + return true; + } + + return false; + } } diff --git a/course/classes/analytics/target/course_gradetopass.php b/course/classes/analytics/target/course_gradetopass.php index cb5727c393d..84152eb2b70 100644 --- a/course/classes/analytics/target/course_gradetopass.php +++ b/course/classes/analytics/target/course_gradetopass.php @@ -164,6 +164,11 @@ class course_gradetopass extends course_enrolments { */ protected function calculate_sample($sampleid, \core_analytics\analysable $course, $starttime = false, $endtime = false) { + if ($this->enrolment_starts_after_calculation_start($sampleid, $starttime)) { + // Discard user enrolments whose start date is after $starttime. + return null; + } + $userenrol = $this->retrieve('user_enrolments', $sampleid); // Get course grade to pass. diff --git a/lib/tests/targets_test.php b/lib/tests/targets_test.php index 616ecc99db9..a61cb8365d3 100644 --- a/lib/tests/targets_test.php +++ b/lib/tests/targets_test.php @@ -148,21 +148,40 @@ class core_analytics_targets_testcase extends advanced_testcase { 'courseend' => $now + (WEEKSECS * 8), 'timestart' => $now, 'timeend' => $now - DAYSECS, - 'isvalid' => false + 'isvalidfortraining' => false, + 'isvalidforprediction' => false ], 'enrolmenttoolong' => [ 'coursestart' => $now, 'courseend' => $now + (WEEKSECS * 8), 'timestart' => $now - (YEARSECS + (WEEKSECS * 8)), 'timeend' => $now + (WEEKSECS * 8), - 'isvalid' => false + 'isvalidfortraining' => false, + 'isvalidforprediction' => false ], 'enrolmentstartaftercourse' => [ 'coursestart' => $now, 'courseend' => $now + (WEEKSECS * 8), 'timestart' => $now + (WEEKSECS * 9), 'timeend' => $now + (WEEKSECS * 10), - 'isvalid' => false + 'isvalidfortraining' => false, + 'isvalidforprediction' => false + ], + 'enrolmentstartsafternow' => [ + 'coursestart' => $now, + 'courseend' => $now + (WEEKSECS * 8), + 'timestart' => $now + (WEEKSECS * 2), + 'timeend' => $now + (WEEKSECS * 7), + 'isvalidfortraining' => false, + 'isvalidforprediction' => false + ], + 'enrolmentfinishedbeforenow' => [ + 'coursestart' => $now - (WEEKSECS * 4), + 'courseend' => $now - (WEEKSECS * 1), + 'timestart' => $now - (WEEKSECS * 3), + 'timeend' => $now - (WEEKSECS * 2), + 'isvalidfortraining' => true, + 'isvalidforprediction' => false ], ]; } @@ -228,9 +247,11 @@ class core_analytics_targets_testcase extends advanced_testcase { * @param int $courseend Course end date * @param int $timestart Enrol start date * @param int $timeend Enrol end date - * @param boolean $isvalid True when sample is valid, false when it is not + * @param boolean $isvalidfortraining True when sample is valid for training, false when it is not + * @param boolean $isvalidforprediction True when sample is valid for prediction, false when it is not */ - public function test_core_target_course_completion_samples($coursestart, $courseend, $timestart, $timeend, $isvalid) { + public function test_core_target_course_completion_samples($coursestart, $courseend, $timestart, $timeend, + $isvalidfortraining, $isvalidforprediction) { $this->resetAfterTest(true); @@ -254,7 +275,8 @@ class core_analytics_targets_testcase extends advanced_testcase { $target->add_sample_data($samplesdata); $sampleid = reset($sampleids); - $this->assertEquals($isvalid, $target->is_valid_sample($sampleid, $analysable)); + $this->assertEquals($isvalidfortraining, $target->is_valid_sample($sampleid, $analysable, true)); + $this->assertEquals($isvalidforprediction, $target->is_valid_sample($sampleid, $analysable, false)); } /** @@ -391,11 +413,15 @@ class core_analytics_targets_testcase extends advanced_testcase { $student1 = $dg->create_user(); $student2 = $dg->create_user(); $student3 = $dg->create_user(); + $student4 = $dg->create_user(); $studentrole = $DB->get_record('role', array('shortname' => 'student')); $dg->enrol_user($student1->id, $course1->id, $studentrole->id); $dg->enrol_user($student2->id, $course1->id, $studentrole->id); $dg->enrol_user($student3->id, $course1->id, $studentrole->id); + $enrolstart = mktime(0, 0, 0, 10, 25, 2015); + $dg->enrol_user($student4->id, $course1->id, $studentrole->id, 'manual', $enrolstart); + // get_all_samples() does not guarantee any order, so let's // explicitly define the expectations here for later comparing. // Expectations format being array($userid => expectation, ...) @@ -413,6 +439,9 @@ class core_analytics_targets_testcase extends advanced_testcase { // Student 3 (has no grade) fails, so it's non achieved sample. $expectations[$student3->id] = 1; + // Student 4 should be null as its enrolment timestart is after the this range. + $expectations[$student4->id] = null; + $courseitem->gradepass = 50; $DB->update_record('grade_items', $courseitem); @@ -431,9 +460,12 @@ class core_analytics_targets_testcase extends advanced_testcase { $method = $class->getMethod('calculate_sample'); $method->setAccessible(true); + $starttime = mktime(0, 0, 0, 10, 24, 2015); + // Verify all the expectations are fulfilled. foreach ($sampleids as $sampleid => $key) { - $this->assertEquals($expectations[$samplesdata[$key]['user']->id], $method->invoke($target, $sampleid, $analysable)); + $this->assertEquals($expectations[$samplesdata[$key]['user']->id], $method->invoke($target, $sampleid, + $analysable, $starttime)); } } }