Merge branch 'MDL-82068' of https://github.com/paulholden/moodle

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
      */