From d8085396de526e56cd1c730b75bede9ffc7c56d4 Mon Sep 17 00:00:00 2001 From: Marty Gilbert Date: Tue, 23 Jun 2020 08:27:40 -0400 Subject: [PATCH 1/2] MDL-69102 badge: Fixes broken WHERE clause When buildilng the 'WHERE' clause, the SQL generated a query that selected any user who was a member of the **last** group, not all of the groups. I believe the query has to be re-worked so that it only returns users who are in ALL of the groups. This can be done by a GROUP BY and HAVING. --- badges/criteria/award_criteria_cohort.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/badges/criteria/award_criteria_cohort.php b/badges/criteria/award_criteria_cohort.php index 1e0bb80ad53..81290eb07a8 100644 --- a/badges/criteria/award_criteria_cohort.php +++ b/badges/criteria/award_criteria_cohort.php @@ -240,13 +240,23 @@ class award_criteria_cohort extends award_criteria { return array($join, $where, $params); } else { // User is a member of ALL of the specified cohorts. - $join = " LEFT JOIN {cohort_members} cm ON cm.userid = u.id"; + $join = ' LEFT JOIN {cohort_members} cm ON cm.userid = u.id'; + $where = ' AND ('; $i = 0; foreach ($this->params as $param) { - $i++; - $where = ' AND cm.cohortid = :cohortid'.$i; + if ($i == 0) { + $where .= 'cm.cohortid = :cohortid'.$i; + } else { + $where .= ' OR cm.cohortid = :cohortid'.$i; + } $params['cohortid'.$i] = $param['cohort']; + $i++; } + $where .= ') + GROUP BY u.id, bi.badgeid + HAVING COUNT(cm.cohortid) = :cohortcount'; + $params['cohortcount'] = $i; + return array($join, $where, $params); } } From 377d6dc12a857bb68fe804cfd0cff8af37ca1f90 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Thu, 8 Apr 2021 19:19:09 +0200 Subject: [PATCH 2/2] MDL-69102 phpunit: Add test to cover ALL cohort criteria That way we can check that the expectations are working ok for every database. Previously only the ANY case was being tested. Also, convert them to course badges and check, via review_all_criteria() that the SQL structures returned by award_criteria_cohort->get_completed_criteria_sql() doesn't fail and return the expected counters. --- badges/tests/badgeslib_test.php | 108 +++++++++++++++++++++++++++++--- 1 file changed, 100 insertions(+), 8 deletions(-) diff --git a/badges/tests/badgeslib_test.php b/badges/tests/badgeslib_test.php index fa0baa75c14..3231db1628f 100644 --- a/badges/tests/badgeslib_test.php +++ b/badges/tests/badgeslib_test.php @@ -624,35 +624,127 @@ class badgeslib_test extends advanced_testcase { } /** - * Test badges observer when cohort_member_added event is fired. + * Test badges observer when cohort_member_added event is fired and user required to belong to any cohort. + * + * @covers award_criteria_cohort */ - public function test_badges_observer_cohort_criteria_review() { + public function test_badges_observer_any_cohort_criteria_review() { global $CFG; require_once("$CFG->dirroot/cohort/lib.php"); - $cohort = $this->getDataGenerator()->create_cohort(); + $cohort1 = $this->getDataGenerator()->create_cohort(); + $cohort2 = $this->getDataGenerator()->create_cohort(); $this->preventResetByRollback(); // Messaging is not compatible with transactions. + $badge = new badge($this->badgeid); $this->assertFalse($badge->is_issued($this->user->id)); + $this->assertSame(0, $badge->review_all_criteria()); // Verify award_criteria_cohort->get_completed_criteria_sql(). // Set up the badge criteria. $criteriaoverall = award_criteria::build(array('criteriatype' => BADGE_CRITERIA_TYPE_OVERALL, 'badgeid' => $badge->id)); $criteriaoverall->save(array('agg' => BADGE_CRITERIA_AGGREGATION_ANY)); $criteriaoverall1 = award_criteria::build(array('criteriatype' => BADGE_CRITERIA_TYPE_COHORT, 'badgeid' => $badge->id)); - $criteriaoverall1->save(array('agg' => BADGE_CRITERIA_AGGREGATION_ANY, 'cohort_cohorts' => array('0' => $cohort->id))); - - // Make the badge active. + $criteriaoverall1->save(array('agg' => BADGE_CRITERIA_AGGREGATION_ANY, + 'cohort_cohorts' => array('0' => $cohort1->id, '1' => $cohort2->id))); $badge->set_status(BADGE_STATUS_ACTIVE); + // Reload it to contain criteria. + $badge = new badge($this->badgeid); + $this->assertFalse($badge->is_issued($this->user->id)); + $this->assertSame(0, $badge->review_all_criteria()); // Verify award_criteria_cohort->get_completed_criteria_sql(). + // Add the user to the cohort. - cohort_add_member($cohort->id, $this->user->id); + cohort_add_member($cohort2->id, $this->user->id); + $this->assertDebuggingCalled(); // Verify that the badge was awarded. - $this->assertDebuggingCalled(); $this->assertTrue($badge->is_issued($this->user->id)); + // As the badge has been awarded to user because core_badges_observer been called when the member has been added to the + // cohort, there are no other users that can award this badge. + $this->assertSame(0, $badge->review_all_criteria()); // Verify award_criteria_cohort->get_completed_criteria_sql(). + } + /** + * Test badges observer when cohort_member_added event is fired and user required to belong to multiple (all) cohorts. + * + * @covers award_criteria_cohort + */ + public function test_badges_observer_all_cohort_criteria_review() { + global $CFG; + + require_once("$CFG->dirroot/cohort/lib.php"); + + $cohort1 = $this->getDataGenerator()->create_cohort(); + $cohort2 = $this->getDataGenerator()->create_cohort(); + $cohort3 = $this->getDataGenerator()->create_cohort(); + + // Add user2 to cohort1 and cohort3. + $user2 = $this->getDataGenerator()->create_user(); + cohort_add_member($cohort3->id, $user2->id); + cohort_add_member($cohort1->id, $user2->id); + + // Add user3 to cohort1, cohort2 and cohort3. + $user3 = $this->getDataGenerator()->create_user(); + cohort_add_member($cohort1->id, $user3->id); + cohort_add_member($cohort2->id, $user3->id); + cohort_add_member($cohort3->id, $user3->id); + + $this->preventResetByRollback(); // Messaging is not compatible with transactions. + + // Cohort criteria are used in site badges. + $badge = new badge($this->badgeid); + + $this->assertFalse($badge->is_issued($this->user->id)); + $this->assertSame(0, $badge->review_all_criteria()); // Verify award_criteria_cohort->get_completed_criteria_sql(). + + // Set up the badge criteria. + $criteriaoverall = award_criteria::build(array('criteriatype' => BADGE_CRITERIA_TYPE_OVERALL, 'badgeid' => $badge->id)); + $criteriaoverall->save(array('agg' => BADGE_CRITERIA_AGGREGATION_ANY)); + $criteriaoverall1 = award_criteria::build(array('criteriatype' => BADGE_CRITERIA_TYPE_COHORT, 'badgeid' => $badge->id)); + $criteriaoverall1->save(array('agg' => BADGE_CRITERIA_AGGREGATION_ALL, + 'cohort_cohorts' => array('0' => $cohort1->id, '1' => $cohort2->id, '2' => $cohort3->id))); + $badge->set_status(BADGE_STATUS_ACTIVE); + + // Reload it to contain criteria. + $badge = new badge($this->badgeid); + + // Verify that the badge was not awarded yet (ALL cohorts are needed and review_all_criteria has to be called). + $this->assertFalse($badge->is_issued($this->user->id)); + $this->assertFalse($badge->is_issued($user2->id)); + $this->assertFalse($badge->is_issued($user3->id)); + + // Verify that after calling review_all_criteria, users with the criteria (user3) award the badge instantly. + $this->assertSame(1, $badge->review_all_criteria()); // Verify award_criteria_cohort->get_completed_criteria_sql(). + $this->assertFalse($badge->is_issued($this->user->id)); + $this->assertFalse($badge->is_issued($user2->id)); + $this->assertTrue($badge->is_issued($user3->id)); + $this->assertDebuggingCalled(); + + // Add the user to the cohort1. + cohort_add_member($cohort1->id, $this->user->id); + + // Verify that the badge was not awarded yet (ALL cohorts are needed). + $this->assertFalse($badge->is_issued($this->user->id)); + $this->assertSame(0, $badge->review_all_criteria()); // Verify award_criteria_cohort->get_completed_criteria_sql(). + + // Add the user to the cohort3. + cohort_add_member($cohort3->id, $this->user->id); + + // Verify that the badge was not awarded yet (ALL cohorts are needed). + $this->assertFalse($badge->is_issued($this->user->id)); + $this->assertSame(0, $badge->review_all_criteria()); // Verify award_criteria_cohort->get_completed_criteria_sql(). + + // Add user to cohort2. + cohort_add_member($cohort2->id, $this->user->id); + $this->assertDebuggingCalled(); + + // Verify that the badge was awarded (ALL cohorts). + $this->assertTrue($badge->is_issued($this->user->id)); + // As the badge has been awarded to user because core_badges_observer been called when the member has been added to the + // cohort, there are no other users that can award this badge. + $this->assertSame(0, $badge->review_all_criteria()); // Verify award_criteria_cohort->get_completed_criteria_sql(). } /**