Merge pull request #58 from catalyst/issue52

issue #52: implement a logical operator for rules
catalyst · Mar 25, 2024 · 292cdff · 292cdff
2 parents 01630a9 + 9dfe458
commit 292cdff
Show file tree

Hide file tree

Showing 13 changed files with 247 additions and 35 deletions.
diff --git a/README.md b/README.md
@@ -54,12 +54,13 @@ Conditions are simple predicates which assert something about a user in the syst
 
 ## Rules
 
-Rules are what determine if a user will be added or removed from a cohort. A rule is defined by two things:
+Rules are what determine if a user will be added or removed from a cohort. A rule is defined by few things:
 
 1. A cohort
 2. A set of conditions
+3. A logical operator to be applied for conditions (OR/AND)
 
-For a user to be added to the cohort specified by a rule, they must match all of the rule's conditions. 
+For users to be added to the cohort specified by a rule, they must match all of rule's conditions (logical operator AND) or any of rule's conditions (logical operator OR). 
 
 **NB:** A cohort can be managed by _one and only one_ rule. This is to prevent rules competing over users in a cohort (e.g., to avoid situations where Rule A wants users a, b, c to be in a cohort, but Rule B wants to remove user c from the same cohort). 
 

diff --git a/classes/condition_manager.php b/classes/condition_manager.php
@@ -211,12 +211,14 @@ public static function get_conditions_with_event(base $event): array {
      * A helper function to build sql data for given list of conditions.
      *
      * @param \tool_dynamic_cohorts\condition[] $conditions A list of conditions.
-     * @param int|null $userid
+     * @param string $operator Logical operator.
+     * @param int|null $userid Optional user id in case we need to build SQL for one user. For example when event is triggered.
      *
      * @return \tool_dynamic_cohorts\condition_sql
      */
-    public static function build_sql_data(array $conditions, ?int $userid = null): condition_sql {
-        $where = ' u.deleted = 0 ';
+    public static function build_sql_data(array $conditions, string $operator = rule_manager::CONDITIONS_OPERATOR_AND,
+                                          ?int $userid = null): condition_sql {
+        $where = '';
         $join = '';
         $params = [];
 
@@ -239,7 +241,10 @@ public static function build_sql_data(array $conditions, ?int $userid = null): c
             }
 
             if (!empty($sqldata->get_where())) {
-                $where .= ' AND (' . $sqldata->get_where() . ')';
+                if (!empty($where)) {
+                    $where .= ' ' . rule_manager::get_logical_operator_text($operator);
+                }
+                $where .= ' (' . $sqldata->get_where() . ')';
             }
 
             if (!empty($sqldata->get_params())) {
@@ -249,10 +254,12 @@ public static function build_sql_data(array $conditions, ?int $userid = null): c
 
         if ($userid) {
             $userparam = condition_sql::generate_param_alias();
-            $where .= " AND u.id = :{$userparam} ";
+            $where .= " AND ( u.id = :{$userparam}) ";
             $params += [$userparam => $userid];
         }
 
+        $where .= '  AND ( u.deleted = 0) ';
+
         return new condition_sql($join, $where, $params);
     }
 }
diff --git a/classes/reportbuilder/local/systemreports/matching_users.php b/classes/reportbuilder/local/systemreports/matching_users.php
@@ -52,7 +52,7 @@ protected function initialise(): void {
             $this->add_base_condition_sql(' true = false');
         }
 
-        $sql = condition_manager::build_sql_data($conditions);
+        $sql = condition_manager::build_sql_data($conditions, $rule->get('operator'));
 
         $this->add_base_fields('DISTINCT u.id');
         $this->add_join($sql->get_join());

diff --git a/classes/reportbuilder/local/systemreports/rules.php b/classes/reportbuilder/local/systemreports/rules.php
@@ -28,6 +28,7 @@
 use tool_dynamic_cohorts\rule;
 use pix_icon;
 use html_writer;
+use tool_dynamic_cohorts\rule_manager;
 
 /**
  * Rules admin table.
@@ -88,7 +89,7 @@ protected function initialise(): void {
         ))
             ->set_type(column::TYPE_TEXT)
             ->set_is_sortable(false)
-            ->add_fields("{$rulealias}.id")
+            ->add_fields("{$rulealias}.id, {$rulealias}.operator")
             ->add_callback(static function($id, $row): string {
                 $rule = new rule(0, $row);
                 $conditions = count($rule->get_condition_records());
@@ -99,8 +100,10 @@ protected function initialise(): void {
                         'data-ruleid' => $rule->get('id'),
                     ]);
                 }
-
-                return $conditions;
+                return  get_string('conditionstext', 'tool_dynamic_cohorts', (object)[
+                    'conditions' => $conditions,
+                    'operator' => rule_manager::get_logical_operator_text($rule->get('operator')),
+                ]);
             });
 
         $this->add_column_from_entity('rule_entity:status');

diff --git a/classes/rule.php b/classes/rule.php
@@ -65,6 +65,10 @@ protected static function define_properties() {
                 'type' => PARAM_INT,
                 'default' => 0,
             ],
+            'operator' => [
+                'type' => PARAM_INT,
+                'default' => 0,
+            ],
         ];
     }
 

diff --git a/classes/rule_form.php b/classes/rule_form.php
@@ -99,6 +99,13 @@ protected function definition() {
         );
         $mform->addHelpButton('bulkprocessing', 'bulkprocessing', 'tool_dynamic_cohorts');
 
+        $mform->addElement('select',
+            'operator',
+            get_string('logical_operator', 'tool_dynamic_cohorts'),
+            [rule_manager::CONDITIONS_OPERATOR_AND => 'AND', rule_manager::CONDITIONS_OPERATOR_OR => 'OR']);
+        $mform->addHelpButton('operator', 'logical_operator', 'tool_dynamic_cohorts');
+        $mform->setType('operator', PARAM_INT);
+
         $this->add_action_buttons();
     }
 

diff --git a/classes/rule_manager.php b/classes/rule_manager.php
@@ -42,6 +42,16 @@ class rule_manager {
      */
     const BULK_PROCESSING_SIZE = 10000;
 
+    /**
+     * Conditions logical operator AND.
+     */
+    const CONDITIONS_OPERATOR_AND = 0;
+
+    /**
+     * Conditions logical operator OR.
+     */
+    const CONDITIONS_OPERATOR_OR = 1;
+
     /**
      * Builds rule edit URL.
      *
@@ -121,6 +131,7 @@ public static function process_form(\stdClass $formdata): rule {
             'cohortid' => $formdata->cohortid,
             'description' => $formdata->description,
             'bulkprocessing' => $formdata->bulkprocessing,
+            'operator' => $formdata->operator,
         ];
 
         $oldcohortid = 0;
@@ -223,7 +234,7 @@ public static function get_matching_users(rule $rule, ?int $userid = null): arra
         $sql = "SELECT DISTINCT u.id FROM {user} u";
 
         try {
-            $sqldata = condition_manager::build_sql_data($conditions, $userid);
+            $sqldata = condition_manager::build_sql_data($conditions, $rule->get('operator'), $userid);
         } catch (\Exception $exception ) {
             self::trigger_matching_failed_event($rule, $exception->getMessage());
             $rule->mark_broken();
@@ -261,7 +272,7 @@ public static function get_matching_users_count(rule $rule, ?int $userid = null)
         $sql = "SELECT COUNT(DISTINCT u.id) cnt FROM {user} u";
 
         try {
-            $sqldata = condition_manager::build_sql_data($conditions, $userid);
+            $sqldata = condition_manager::build_sql_data($conditions, $rule->get('operator'), $userid);
         } catch (\Exception $exception ) {
             self::trigger_matching_failed_event($rule, $exception->getMessage());
             $rule->mark_broken();
@@ -400,4 +411,14 @@ public static function get_rules_with_condition(condition_base $condition): arra
 
         return $rules;
     }
+
+    /**
+     * Returns logical operator text based on value.
+     *
+     * @param int $operator Operator value.
+     * @return string
+     */
+    public static function get_logical_operator_text(int $operator): string {
+        return $operator == self::CONDITIONS_OPERATOR_AND ? 'AND' : 'OR';
+    }
 }
diff --git a/db/install.xml b/db/install.xml
@@ -13,6 +13,7 @@
         <FIELD NAME="enabled" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Is the rule enabled"/>
         <FIELD NAME="bulkprocessing" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Should the rule be processed in bulk"/>
         <FIELD NAME="broken" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Is this rule broken?"/>
+        <FIELD NAME="operator" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false" COMMENT="Logical operator (OR/AND) for all conditions in the rule"/>
         <FIELD NAME="usermodified" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
         <FIELD NAME="timecreated" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
         <FIELD NAME="timemodified" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>

diff --git a/db/upgrade.php b/db/upgrade.php
@@ -0,0 +1,48 @@
+<?php
+// This file is part of Moodle - https://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.
+
+/**
+ * Upgrade hook.
+ *
+ * @package     tool_dynamic_cohorts
+ * @copyright   2024 Catalyst IT
+ * @license     https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+/**
+ * Upgrade the plugin.
+ *
+ * @param int $oldversion The old version of the plugin
+ * @return bool
+ */
+function xmldb_tool_dynamic_cohorts_upgrade($oldversion): bool {
+    global $DB;
+
+    $dbman = $DB->get_manager();
+
+    if ($oldversion < 2024032501) {
+        $table = new xmldb_table('tool_dynamic_cohorts');
+        $field = new xmldb_field('operator', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, 0);
+
+        if (!$dbman->field_exists($table, $field)) {
+            $dbman->add_field($table, $field);
+        }
+
+        upgrade_plugin_savepoint(true, 2024032501, 'tool', 'dynamic_cohorts');
+    }
+
+    return true;
+}
diff --git a/lang/en/tool_dynamic_cohorts.php b/lang/en/tool_dynamic_cohorts.php
@@ -40,6 +40,7 @@
 $string['cohortid_help'] = 'A cohort to manage as part of this rule. Only cohorts that are not managed by other plugins are displayed in this list.';
 $string['condition'] = 'Condition';
 $string['conditions'] = 'Conditions';
+$string['conditionstext'] = '{$a->conditions} ( logical {$a->operator} )';
 $string['conditionsformtitle'] = 'Rule conditions';
 $string['conditionchnagesnotapplied'] = 'Condition changes are not applied until you save the rule form';
 $string['conditionformtitle'] = 'Rule condition';
@@ -81,6 +82,8 @@
 $string['ismemberof'] = 'is member of';
 $string['isnotmemberof'] = 'is not member of';
 $string['isnotempty'] = 'is not empty';
+$string['logical_operator'] = 'Logical operator';
+$string['logical_operator_help'] = 'A logical operator to be applied to conditions for this rule. Operator "AND" means a user has to match all conditions to be added to a cohort. "OR" means a user has to match any of conditions to be added to a cohort.';
 $string['managerules'] = 'Manage rules';
 $string['managecohorts'] = 'Manage cohorts';
 $string['matchingusers'] = 'Matching users';

diff --git a/tests/condition_manager_test.php b/tests/condition_manager_test.php
@@ -20,6 +20,7 @@
 use tool_dynamic_cohorts\event\condition_created;
 use tool_dynamic_cohorts\event\condition_deleted;
 use tool_dynamic_cohorts\event\condition_updated;
+use tool_dynamic_cohorts\local\tool_dynamic_cohorts\condition\user_profile;
 
 /**
  * Tests for condition manager class.
@@ -210,4 +211,68 @@ public function test_get_conditions_with_event() {
         $this->assertArrayHasKey('tool_dynamic_cohorts\local\tool_dynamic_cohorts\condition\user_custom_profile', $conditions);
         $this->assertArrayHasKey('tool_dynamic_cohorts\local\tool_dynamic_cohorts\condition\user_profile', $conditions);
     }
+
+    /**
+     * Basic test of building SQL data.
+     */
+    public function test_build_sql_data() {
+        $this->resetAfterTest();
+
+        $this->getDataGenerator()->create_user(['username' => 'user1username']);
+        $this->getDataGenerator()->create_user(['username' => 'user2username']);
+        $this->getDataGenerator()->create_user(['username' => 'test']);
+
+        $cohort = $this->getDataGenerator()->create_cohort();
+
+        $rule = new rule(0, (object)['name' => 'Test rule 1', 'cohortid' => $cohort->id,
+            'operator' => rule_manager::CONDITIONS_OPERATOR_OR]);
+        $rule->save();
+
+        $conditions = [];
+
+        $condition = user_profile::get_instance(0, (object)['ruleid' => $rule->get('id'), 'sortorder' => 1]);
+        $condition->set_config_data([
+            'profilefield' => 'username',
+            'username_operator' => condition_base::TEXT_IS_EQUAL_TO,
+            'username_value' => 'user1username',
+        ]);
+        $condition->get_record()->save();
+        $conditions[] = $condition->get_record();
+
+        $condition = user_profile::get_instance(0, (object)['ruleid' => $rule->get('id'), 'sortorder' => 1]);
+        $condition->set_config_data([
+            'profilefield' => 'username',
+            'username_operator' => condition_base::TEXT_IS_EQUAL_TO,
+            'username_value' => 'user2username',
+        ]);
+        $condition->get_record()->save();
+        $conditions[] = $condition->get_record();
+
+        $sql = condition_manager::build_sql_data($conditions);
+        $this->assertEquals('', $sql->get_join());
+        $this->assertStringNotContainsString('OR', $sql->get_where());
+        $this->assertStringContainsString('AND ( u.deleted = 0)', $sql->get_where());
+        $this->assertTrue(in_array('user1username', $sql->get_params()));
+        $this->assertTrue(in_array('user2username', $sql->get_params()));
+        $this->assertStringNotContainsString('AND ( u.id = ', $sql->get_where());
+        $this->assertFalse(in_array(777, $sql->get_params()));
+
+        $sql = condition_manager::build_sql_data($conditions, rule_manager::CONDITIONS_OPERATOR_OR);
+        $this->assertEquals('', $sql->get_join());
+        $this->assertStringContainsString('OR', $sql->get_where());
+        $this->assertStringContainsString('AND ( u.deleted = 0)', $sql->get_where());
+        $this->assertTrue(in_array('user1username', $sql->get_params()));
+        $this->assertTrue(in_array('user2username', $sql->get_params()));
+        $this->assertStringNotContainsString('AND ( u.id = ', $sql->get_where());
+        $this->assertFalse(in_array(777, $sql->get_params()));
+
+        $sql = condition_manager::build_sql_data($conditions, rule_manager::CONDITIONS_OPERATOR_OR, 777);
+        $this->assertEquals('', $sql->get_join());
+        $this->assertStringContainsString('OR', $sql->get_where());
+        $this->assertStringContainsString('AND ( u.deleted = 0)', $sql->get_where());
+        $this->assertStringContainsString('AND ( u.id = ', $sql->get_where());
+        $this->assertTrue(in_array('user1username', $sql->get_params()));
+        $this->assertTrue(in_array('user2username', $sql->get_params()));
+        $this->assertTrue(in_array(777, $sql->get_params()));
+    }
 }