MDL-82068 reportbuilder: re-factor SQL JOIN collection to trait.

Replace identical implementation of the same in various classes with
usage of the new trait. Implement constructor property promotion in
modified classes at the same time, to help clean up/clarify class
properties.

reportbuilder/classes/local/entities/base.php

@@ -19,7 +19,7 @@ declare(strict_types=1);
 namespace core_reportbuilder\local\entities;
 
 use coding_exception;
-use core_reportbuilder\local\helpers\database;
+use core_reportbuilder\local\helpers\{database, join_trait};
 use core_reportbuilder\local\report\column;
 use core_reportbuilder\local\report\filter;
 use lang_string;
@@ -33,6 +33,8 @@ use lang_string;
  */
 abstract class base {
 
+    use join_trait;
+
     /** @var string $entityname Internal reference to name of entity */
     private $entityname = null;
 
@@ -45,9 +47,6 @@ abstract class base {
     /** @var array $tablejoinaliases Database tables that have already been joined to the report and their aliases */
     private $tablejoinaliases = [];
 
-    /** @var string[] $joins List of SQL joins for the entity */
-    private $joins = [];
-
     /** @var column[] $columns List of columns for the entity */
     private $columns = [];
 
@@ -253,39 +252,6 @@ abstract class base {
         return array_key_exists($tablename, $this->tablejoinaliases);
     }
 
-    /**
-     * Add join clause required for this entity to join to existing tables/entities
-     *
-     * @param string $join
-     * @return self
-     */
-    final public function add_join(string $join): self {
-        $this->joins[trim($join)] = trim($join);
-        return $this;
-    }
-
-    /**
-     * Add multiple join clauses required for this entity {@see add_join}
-     *
-     * @param string[] $joins
-     * @return self
-     */
-    final public function add_joins(array $joins): self {
-        foreach ($joins as $join) {
-            $this->add_join($join);
-        }
-        return $this;
-    }
-
-    /**
-     * Return entity joins
-     *
-     * @return string[]
-     */
-    final public function get_joins(): array {
-        return array_values($this->joins);
-    }
-
     /**
      * Helper method for returning joins necessary for retrieving tags related to the current entity
      *

reportbuilder/classes/local/helpers/custom_fields.php

@@ -40,66 +40,32 @@ use core_customfield\handler;
  */
 class custom_fields {
 
-    /** @var string $entityname Name of the entity */
+    use join_trait;
-    private $entityname;
 
     /** @var handler $handler The handler for the customfields */
-    private $handler;
+    private handler $handler;
-
-    /** @var int $tablefieldalias The table alias and the field name (table.field) that matches the customfield instanceid. */
-    private $tablefieldalias;
-
-    /** @var array additional joins */
-    private $joins = [];
 
     /**
-     * Class customfields constructor.
+     * Constructor
      *
-     * @param string $tablefieldalias table alias and the field name (table.field) that matches the customfield instanceid.
+     * @param string $tablefieldalias The table/field alias to match the instance ID when adding columns and filters.
-     * @param string $entityname name of the entity in the report where we add custom fields.
+     * @param string $entityname The entity name used when adding columns and filters.
      * @param string $component component name of full frankenstyle plugin name.
      * @param string $area name of the area (each component/plugin may define handlers for multiple areas).
      * @param int $itemid item id if the area uses them (usually not used).
      */
-    public function __construct(string $tablefieldalias, string $entityname, string $component, string $area, int $itemid = 0) {
+    public function __construct(
-        $this->tablefieldalias = $tablefieldalias;
+        /** @var string The table/field alias to match the instance ID when adding columns and filters */
-        $this->entityname = $entityname;
+        private readonly string $tablefieldalias,
+        /** @var string The entity name used when adding columns and filters */
+        private readonly string $entityname,
+        string $component,
+        string $area,
+        int $itemid = 0,
+    ) {
         $this->handler = handler::get_handler($component, $area, $itemid);
     }
 
-    /**
-     * Additional join that is needed.
-     *
-     * @param string $join
-     * @return self
-     */
-    public function add_join(string $join): self {
-        $this->joins[trim($join)] = trim($join);
-        return $this;
-    }
-
-    /**
-     * Additional joins that are needed.
-     *
-     * @param array $joins
-     * @return self
-     */
-    public function add_joins(array $joins): self {
-        foreach ($joins as $join) {
-            $this->add_join($join);
-        }
-        return $this;
-    }
-
-    /**
-     * Return joins
-     *
-     * @return string[]
-     */
-    private function get_joins(): array {
-        return array_values($this->joins);
-    }
-
     /**
      * Get table alias for given custom field
      *

reportbuilder/classes/local/helpers/join_trait.php

@@ -0,0 +1,65 @@
+<?php
+// This file is part of Moodle - http://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 <http://www.gnu.org/licenses/>.
+
+declare(strict_types=1);
+
+namespace core_reportbuilder\local\helpers;
+
+/**
+ * Trait for classes that expect to store SQL table joins
+ *
+ * @package     core_reportbuilder
+ * @copyright   2024 Paul Holden <paulh@moodle.com>
+ * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+trait join_trait {
+
+    /** @var string[] SQL table joins */
+    private array $joins = [];
+
+    /**
+     * Add single SQL table join
+     *
+     * @param string $join
+     * @return static
+     */
+    final public function add_join(string $join): static {
+        $this->joins[trim($join)] = trim($join);
+        return $this;
+    }
+
+    /**
+     * Add multiple SQL table joins
+     *
+     * @param string[] $joins
+     * @return static
+     */
+    final public function add_joins(array $joins): static {
+        foreach ($joins as $join) {
+            $this->add_join($join);
+        }
+        return $this;
+    }
+
+    /**
+     * Return SQL table joins
+     *
+     * @return string[]
+     */
+    final public function get_joins(): array {
+        return array_values($this->joins);
+    }
+}

reportbuilder/classes/local/helpers/user_profile_fields.php

@@ -44,72 +44,28 @@ require_once($CFG->dirroot.'/user/profile/lib.php');
  */
 class user_profile_fields {
 
-    /** @var array user profile fields */
+    use join_trait;
-    private $userprofilefields;
 
-    /** @var string $entityname Name of the entity */
+    /** @var profile_field_base[] User profile fields */
-    private $entityname;
+    private array $userprofilefields;
-
-    /** @var int $usertablefieldalias The user table/field alias */
-    private $usertablefieldalias;
-
-    /** @var array additional joins */
-    private $joins = [];
 
     /**
-     * Class userprofilefields constructor.
+     * Constructor
      *
-     * @param string $usertablefieldalias The user table/field alias used when adding columns and filters.
+     * @param string $usertablefieldalias The table/field alias to match the user ID when adding columns and filters.
      * @param string $entityname The entity name used when adding columns and filters.
      */
-    public function __construct(string $usertablefieldalias, string $entityname) {
+    public function __construct(
-        $this->usertablefieldalias = $usertablefieldalias;
+        /** @var string The table/field alias to match the user ID when adding columns and filters */
-        $this->entityname = $entityname;
+        private readonly string $usertablefieldalias,
-        $this->userprofilefields = $this->get_user_profile_fields();
+        /** @var string The entity name used when adding columns and filters */
-    }
+        private readonly string $entityname,
-
+    ) {
-    /**
+        // Retrieve the list of available/visible user profile fields.
-     * Retrieves the list of available/visible user profile fields
+        $this->userprofilefields = array_filter(
-     *
+            profile_get_user_fields_with_data(0),
-     * @return profile_field_base[]
+            fn(profile_field_base $field) => $field->is_visible(),
-     */
+        );
-    private function get_user_profile_fields(): array {
-        return array_filter(profile_get_user_fields_with_data(0), static function(profile_field_base $profilefield): bool {
-            return $profilefield->is_visible();
-        });
-    }
-
-    /**
-     * Additional join that is needed.
-     *
-     * @param string $join
-     * @return self
-     */
-    public function add_join(string $join): self {
-        $this->joins[trim($join)] = trim($join);
-        return $this;
-    }
-
-    /**
-     * Additional joins that are needed.
-     *
-     * @param array $joins
-     * @return self
-     */
-    public function add_joins(array $joins): self {
-        foreach ($joins as $join) {
-            $this->add_join($join);
-        }
-        return $this;
-    }
-
-    /**
-     * Return joins
-     *
-     * @return string[]
-     */
-    private function get_joins(): array {
-        return array_values($this->joins);
     }
 
     /**

reportbuilder/classes/local/report/column.php

@@ -20,8 +20,7 @@ namespace core_reportbuilder\local\report;
 
 use coding_exception;
 use lang_string;
-use core_reportbuilder\local\helpers\aggregation;
+use core_reportbuilder\local\helpers\{aggregation, database, join_trait};
-use core_reportbuilder\local\helpers\database;
 use core_reportbuilder\local\aggregation\base;
 use core_reportbuilder\local\models\column as column_model;
 
@@ -34,6 +33,8 @@ use core_reportbuilder\local\models\column as column_model;
  */
 final class column {
 
+    use join_trait;
+
     /** @var int Column type is integer */
     public const TYPE_INTEGER = 1;
 
@@ -55,24 +56,12 @@ final class column {
     /** @var int $index Column index within a report */
     private $index;
 
-    /** @var string $columnname Internal reference to name of column */
-    private $columnname;
-
-    /** @var lang_string $columntitle Used as a title for the column in reports */
-    private $columntitle;
-
     /** @var bool $hascustomcolumntitle Used to store if the column has been given a custom title */
     private $hascustomcolumntitle = false;
 
-    /** @var string $entityname Name of the entity this column belongs to */
-    private $entityname;
-
     /** @var int $type Column data type (one of the TYPE_* class constants) */
     private $type = self::TYPE_TEXT;
 
-    /** @var string[] $joins List of SQL joins for this column */
-    private $joins = [];
-
     /** @var array $fields */
     private $fields = [];
 
@@ -101,16 +90,16 @@ final class column {
     private $attributes = [];
 
     /** @var bool $available Used to know if column is available to the current user or not */
-    protected $available = true;
+    private $available = true;
 
     /** @var bool $deprecated */
-    protected $deprecated = false;
+    private $deprecated = false;
 
     /** @var string $deprecatedmessage */
-    protected $deprecatedmessage;
+    private $deprecatedmessage;
 
     /** @var column_model $persistent */
-    protected $persistent;
+    private $persistent;
 
     /**
      * Column constructor
@@ -129,10 +118,15 @@ final class column {
      *      this value should be the result of calling {@see get_entity_name}, however if creating columns inside reports directly
      *      it should be the name of the entity as passed to {@see \core_reportbuilder\local\report\base::annotate_entity}
      */
-    public function __construct(string $name, ?lang_string $title, string $entityname) {
+    public function __construct(
-        $this->columnname = $name;
+        /** @var string Internal name of the column */
-        $this->columntitle = $title;
+        private string $name,
-        $this->entityname = $entityname;
+        /** @var lang_string|null Title of the column used in reports */
+        private ?lang_string $title,
+        /** @var string Name of the entity this column belongs to */
+        private readonly string $entityname,
+    ) {
+
     }
 
     /**
@@ -142,7 +136,7 @@ final class column {
      * @return self
      */
     public function set_name(string $name): self {
-        $this->columnname = $name;
+        $this->name = $name;
         return $this;
     }
 
@@ -152,7 +146,7 @@ final class column {
      * @return mixed
      */
     public function get_name(): string {
-        return $this->columnname;
+        return $this->name;
     }
 
     /**
@@ -162,7 +156,7 @@ final class column {
      * @return self
      */
     public function set_title(?lang_string $title): self {
-        $this->columntitle = $title;
+        $this->title = $title;
         $this->hascustomcolumntitle = true;
         return $this;
     }
@@ -173,7 +167,7 @@ final class column {
      * @return string
      */
     public function get_title(): string {
-        return $this->columntitle ? (string) $this->columntitle : '';
+        return $this->title ? (string) $this->title : '';
     }
 
     /**
@@ -251,44 +245,6 @@ final class column {
         return $this->type;
     }
 
-    /**
-     * Add join clause required for this column to join to existing tables/entities
-     *
-     * This is necessary in the case where {@see add_field} is selecting data from a table that isn't otherwise queried
-     *
-     * @param string $join
-     * @return self
-     */
-    public function add_join(string $join): self {
-        $this->joins[trim($join)] = trim($join);
-        return $this;
-    }
-
-    /**
-     * Add multiple join clauses required for this column, passing each to {@see add_join}
-     *
-     * Typically when defining columns in entities, you should pass {@see \core_reportbuilder\local\report\base::get_joins} to
-     * this method, so that all entity joins are included in the report when your column is added to it
-     *
-     * @param string[] $joins
-     * @return self
-     */
-    public function add_joins(array $joins): self {
-        foreach ($joins as $join) {
-            $this->add_join($join);
-        }
-        return $this;
-    }
-
-    /**
-     * Return column joins
-     *
-     * @return string[]
-     */
-    public function get_joins(): array {
-        return array_values($this->joins);
-    }
-
     /**
      * Adds a field to be queried from the database that is necessary for this column
      *

reportbuilder/classes/local/report/filter.php

@@ -21,7 +21,7 @@ namespace core_reportbuilder\local\report;
 use lang_string;
 use moodle_exception;
 use core_reportbuilder\local\filters\base;
-use core_reportbuilder\local\helpers\database;
+use core_reportbuilder\local\helpers\{database, join_trait};
 use core_reportbuilder\local\models\filter as filter_model;
 
 /**
@@ -33,17 +33,7 @@ use core_reportbuilder\local\models\filter as filter_model;
  */
 final class filter {
 
-    /** @var string $filterclass */
+    use join_trait;
-    private $filterclass;
-
-    /** @var string $name */
-    private $name;
-
-    /** @var lang_string $header */
-    private $header;
-
-    /** @var string $entity */
-    private $entityname;
 
     /** @var string $fieldsql */
     private $fieldsql = '';
@@ -51,26 +41,23 @@ final class filter {
     /** @var array $fieldparams */
     private $fieldparams = [];
 
-    /** @var string[] $joins */
-    protected $joins = [];
-
     /** @var bool $available */
-    protected $available = true;
+    private $available = true;
 
     /** @var bool $deprecated */
-    protected $deprecated = false;
+    private $deprecated = false;
 
     /** @var string $deprecatedmessage */
-    protected $deprecatedmessage;
+    private $deprecatedmessage;
 
     /** @var mixed $options */
-    protected $options;
+    private $options;
 
     /** @var array $limitoperators */
-    protected $limitoperators = [];
+    private $limitoperators = [];
 
     /** @var filter_model $persistent */
-    protected $persistent;
+    private $persistent;
 
     /**
      * Filter constructor
@@ -86,22 +73,21 @@ final class filter {
      * @throws moodle_exception For invalid filter class
      */
     public function __construct(
-        string $filterclass,
+        /** @var string Filter type class to use, must extend {@see base} filter class */
-        string $name,
+        private readonly string $filterclass,
-        lang_string $header,
+        /** @var string Internal name of the filter */
-        string $entityname,
+        private readonly string $name,
+        /** @var lang_string Title of the filter used in reports */
+        private lang_string $header,
+        /** @var string Name of the entity this filter belongs to */
+        private readonly string $entityname,
         string $fieldsql = '',
-        array $fieldparams = []
+        array $fieldparams = [],
     ) {
         if (!class_exists($filterclass) || !is_subclass_of($filterclass, base::class)) {
             throw new moodle_exception('filterinvalid', 'reportbuilder', '', null, $filterclass);
         }
 
-        $this->filterclass = $filterclass;
-        $this->name = $name;
-        $this->header = $header;
-        $this->entityname = $entityname;
-
         if ($fieldsql !== '') {
             $this->set_field_sql($fieldsql, $fieldparams);
         }
@@ -163,44 +149,6 @@ final class filter {
         return $this->get_entity_name() . ':' . $this->get_name();
     }
 
-    /**
-     * Return joins
-     *
-     * @return string[]
-     */
-    public function get_joins(): array {
-        return array_values($this->joins);
-    }
-
-    /**
-     * Add join clause required for this filter to join to existing tables/entities
-     *
-     * This is necessary in the case where {@see set_field_sql} is selecting data from a table that isn't otherwise queried
-     *
-     * @param string $join
-     * @return self
-     */
-    public function add_join(string $join): self {
-        $this->joins[trim($join)] = trim($join);
-        return $this;
-    }
-
-    /**
-     * Add multiple join clauses required for this filter, passing each to {@see add_join}
-     *
-     * Typically when defining filters in entities, you should pass {@see \core_reportbuilder\local\report\base::get_joins} to
-     * this method, so that all entity joins are included in the report when your filter is used in it
-     *
-     * @param string[] $joins
-     * @return self
-     */
-    public function add_joins(array $joins): self {
-        foreach ($joins as $join) {
-            $this->add_join($join);
-        }
-        return $this;
-    }
-
     /**
      * Get SQL expression for the field
      *

reportbuilder/tests/local/entities/base_test.php

@@ -43,7 +43,7 @@ defined('MOODLE_INTERNAL') || die();
  * @copyright   2021 David Matamoros <davidmc@moodle.com>
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class base_test extends advanced_testcase {
+final class base_test extends advanced_testcase {
 
     /**
      * Test entity table alias
@@ -192,50 +192,6 @@ class base_test extends advanced_testcase {
         $this->assertEquals($newtitle, $entity->get_entity_title());
     }
 
-    /**
-     * Test adding single join
-     */
-    public function test_add_join(): void {
-        $entity = new base_test_entity();
-
-        $tablejoin = "JOIN {course} c2 ON c2.id = c1.id";
-        $entity->add_join($tablejoin);
-
-        $this->assertEquals([$tablejoin], $entity->get_joins());
-    }
-
-    /**
-     * Test adding multiple joins
-     */
-    public function test_add_joins(): void {
-        $entity = new base_test_entity();
-
-        $tablejoins = [
-            "JOIN {course} c2 ON c2.id = c1.id",
-            "JOIN {course} c3 ON c3.id = c1.id",
-        ];
-        $entity->add_joins($tablejoins);
-
-        $this->assertEquals($tablejoins, $entity->get_joins());
-    }
-
-    /**
-     * Test adding duplicate joins
-     */
-    public function test_add_duplicate_joins(): void {
-        $entity = new base_test_entity();
-
-        $tablejoins = [
-            "JOIN {course} c2 ON c2.id = c1.id",
-            "JOIN {course} c3 ON c3.id = c1.id",
-        ];
-        $entity
-            ->add_joins($tablejoins)
-            ->add_joins($tablejoins);
-
-        $this->assertEquals($tablejoins, $entity->get_joins());
-    }
-
     /**
      * Test getting column
      */

reportbuilder/tests/local/helpers/custom_fields_test.php

@@ -43,7 +43,7 @@ require_once("{$CFG->dirroot}/reportbuilder/tests/helpers.php");
  * @copyright   2021 David Matamoros <davidmc@moodle.com>
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class custom_fields_test extends core_reportbuilder_testcase {
+final class custom_fields_test extends core_reportbuilder_testcase {
 
     /**
      * Generate custom fields, one of each type
@@ -117,49 +117,34 @@ class custom_fields_test extends core_reportbuilder_testcase {
     }
 
     /**
-     * Test for add_join
+     * Test that joins added to the custom fields helper are present in its columns/filters
      */
     public function test_add_join(): void {
         $this->resetAfterTest();
 
         $customfields = $this->generate_customfields();
 
-        // By default, we always join on the customfield data table.
+        // We always join on the customfield data table.
-        $columns = $customfields->get_columns();
+        $columnjoins = $customfields->get_columns()[0]->get_joins();
-        $joins = $columns[0]->get_joins();
+        $this->assertCount(1, $columnjoins);
+        $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $columnjoins[0]);
 
-        $this->assertCount(1, $joins);
+        $filterjoins = $customfields->get_filters()[0]->get_joins();
-        $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $joins[0]);
+        $this->assertCount(1, $filterjoins);
+        $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $filterjoins[0]);
 
         // Add additional join.
         $customfields->add_join('JOIN {test} t ON t.id = id');
 
-        $columns = $customfields->get_columns();
+        $columnjoins = $customfields->get_columns()[0]->get_joins();
-        $joins = $columns[0]->get_joins();
+        $this->assertCount(2, $columnjoins);
+        $this->assertEquals('JOIN {test} t ON t.id = id', $columnjoins[0]);
+        $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $columnjoins[1]);
 
-        $this->assertCount(2, $joins);
+        $filterjoins = $customfields->get_filters()[0]->get_joins();
-        $this->assertEquals('JOIN {test} t ON t.id = id', $joins[0]);
+        $this->assertCount(2, $filterjoins);
-        $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $joins[1]);
+        $this->assertEquals('JOIN {test} t ON t.id = id', $filterjoins[0]);
-    }
+        $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $filterjoins[1]);
-
-    /**
-     * Test for add_joins
-     */
-    public function test_add_joins(): void {
-        $this->resetAfterTest();
-
-        $customfields = $this->generate_customfields();
-
-        // Add additional joins.
-        $customfields->add_joins(['JOIN {test} t ON t.id = id', 'JOIN {test2} t2 ON t2.id = id']);
-
-        $columns = $customfields->get_columns();
-        $joins = $columns[0]->get_joins();
-
-        $this->assertCount(3, $joins);
-        $this->assertEquals('JOIN {test} t ON t.id = id', $joins[0]);
-        $this->assertEquals('JOIN {test2} t2 ON t2.id = id', $joins[1]);
-        $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $joins[2]);
     }
 
     /**
@@ -254,7 +239,7 @@ class custom_fields_test extends core_reportbuilder_testcase {
      *
      * @return array[]
      */
-    public function custom_report_filter_provider(): array {
+    public static function custom_report_filter_provider(): array {
         return [
             'Filter by text custom field' => ['course:customfield_text', [
                 'course:customfield_text_operator' => text::IS_EQUAL_TO,

reportbuilder/tests/local/helpers/join_trait_test.php

@@ -0,0 +1,86 @@
+<?php
+// This file is part of Moodle - http://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 <http://www.gnu.org/licenses/>.
+
+declare(strict_types=1);
+
+namespace core_reportbuilder\local\helpers;
+
+use advanced_testcase;
+
+/**
+ * Unit tests for the join trait
+ *
+ * @package     core_reportbuilder
+ * @covers      \core_reportbuilder\local\helpers\join_trait
+ * @copyright   2024 Paul Holden <paulh@moodle.com>
+ * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+final class join_trait_test extends advanced_testcase {
+
+    /**
+     * Test adding single join
+     */
+    public function test_add_join(): void {
+
+        /** @var join_trait $trait */
+        $trait = $this->getObjectForTrait(join_trait::class);
+        $trait->add_join('JOIN {test} t ON t.id = a.id');
+
+        $this->assertEquals(['JOIN {test} t ON t.id = a.id'], $trait->get_joins());
+    }
+
+    /**
+     * Test adding single join multiple times
+     */
+    public function test_add_join_multiple(): void {
+
+        /** @var join_trait $trait */
+        $trait = $this->getObjectForTrait(join_trait::class);
+
+        // Add multiple joins, two of which are duplicates.
+        $trait->add_join('JOIN {test} t1 ON t1.id = a.id')
+            ->add_join('JOIN {test} t2 ON t2.id = b.id')
+            ->add_join('JOIN {test} t1 ON t1.id = a.id');
+
+        // The duplicated join is normalised away.
+        $this->assertEquals([
+            'JOIN {test} t1 ON t1.id = a.id',
+            'JOIN {test} t2 ON t2.id = b.id',
+        ], $trait->get_joins());
+    }
+
+    /**
+     * Test adding multiple joins
+     */
+    public function test_add_joins(): void {
+
+        /** @var join_trait $trait */
+        $trait = $this->getObjectForTrait(join_trait::class);
+
+        // Add multiple joins, two of which are duplicates.
+        $trait->add_joins([
+            'JOIN {test} t1 ON t1.id = a.id',
+            'JOIN {test} t2 ON t2.id = b.id',
+            'JOIN {test} t1 ON t1.id = a.id',
+        ]);
+
+        // The duplicated join is normalised away.
+        $this->assertEquals([
+            'JOIN {test} t1 ON t1.id = a.id',
+            'JOIN {test} t2 ON t2.id = b.id',
+        ], $trait->get_joins());
+    }
+}

reportbuilder/tests/local/helpers/user_profile_fields_test.php

@@ -42,7 +42,7 @@ require_once("{$CFG->dirroot}/reportbuilder/tests/helpers.php");
  * @copyright   2021 David Matamoros <davidmc@moodle.com>
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class user_profile_fields_test extends core_reportbuilder_testcase {
+final class user_profile_fields_test extends core_reportbuilder_testcase {
 
     /**
      * Generate custom profile fields, one of each type
@@ -132,33 +132,34 @@ class user_profile_fields_test extends core_reportbuilder_testcase {
     }
 
     /**
-     * Test for add_join
+     * Test that joins added to the profile fields helper are present in its columns/filters
      */
     public function test_add_join(): void {
         $this->resetAfterTest();
 
         $userprofilefields = $this->generate_userprofilefields();
-        $columns = $userprofilefields->get_columns();
-        $this->assertCount(1, ($columns[0])->get_joins());
 
+        // We always join on the user info data table.
+        $columnjoins = $userprofilefields->get_columns()[0]->get_joins();
+        $this->assertCount(1, $columnjoins);
+        $this->assertStringStartsWith('LEFT JOIN {user_info_data}', $columnjoins[0]);
+
+        $filterjoins = $userprofilefields->get_filters()[0]->get_joins();
+        $this->assertCount(1, $filterjoins);
+        $this->assertStringStartsWith('LEFT JOIN {user_info_data}', $filterjoins[0]);
+
+        // Add additional join.
         $userprofilefields->add_join('JOIN {test} t ON t.id = id');
-        $columns = $userprofilefields->get_columns();
-        $this->assertCount(2, ($columns[0])->get_joins());
-    }
 
-    /**
+        $columnjoins = $userprofilefields->get_columns()[0]->get_joins();
-     * Test for add_joins
+        $this->assertCount(2, $columnjoins);
-     */
+        $this->assertEquals('JOIN {test} t ON t.id = id', $columnjoins[0]);
-    public function test_add_joins(): void {
+        $this->assertStringStartsWith('LEFT JOIN {user_info_data}', $columnjoins[1]);
-        $this->resetAfterTest();
 
-        $userprofilefields = $this->generate_userprofilefields();
+        $filterjoins = $userprofilefields->get_filters()[0]->get_joins();
-        $columns = $userprofilefields->get_columns();
+        $this->assertCount(2, $filterjoins);
-        $this->assertCount(1, ($columns[0])->get_joins());
+        $this->assertEquals('JOIN {test} t ON t.id = id', $filterjoins[0]);
-
+        $this->assertStringStartsWith('LEFT JOIN {user_info_data}', $filterjoins[1]);
-        $userprofilefields->add_joins(['JOIN {test} t ON t.id = id', 'JOIN {test2} t2 ON t2.id = id']);
-        $columns = $userprofilefields->get_columns();
-        $this->assertCount(3, ($columns[0])->get_joins());
     }
 
     /**
@@ -262,7 +263,7 @@ class user_profile_fields_test extends core_reportbuilder_testcase {
      *
      * @return array[]
      */
-    public function custom_report_filter_provider(): array {
+    public static function custom_report_filter_provider(): array {
         return [
             'Filter by checkbox profile field' => ['user:profilefield_checkbox', [
                 'user:profilefield_checkbox_operator' => boolean_select::CHECKED,

reportbuilder/tests/local/report/column_test.php

@@ -32,7 +32,7 @@ use core_reportbuilder\local\helpers\database;
  * @copyright   2020 Paul Holden <paulh@moodle.com>
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class column_test extends advanced_testcase {
+final class column_test extends advanced_testcase {
 
     /**
      * Test column name getter/setter
@@ -112,38 +112,12 @@ class column_test extends advanced_testcase {
         $column->set_type(-1);
     }
 
-    /**
-     * Test adding single join
-     */
-    public function test_add_join(): void {
-        $column = $this->create_column('test');
-        $this->assertEquals([], $column->get_joins());
-
-        $column->add_join('JOIN {user} u ON u.id = table.userid');
-        $this->assertEquals(['JOIN {user} u ON u.id = table.userid'], $column->get_joins());
-    }
-
-    /**
-     * Test adding multiple joins
-     */
-    public function test_add_joins(): void {
-        $tablejoins = [
-            "JOIN {course} c2 ON c2.id = c1.id",
-            "JOIN {course} c3 ON c3.id = c1.id",
-        ];
-
-        $column = $this->create_column('test')
-            ->add_joins($tablejoins);
-
-        $this->assertEquals($tablejoins, $column->get_joins());
-    }
-
     /**
      * Data provider for {@see test_add_field}
      *
      * @return array
      */
-    public function add_field_provider(): array {
+    public static function add_field_provider(): array {
         return [
             ['foo', '', ['foo AS c1_foo']],
             ['foo', 'bar', ['foo AS c1_bar']],
@@ -231,7 +205,7 @@ class column_test extends advanced_testcase {
      *
      * @return array
      */
-    public function add_fields_provider(): array {
+    public static function add_fields_provider(): array {
         return [
             ['t.foo', ['t.foo AS c1_foo']],
             ['t.foo bar', ['t.foo AS c1_bar']],
@@ -327,7 +301,7 @@ class column_test extends advanced_testcase {
      *
      * @return array[]
      */
-    public function column_type_provider(): array {
+    public static function column_type_provider(): array {
         return [
             [column::TYPE_INTEGER, 42],
             [column::TYPE_TEXT, 'Hello'],

reportbuilder/tests/local/report/filter_test.php

@@ -31,7 +31,7 @@ use core_reportbuilder\local\filters\text;
  * @copyright   2021 Paul Holden <paulh@moodle.com>
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class filter_test extends advanced_testcase {
+final class filter_test extends advanced_testcase {
 
     /**
      * Test getting filter class
@@ -120,32 +120,6 @@ class filter_test extends advanced_testcase {
         $this->assertEquals(['username_1' => 'test', 'idnumber_1' => 'bar'], $params);
     }
 
-    /**
-     * Test adding single join
-     */
-    public function test_add_join(): void {
-        $filter = $this->create_filter('username', 'u.username');
-        $this->assertEquals([], $filter->get_joins());
-
-        $filter->add_join('JOIN {user} u ON u.id = table.userid');
-        $this->assertEquals(['JOIN {user} u ON u.id = table.userid'], $filter->get_joins());
-    }
-
-    /**
-     * Test adding multiple joins
-     */
-    public function test_add_joins(): void {
-        $tablejoins = [
-            "JOIN {course} c2 ON c2.id = c1.id",
-            "JOIN {course} c3 ON c3.id = c1.id",
-        ];
-
-        $filter = $this->create_filter('username', 'u.username')
-            ->add_joins($tablejoins);
-
-        $this->assertEquals($tablejoins, $filter->get_joins());
-    }
-
     /**
      * Test is available
      */