#382 - process grade grades properly

CHANGES.md

@@ -1,5 +1,12 @@
 # Release notes
 
+## 2025101400
+1. 2025-10-14: fix: #382: ensure grade_grades table is merged properly. Thanks Daniel Tomé.
+   1. Added tests for the new grade_grades table merger.
+   2. Improved some existing tests.
+   3. Improve Makefile to let run phpcs/phpcbf more easily.
+
+
 ## 2025092100
 
 1. 2025-09-21: improvement: #372: add output from last steps of regrading and reaggregation of course completions.
@@ -48,11 +55,11 @@
 When upgrading to this version, you have to choose one of these paths:
 
 1. **In case you have local plugin customizations:** you must check twice
-   the new plugin structure since there has been a major refactor 
-   of the whole plugin. The `lib/` directory was removed, and 
+   the new plugin structure since there has been a major refactor
+   of the whole plugin. The `lib/` directory was removed, and
    most of the plugin classes were moved inside the `classes/` directory
    for a better code organization and with the benefit of autoloading.
-   Also, we removed the vast majority of functions from the `lib.php`, 
+   Also, we removed the vast majority of functions from the `lib.php`,
    leaving there only the necessary Moodle callbacks.
    Update your local customizations appropriately according to new
    classes and file structure.
@@ -74,9 +81,9 @@ When upgrading to this version, you have to choose one of these paths:
       and table mergers processed the merge with success till now.
    2. The callbacks for this hook are meant to process any kind of operations
       from Moodle internals or plugin specific tasks, that are transversal,
-      (operations not specific for a single table) or any kind of 
+      (operations not specific for a single table) or any kind of
       aggregation operation, not updated by the table mergers.
-   3. To provide you an example, we have moved the regrading of the users and 
+   3. To provide you an example, we have moved the regrading of the users and
       the course recompletions into callbacks for this hook.
    4. We think this hook will help Moodle and plugin developers to adjust the
       merge users tool to better fit any Moodle instance (with a variable
@@ -90,7 +97,7 @@ Moodle core (subsystems, and so) and plugins really know how user's
 information is managed. So, their maintainers have the full knowledge
 and they can provide callbacks for both hooks:
 1. Callbacks for `add_settings_before_merging` hook may help providing specific
- database-related settings: mainly table mergers (setting `tablemergers`), 
+ database-related settings: mainly table mergers (setting `tablemergers`),
  compound indexes (setting `compoundindexes`) or user-related table columns
  (setting `userfieldnames`).
 2. Callbacks for `after_merged_all_tables` hook may help providing specific
@@ -100,7 +107,7 @@ and they can provide callbacks for both hooks:
 **Just as a clarification:** The inclusion of the hooks does not alter the
 way of using this plugin at all. It will behave as it did.
 
-However, these two hooks provide you as a developer and maintainer of your 
+However, these two hooks provide you as a developer and maintainer of your
 plugin or Moodle instance powerful tools to customize the behaviour of the merge,
 just placing the necessary callbacks and related stuff in your own
 code, to ensure merge users is processed properly.
@@ -110,18 +117,18 @@ code, to ensure merge users is processed properly.
 1. 2025-08-18: improvement: #348: added hook to load database-related settings.
    1. This is though to help Moodle and plugin developers to adjust their code to help
       this plugin merge users properly.
-   2. The settings that are loaded by this hook are those populated on the old 
+   2. The settings that are loaded by this hook are those populated on the old
       `config/config.php` and `config/config.local.php` files. These files are not supported any more.
    3. The content of the old `config/config.php` is now placed on `classes/local/default_db_config.php`.
       This must help this plugin maintainers to keep in a single place the default behaviour.
    4. Added tests to ensure the database-related settings are kept properly.
    5. Priority of the settings (more priority settings are kept, in front of subsequent settings):
-      1. Custom admin setting: the set of settings with the highest priority. 
+      1. Custom admin setting: the set of settings with the highest priority.
          This must let administrators adjusting plugin behaviour at any time.
       2. Hook settings: settings populated from this hook's callbacks are the second set of
          settings in priority.
       3. Default settings: the plugin's default settings are kept as with the lowest priority.
-         Any existing setting from hooks and custom settings will replace default ones. 
+         Any existing setting from hooks and custom settings will replace default ones.
 
 ## 2025081700
 
@@ -134,7 +141,7 @@ code, to ensure merge users is processed properly.
 
 ## 2025081603
 
-1. 2025-0816: improvement: #244: allow resetting web user selection. Unified search and review tables. 
+1. 2025-0816: improvement: #244: allow resetting web user selection. Unified search and review tables.
  Added extra column on search table to show whether a user is already suspended (probably already merged).
 2. 2025-08-16: improvement: #345: move config.local.php into a new admin setting, in JSON format, for being human-readable.
    1. Also, consider that setting with name `alwaysRollback` was renamed to `alwaysrollback` to unify the case insensitiveness
@@ -143,13 +150,13 @@ code, to ensure merge users is processed properly.
 ### UPGRADING
 
 **Recommendation when upgrading:** Keep your `config/config.local.php` in place. It will help
-updating the value of the new admin setting  `tool_mergeusers/customdbsettings` **automatically**, 
-without the need to convert your old `config/config.local.php` to JSON. 
+updating the value of the new admin setting  `tool_mergeusers/customdbsettings` **automatically**,
+without the need to convert your old `config/config.local.php` to JSON.
 But, it is only a recommendation.
 
-Otherwise, you will have to update that admin setting manually with the content of your previous 
-`config/config.local.php` on the new admin setting. To help you, you can use the 
-`tool_mergeusers\local\jsonizer::to_json($customsettings)` with an array with all your 
+Otherwise, you will have to update that admin setting manually with the content of your previous
+`config/config.local.php` on the new admin setting. To help you, you can use the
+`tool_mergeusers\local\jsonizer::to_json($customsettings)` with an array with all your
 `$customsettings`, and it will provide you the JSON content to place.
 
 If you did not have any customization or file on `config/config.local.php`, you have to do nothing
@@ -208,7 +215,7 @@ this new API.
 
 ## 2025020502
 
-1. 2025-02-05 #295 - fix: remove deprecation warning on CLIGathering, related to Iterator. Thanks to @CatSema. 
+1. 2025-02-05 #295 - fix: remove deprecation warning on CLIGathering, related to Iterator. Thanks to @CatSema.
 
 ## 2025020501
 
@@ -234,10 +241,10 @@ this new API.
 2. 2025-01-22 #292 - Bump plugin version and requires Moodle 4.5 onwards.
    1. CI passes on green, as before, for PHP 8.1, 8.2 and 8.3 only for core MOODLE_405_STABLE.
    2. version.php updated.
-   3. Improve type detection on IDE. 
+   3. Improve type detection on IDE.
    4. Uses new trait on the assign_test class.
 3. 2025-01-17 #291 - Make web administration work on merge users.
-   1. Removed lines requiring "lib/outputcomponents.php" from two files. 
+   1. Removed lines requiring "lib/outputcomponents.php" from two files.
 
 ## 2024060300
 

Makefile

@@ -23,10 +23,10 @@ ifndef docker-with-xdebug:
 docker-with-xdebug := docker exec -e XDEBUG_SESSION=1 -it $(container_name)
 endif
 ifndef phpcs:
-phpcs := $(docker) local/codechecker/vendor/bin/phpcs
+phpcs := $(docker) php local/codechecker/vendor/bin/phpcs
 endif
 ifndef phpcbf:
-phpcbf := $(docker) local/codechecker/vendor/bin/phpcbf
+phpcbf := $(docker) php local/codechecker/vendor/bin/phpcbf
 endif
 
 .PHONY: start
@@ -40,11 +40,11 @@ stop:
 .PHONY: pass-tests
 pass-tests: options =
 pass-tests:
-	$(docker) vendor/bin/phpunit -c admin/tool/mergeusers --testdox $(options)
+	$(docker) php vendor/bin/phpunit -c admin/tool/mergeusers --testdox $(options)
 
 .PHONY: pass-tests-with-xdebug
 pass-tests-with-xdebug:
-	$(docker-with-xdebug) vendor/bin/phpunit -c admin/tool/mergeusers --testdox $(options)
+	$(docker-with-xdebug) php vendor/bin/phpunit -c admin/tool/mergeusers --testdox $(options)
 
 .PHONY: build-phpunit-xml
 build-phpunit-xml:

classes/local/default_db_config.php

@@ -29,6 +29,7 @@
 use tool_mergeusers\local\merger\assign_submission_table_merger;
 use tool_mergeusers\local\merger\generic_table_merger;
 use tool_mergeusers\local\merger\quiz_attempts_table_merger;
+use tool_mergeusers\local\merger\grade_grades_table_merger;
 
 /**
  * Default database-related configuration.
@@ -197,6 +198,7 @@ class default_db_config {
             'default' => generic_table_merger::class,
             'quiz_attempts' => quiz_attempts_table_merger::class,
             'assign_submission' => assign_submission_table_merger::class,
+            'grade_grades' => grade_grades_table_merger::class,
         ],
 
         'alwaysrollback' => false,

classes/local/merger/finder/in_memory_assign_submission_finder.php

@@ -33,8 +33,7 @@
  * @copyright 2018 onwards Universitat Rovira i Virgili (https://www.urv.cat)
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class in_memory_assign_submission_finder implements assign_submission_finder
-{
+class in_memory_assign_submission_finder implements assign_submission_finder {
     /** @var array list of assign submissions for a given user. */
     private array $memory;
 

classes/local/merger/generic_table_merger.php

@@ -137,10 +137,8 @@ protected function merge_compound_index(
         global $DB;
 
         $otherfieldsstr = implode(', ', $otherfields);
-        $sql = 'SELECT id, ' . $userfield . ', ' . $otherfieldsstr .
-            ' FROM {' . $data['tableName'] . '} ' .
-            ' WHERE ' . $userfield . ' IN ( ?, ?)';
-        $result = $DB->get_records_sql($sql, [$data['fromid'], $data['toid']]);
+        [$sql, $params] = $this->build_sql_query($data, $userfield, $otherfieldsstr);
+        $result = $DB->get_records_sql($sql, $params);
 
         $itemarr = [];
         $idstoremove = [];
@@ -152,23 +150,10 @@ protected function merge_compound_index(
             $keyfromotherstr = implode('-', $keyfromother);
             $itemarr[$keyfromotherstr][$resobj->$userfield] = $id;
         }
-        unset($result);
 
-        foreach ($itemarr as $otherinfo) {
-            // If and only if we have only one result, and it is from the current user => update record.
-            if (count($otherinfo) == 1) {
-                if (isset($otherinfo[$data['fromid']])) {
-                    $recordstomodify[$otherinfo[$data['fromid']]] = $otherinfo[$data['fromid']];
-                }
-            } else {
-                // Both users appear in the group.
-                // Confirm both records exist, preventing problems from inconsistent data in database.
-                if (isset($otherinfo[$data['toid']]) && isset($otherinfo[$data['fromid']])) {
-                    $useridtoclean = $this->get_user_id_to_delete_on_conflicts($data['toid'], $data['fromid']);
-                    $idstoremove[$otherinfo[$useridtoclean]] = $otherinfo[$useridtoclean];
-                }
-            }
-        }
+        $this->find_ids_to_update_and_remove($itemarr, $result, $data, $recordstomodify, $idstoremove);
+
+        unset($result);
         unset($itemarr);
         // We know that idstoremove have always to be removed and NOT to be updated.
         foreach ($idstoremove as $id) {
@@ -183,6 +168,95 @@ protected function merge_compound_index(
         unset($sql);
     }
 
+    /**
+     * Generates an SQL query that selects records from a table based on the user field ID.
+     *
+     * This method can be overriden by subclasses to adapt the SQL query to their needs.
+     *
+     * @param array $data Array containing the table name, `fromid`, and `toid` for the merging operation.
+     * @param string $userfield The field name in the table that refers to the user ID.
+     * @param string $otherfieldsstr A string representing the other fields in the compound index, separated by commas.
+     * @return array Array with the form [$sql, $params] to be used.
+     */
+    protected function build_sql_query(array $data, string $userfield, string $otherfieldsstr): array {
+        $sql = 'SELECT id, ' . $userfield . ', ' . $otherfieldsstr .
+            ' FROM {' . $data['tableName'] . '} ' .
+            ' WHERE ' . $userfield . ' IN ( ?, ?)';
+        return [$sql, [$data['fromid'], $data['toid']]];
+    }
+
+    /**
+     * Finds the records to update and remove from a compound index.
+     *
+     * This method can be overridden by subclasses to adapt the logic to their needs.
+     *
+     * At current implementation, $result is not used, but it is provided for subclasses
+     * just in case they have extra detail returned from self::build_sql_query() implementation.
+     *
+     * @param array $itemarr The grouped records from the compound index for a specific combination of other fields.
+     * @param array $result The records retrieved from the database.
+     * @param array $data The merging operation details, including `fromid`, `toid`, and table name.
+     * @param array &$recordstomodify Array to store the IDs of records that need to be updated.
+     * @param array &$idstoremove Array to store the IDs of records that need to be removed.
+     * @return void
+     */
+    protected function find_ids_to_update_and_remove(
+        array $itemarr,
+        array $result,
+        array $data,
+        array &$recordstomodify,
+        array &$idstoremove,
+    ): void {
+        foreach ($itemarr as $otherinfo) {
+            // If and only if we have only one result, and it is from the current user => update record.
+            if (count($otherinfo) == 1) {
+                if (isset($otherinfo[$data['fromid']])) {
+                    $recordstomodify[$otherinfo[$data['fromid']]] = $otherinfo[$data['fromid']];
+                }
+            } else {
+                $this->process_duplicated_records_for_compound_index(
+                    $otherinfo,
+                    $result,
+                    $data,
+                    $recordstomodify,
+                    $idstoremove
+                );
+            }
+        }
+    }
+
+    /**
+     * Prevents database and PHP processing errors due to multiple records for the same compound index.
+     *
+     * To do so, it removes the records for one of the users, either the user to keep or the user to remove,
+     * depending on the configuration setting 'uniquekeynewidtomaintain'.
+     *
+     * In current implementation, there is only need to check whether to delete some records.
+     * However, the method signature provides also the possibility to update records,
+     * just in case a subclass needs to do so.
+     *
+     * @param array $conflictingrecords The grouped records from the compound index for a specific combination of other fields.
+     * @param array $result The records retrieved from the database.
+     * @param array $data The merging operation details, including `fromid`, `toid`, and table name.
+     * @param array &$recordstomodify Array to store the IDs of records that need to be updated.
+     * @param array &$idstoremove Array to store the IDs of records that need to be removed.
+     * @return void
+     */
+    protected function process_duplicated_records_for_compound_index(
+        array $conflictingrecords,
+        array $result,
+        array $data,
+        array &$recordstomodify,
+        array &$idstoremove,
+    ): void {
+        // Both users appear in the group.
+        // Confirm both records exist, preventing problems from inconsistent data in database.
+        if (isset($conflictingrecords[$data['toid']]) && isset($conflictingrecords[$data['fromid']])) {
+            $useridtoclean = $this->get_user_id_to_delete_on_conflicts($data['toid'], $data['fromid']);
+            $idstoremove[$conflictingrecords[$useridtoclean]] = $conflictingrecords[$useridtoclean];
+        }
+    }
+
     /**
      * Processes accordingly the cleaning up of records after a compound index is already processed.
      *
@@ -334,10 +408,22 @@ protected function update_records(
      * @param int $fromid user.id from user to remove.
      * @return int user.id from which records have to be deleted.
      */
-    private function get_user_id_to_delete_on_conflicts(int $toid, int $fromid): int {
+    protected function get_user_id_to_delete_on_conflicts(int $toid, int $fromid): int {
         return $this->newidtomaintain ? $fromid : $toid;
     }
 
+    /**
+     * Informs the user.id from which records have to be kept when conflicting
+     * records arises.
+     *
+     * @param int $toid user.id from user to keep.
+     * @param int $fromid user.id from user to remove.
+     * @return int user.id from which records have to be kept.
+     */
+    protected function get_user_id_to_keep_on_conflicts(int $toid, int $fromid): int {
+        return $this->newidtomaintain ? $toid : $fromid;
+    }
+
     /**
      * Gets the fields name on a compound index case, excluding the given $userField.
      * Therefore, if there are multiple user-related fields in a compound index,