moodle-tool_mergeusers
moodle-tool_mergeusers copied to clipboard
execute merge users from web via adhoc task
Instead of making the admin to wait for the merging process to complete, we could define an adhoc task to process this request via cron.
This way, admin user takes the control just afterwards queueing the request, being executed asap by the Moodle cron.
We should list them on the logs as pending
. We could update this status by processing
when the adhoc tasks comes in action. And finally, show the link to the logs as usual.
We may consider to allow the admin to remove the request if it is in status pending
.
It would be nice to let the ad-hoc task retry the merge request N times before queueing this request to the scheduled task.
Think that failing adhoc tasks are retried by Moodle automatically.
So, we could add a guard clause to let to retry it until N times programatically.
Another interesting feature would be to send an email to the requester admin user when the merging operation ends (either with success or failing). If we implement N tretrials, the email should be send every time it is tried with ad-hoc task.
No email should be sent if the merging request is processed by the scheduled task.
Hi @jpahullo
Is there any plans to implement this nice enhancement? I believe it would resolve #206
Cheers, Mikhail
Hi @golenkovm,
Thanks for contributing with your comments.
There is no priority in our institution to do this, so right now, I cannot say any deadline soon.
However, if you contribute into this, it will be easier for us to get it integrated.
Thanks for all,
Jordi
We're interested in contributing such PR. How do you think such patch would be affected by 4.x changes?
Hi! It would be very nice!
I think thre is no big changes for ad-hoc tasks in Moodle 4, so its implementation would run ok for any supported versions of this plugin.
If we face any problem, it would be nicer to implement it for Moodle 4 first and then test if it still works in other versions like Moodle 3.11 and M3.9.
If you have any clue, just share it and discuss.
Thanks for your time!
NOTE: To document on the development of this issue:
As shown in https://github.com/moodle/moodle/blob/master/lib/classes/task/adhoc_task.php#L151, we can set up a concurrency limit for specific adhoc tasks. We should encourage setting the limit of concurrency of 1 to prevent possible unadverted effects of having 2 or more concurrent merging users.
In our institution, for instance, there are series of merging actions related to the same person, passing from username A to B, and then B to C. So, concurrent mergings from A to B and B to C would not be the same as a sequential A to B, and then B to C.
Also, possible concurrent effects on the same records between the concurrent merging could bring timeouts and errors of merging. Instead, sequencial mergings would not be causing unadverted errors: only errors due to the data model and current content of the Moodle database.
Hi @nvallinoto,
The scheduled task would iterate over the table of merge requests and add as many adhoc tasks as queued requests.
For any merge request that we create an adhoc task, we have to update the request's status. According to https://github.com/ndunand/moodle-tool_mergeusers/issues/218#issuecomment-1361260874 comment, we should change it to queued to be processed (i.e., the adhoc_task is created for it)
.
The adhoc task would be generated with ONLY the merge request id
as customdata. We do not have to set any user to the adhoc task, since it has to be run as cron user.
When adhoc task is run by the Moodle cron, it will take the merge request id
and load its record. We should also increment the current number of merge processings (i.e., 0 when the merge request is queued, so it will be 1 on the first attempt to merge both users by an adhoc task, 2 on the next attempt in case of failure, etc).
The first work of the adhoc task is to find users by its criteria:
- If 0 users match any of the remove or keep users, the merge request is updated its status as failed:
completed with errors. (i.e., even with all the current retries, the merging process could not end with success).
Adhoc task have to complete without exception. So Moodle will not repeat this adhoc task. - If > 1 users match any of the remove or keep users, the merge request is updated its status as failed:
completed with errors. (i.e., even with all the current retries, the merging process could not end with success).
. Adhoc task have to complete without exception. So Moodle will not repeat this adhoc task. - Otherwise, there is only a unique user.id that match the criteria for both remove and keep users. In this case, we can use the existing plugin API, passing both user.id.
- If merge concludes with errors, we have always to store/append any generated log into the current logs. We have to separate the logs per attempt, so they can be printed easily. Also, in case of error during the merge processing, the adhoc task have to terminate with an exception, so that Moodle queues again the adhoc tasks for a later processing. If there is no error,
- If the merge concludes with success, we store the logs. Then, we have to run the merge request AGAIN, in this same adhoc task. Why? User may be interacting with Moodle while merge request is being processed, so that NO ALL records are correctly migrated into the user to keep. This way, this repetition of the processing using the same plugin API, with the user to remove already deactivated, will ensure ALL records are correctly migrated into the user to keep.
Finally, if the adhoc task reaches a number of N failed attempts, the adhoc task should conclude without exceptions to prevent queueing again this same adhoc task and repeat it. The number of N
attempts has to be a new setting on this plugin, configurable by the admins. Default to 3.
This way, according to the above processing, before finishing the adhoc task, the adhoc task detects if the current number of processings => N, the adhoc task will update the status to completed with errors. (i.e., even with all the current retries, the merging process could not end with success)
, and conclude normally. We should add/append a last log informing that the request was not processed anymore after N
attempts.
You can see examples in Moodle core of adhoc tasks, and see how they can work.
Thanks a lot for your time,
Jordi
thank you jpahullo,
I started to write the adhoc task. I need an explanation about these two points:
-
"If 0 users match any of the remove or keep users, the merge request is updated its status as failed:" The match of the remove or keep users should be find in user table or elsewhere ? Otherwise if the remove or keep users should be matched in tool_mergeusers_queue table can you explain better the meaning to find 0, 1 or more occurrences of them in the table ? In the last case how is it possible to find no occurrences of a user to keep or remove if they have been called by the current adhoc task ?
-
"Otherwise, there is only a unique user.id that match the criteria for both remove and keep users. In this case, we can use the existing plugin API, passing both user.id." Is there an API to merge two user accounts ? Perhaps you mean the function merge() from the class MergeUserTool in the mergeusertool.php ?
Thanks again for clarifying my doubts.
Hi @nvallinoto !
About point 2), you're right: it was https://github.com/ndunand/moodle-tool_mergeusers/blob/master/lib/mergeusertool.php#L190.
About point 1), the merge request stores initially only data like: detail for user to remove: "idnumber=12345F"; detail for user to keep: "username=678678678-k". So, it is possible that when enters into action the adhoc task, for any reason, the user to remove or the user to keep does not exists. We have not to assume that there will be always a single user matching. We must cover all cases.
Thank you so much!
Jordi
@jpahullo, about point 1) the check if exists only one user to keep or to remove is already done by the web service (new API) tool_mergeusers_enqueue_merging_request. If the number of users returned by input parameters choosen by the external calling function in both case of the user to keep and to remove is 0 or > 1 an exception is thrown and the API will show the reason of the error. Otherwise (number of user = 1) the web service populates the fields keepuserid and removeuserid too in the table tool_mergeusers_queue. So we don't need to do it again during the adhoc task.
Is it ok for you ? Nicola
I've updated the diagram which summarizes graphically the way I've followed in writing the code. 20230403_Moodle_MergeUserAccounts_API.pdf It's the same I uploaded in related #218 issue.
Hi Jordi, here you find my code for adhoc_task merge_user_accounts following your comment https://github.com/ndunand/moodle-tool_mergeusers/issues/207#issuecomment-1491851294 I included the termination where retries reaches max attempts (variable added in settings.php) Tell me if it meets your requirements. Nicola
<?php
/**
* Merge user accounts
* @package tool_mergeusers
* @author Nicola Vallinoto <[email protected]>
* @copyright Copyright (c) 2023 Liguria Digitale
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
namespace tool_mergeusers\task;
/**
* Adhoc task to merge user accounts.
*
* @package tool_mergeusers
* @author Nicola Vallinoto <[email protected]>
* @copyright Copyright (c) 2023 Liguria Digitale
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* */
class merge_user_accounts extends \core\task\adhoc_task {
/**
* Execute the task.
*/
/**
* STATUS = missing merging request id This id really does not exist.
* @var integer
*/
const STATUS_MISSING_MERGING_REQUEST_ID = 1;
/**
* STATUS = merging request queued but not yet processed
* @var integer
*/
const STATUS_QUEUED_NOT_PROCESSED = 2;
/**
* STATUS = merging request queued, to be processed (the adhoc task is created for it)
* @var integer
*/
const STATUS_QUEUED_TO_BE_PROCESSED = 3;
/**
* STATUS = merging request in progress but not concluded (its adhoc_task has started)
* @var integer
*/
const STATUS_INPROGRESS_NOT_CONCLUDED = 4;
/**
* STATUS = tried with error; pending of retrial (the merging process has found some error
* and the adhoc_task is queued to be retried in the future)
* @var integer
*/
const STATUS_TRIED_WITH_ERROR = 5;
/**
* STATUS = completed with success
* @var integer
*/
const STATUS_COMPLETED_WITH_SUCCESS = 6;
/**
* STATUS = completed with errors (even with all the current retries the merging process
* could not end with success).
* @var integer
*/
const STATUS_COMPLETED_WITH_ERRORS = 7;
public function execute($mergerequestid) {
// Merge the users.
$maxattempts = get_config('tool_mergeusers', 'maxattempts');
$tablemergingrequests = 'tool_mergeusers_queue';
$record = $DB->get_record($tablemergingrequests, ['id' => $mergerequestid]);
$usertokeep = $record->keepuserid;
$usertoremove = $record->removuserid;
$retries = $record->retries + 1;
$logs = $record->log;
$this->update_retries_in_table($tablemergingrequests,
$mergerequestid,
$retries);
$log = array();
$success = true;
$mut = new MergeUserTool();
list($success, $log, $logid) = $mut->merge($usertokeep, $usertoremove);
if ($success == true) {
$status = STATUS_COMPLETED_WITH_SUCCESS;
$logs = $logs.' '.$log;
$this->update_status_and_log_in_table($tablemergingrequests,
$mergerequestid,
$status,
$logs);
// We run the merge request AGAIN because the user may be interacting with Moodle
// while merge request is being processe, so that NO ALL records are correctly migrated
// into the user to keep. It will ensure ALL records are correctly migrated into the user to keep
$log2 = array();
$success2 = true;
$mut2 = new MergeUserTool();
list($success2, $log2, $logid2) = $mut2->merge($usertokeep, $usertoremove);
// Repeat again the update
if ($success2 == true) {
$status = STATUS_COMPLETED_WITH_SUCCESS;
} elseif ($success2 == false) {
$status = STATUS_TRIED_WITH_ERROR;
}
// END.
} elseif ($success == false) {
if ($retries >= $maxattempts) {
$status = STATUS_COMPLETED_WITH_ERRORS;
// Send a notification to administrator ?
}
else {
$status = STATUS_TRIED_WITH_ERROR;
}
$logs = $logs.' '.$log;
$this->update_status_and_log_in_table($tablemergingrequests,
$mergerequestid,
$status,
$logs);
}
$logs = $logs.' '.$log2;
$this->update_status_and_log_in_table($tablemergingrequests,
$mergerequestid,
$status,
$logs);
// Reset mut session.
$SESSION->mut = null;
}
/**
* Function for updating number of retries.
*/
private function update_retries_in_table(string $table,
int $idrecord,
int $retries) {
global $DB;
$timenow = time();
$sql = $DB->update_record(
$table,
[
'id' => $idrecord,
'retries' => $retries,
'timecompleted' >= $timenow
],
$bulk = false
);
}
/**
* Function for updating the status and log of the merging request.
*/
private function update_status_and_log_in_table(string $table,
int $idrecord,
int $status,
string $log) {
global $DB;
$timenow = time();
$sql = $DB->update_record(
$table,
[
'id' => $idrecord,
'status' => $status,
'timecompleted' >= $timenow,
'log' => $log
],
$bulk = false
);
}
}
Hi @nvallinoto ,
Thanks a lot for your work. It is being so good.
There are some comments I want to make:
- Adhoc task have to always look for the criteria of the merge requests (related to your comment). Running the search criteria when queueing the request is not sufficient. Maybe there are several users that match the criteria when the adhoc task is being run. So, I wouldn't perform the search of the user.ids for the users to keep and remove while queueing the request. I just would record the requests and that's all. If you want, you can leave the user search while queueing, so it is like an early detection of wrong request (not so specific). In the end, I would run again the search criteria at the top of the adhoc task.
- Regarding to the adhoc-task (your comment). It is so good. I'd like to mention some things however:
- STATUS_* attributes should go to some other entity named
merge_request
or so, that would be the mapping of the table record. So, themerge_request
class has all the "things". Thestatus
is not something related to the adhoc task, but to themerge_request
. - The STATUS_* PHPDoc: I would remove the prefix "STATUS = ...", leaving just its explanation.
- The log field is a JSON field on the database table. So, you should work with it as a JSON entity. So, we should load it as
$arraylogs = json_decode($logs, true);
or so. Then, to separate every try, maybe we should build a bidimensional array like$arraylogs[$retries]=$logs;
, so that we can gather every retry logs into a different set of logs. This would affect also to the renderer, to properly output the logs for every retry, iterating first by the retries array, and then keep the current loop as a nested toop to iterate over all log entries.
- STATUS_* attributes should go to some other entity named
In general, I propose something like this:
<?php
/**
* Merge user accounts
* @package tool_mergeusers
* @author Nicola Vallinoto <[email protected]>
* @copyright Copyright (c) 2023 Liguria Digitale
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
namespace tool_mergeusers\task;
/**
* Adhoc task to merge user accounts.
*
* @package tool_mergeusers
* @author Nicola Vallinoto <[email protected]>
* @copyright Copyright (c) 2023 Liguria Digitale
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* */
class merge_user_accounts extends \core\task\adhoc_task {
/**
* Missing merging request id This id really does not exist.
* @var integer
*/
const STATUS_MISSING_MERGING_REQUEST_ID = 1;
/**
* Merging request queued but not yet processed.
* @var integer
*/
const STATUS_QUEUED_NOT_PROCESSED = 2;
/**
* Merging request queued, to be processed (the adhoc task is created for it).
* @var integer
*/
const STATUS_QUEUED_TO_BE_PROCESSED = 3;
/**
* Merging request is in progress but not concluded (its adhoc_task has started).
* @var integer
*/
const STATUS_INPROGRESS_NOT_CONCLUDED = 4;
/**
* Tried with error; pending of retrial (the merging process has found some error
* and the adhoc_task is queued to be retried in the future).
* @var integer
*/
const STATUS_TRIED_WITH_ERROR = 5;
/**
* Completed with success.
* @var integer
*/
const STATUS_COMPLETED_WITH_SUCCESS = 6;
/**
* Completed with errors (even with all the current retries the merging process
* could not end with success).
* @var integer
*/
const STATUS_COMPLETED_WITH_ERRORS = 7;
/**
* Reference table for merge requests.
*/
const TABLE_MERGE_REQUEST = 'tool_mergeusers_queue';
public function execute($mergerequestid) {
global $DB, $SESSION;
// Merge the users.
$maxattempts = get_config('tool_mergeusers', 'maxattempts');
$record = $DB->get_record(self::TABLE_MERGE_REQUEST, ['id' => $mergerequestid]);
// MISSING: Look for user to keep and user to remove criteria, and fail if it matches no user or > 1 users.
$mergerequest = $this->merge($record, $maxattempts, self::STATUS_TRIED_WITH_ERROR);
if ($mergerequest->status == self::STATUS_COMPLETED_WITH_SUCCESS) {
/* We run the merge request AGAIN because the user may be interacting with Moodle
* while merge request is being processe, so that NO ALL records are correctly migrated
* into the user to keep. It will ensure ALL records are correctly migrated into the user to keep. */
$mergerequest = $this->merge($mergerequest, $maxattempts, self::STATUS_COMPLETED_WITH_ERRORS);
}
if ($mergerequest->status != self::STATUS_COMPLETED_WITH_ERRORS &&
$mergerequest->status != self::STATUS_COMPLETED_WITH_SUCCESS) {
// TODO: implement this new exception, extending from moodle_exception.
// Throwing exception will ensure this adhoc task is re-queued until $maxretries is reached.
throw new failed_merge_request($mergerequest->retries, $mergerequest->status);
}
}
/**
* Function for updating number of retries.
*/
private function update_retries_in_table(int $idrecord,
int $retries) {
global $DB;
$DB->update_record(
self::TABLE_MERGE_REQUEST,
(object)[
'id' => $idrecord,
'retries' => $retries,
'timecompleted' >= time(),
]
);
}
/**
* Function for updating the status and log of the merging request.
*/
private function update_status_and_log_in_table(int $idrecord,
int $status,
string $log) {
global $DB;
$DB->update_record(
self::TABLE_MERGE_REQUEST,
(object)[
'id' => $idrecord,
'status' => $status,
'timecompleted' >= time(),
'log' => json_encode($log),
]
);
}
private function merge(stdClass $record, int $maxattempts, int $statusforerror) {
$mergerequestid = $record->id;
$usertokeep = $record->keepuserid;
$usertoremove = $record->removuserid;
$retries = $record->retries + 1;
$logs = $record->log;
$mut = new MergeUserTool();
list($success, $log, $logid) = $mut->merge($usertokeep, $usertoremove);
if ($success) {
$status = STATUS_COMPLETED_WITH_SUCCESS;
} else {
if ($retries >= $maxattempts) {
$status = STATUS_COMPLETED_WITH_ERRORS;
// Send a notification to administrator ?
} else {
$status = $statusforerror;
}
}
$logs[$retries] = $log;
$this->update_status_and_log_in_table(self::TABLE_MERGE_REQUEST,
$mergerequestid,
$status,
$logs);
$record->log = $logs;
$record->status = $status;
}
}
There are some lines to update properly, and add a new exception class, to say the least. I keep the STATUS_* attributes here, but they should be moved to the new merge_request
class. I simplified the if-then-else when merging request, encapsulating code in another method.
Thanks a lot,
Jordi
By the way, @nvallinoto ,
We should have a field name completiontime
and another different modifiedtime
:
completedtime
would be only filled in when the merge request is completed (either with error or success).
The other times, only modifiedtime
should be updated instead.
This way, it is another way to know if a merge request is completed or not, and to separate concerns: I don't feel good updating the field completedtime
when just updating it, and the merge request is not really completed.
In another concern, look at the status set when retrying on success: there is no opportunity to repeat when merge request was ok and it is repeated to ensure all (if any) data is properly migrated. This way, we prevent entering to an infinite loop of retries.
Thanks a lot,
Jordi
I added completedtime for a mistake. I think we can use only modifiedtime.
Ok. We can go on with just modifiedtime
.
Then, we should add a a log line into the merge requests log, that modifiedtime too, in order to render when the merge request was executed every retry.
in your code the update of the field retries disappeared. Perhaps it's a only simple forgetting.
Yes, it was. I forget to keep that part.
Thanks!
Hi @jpahullo,
thank for new code flux, more readable than mine.
I'm completing the code adding the retries field update.
Just two questions:
- merge_request class. Where to define it and what to be included in it ?
- failed_merge_request. Where to define it? Do you have an example how to implement the new exception extending from moodle_exception ? Thanks again. Nicola
Any new class, as Moodle Dev documentation says, should be placed into classes/
directory. They can go directly there if necessary, or in a classes/local/
directory, with the corresponding namespace.
Thanks!
The merge_request class could be something like:
<?php
/**
* Merge user accounts
* @package tool_mergeusers
* @author Nicola Vallinoto <[email protected]>
* @copyright Copyright (c) 2023 Liguria Digitale
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
namespace tool_mergeusers;
/**
* Merge request definition.
*
* @package tool_mergeusers
* @author Nicola Vallinoto <[email protected]>
* @copyright Copyright (c) 2023 Liguria Digitale
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* */
class merge_request {
/**
* Missing merging request id This id really does not exist.
* @var integer
*/
const STATUS_MISSING_MERGING_REQUEST_ID = 1;
/**
* Merging request queued but not yet processed.
* @var integer
*/
const STATUS_QUEUED_NOT_PROCESSED = 2;
/**
* Merging request queued, to be processed (the adhoc task is created for it).
* @var integer
*/
const STATUS_QUEUED_TO_BE_PROCESSED = 3;
/**
* Merging request is in progress but not concluded (its adhoc_task has started).
* @var integer
*/
const STATUS_INPROGRESS_NOT_CONCLUDED = 4;
/**
* Tried with error; pending of retrial (the merging process has found some error
* and the adhoc_task is queued to be retried in the future).
* @var integer
*/
const STATUS_TRIED_WITH_ERROR = 5;
/**
* Completed with success.
* @var integer
*/
const STATUS_COMPLETED_WITH_SUCCESS = 6;
/**
* Completed with errors (even with all the current retries the merging process
* could not end with success).
* @var integer
*/
const STATUS_COMPLETED_WITH_ERRORS = 7;
private $data;
private function __construct(stdClass $data) {
$this->data = $data;
}
public static function from(stdClass $data) {
return new self($data);
}
public function __get($name)
{
if (isset($this->data->${name})) {
$value = $this->data->${name};
if ($name == 'log') {
$value = json_decode($value, true);
}
return $value;
}
return null;
}
public function __set($name, $value)
{
if (isset($this->data->${name})) {
if ($name == 'log') {
$value = json_encode($value);
}
$this->data->${name} = $value;
}
}
public function convert_log_to_new_format() {
if (!isset($this->data->log) || empty($this->data->log)) {
return;
}
$logs = [];
$logs[1] = json_decode($this->data->log, true);
$this->data->log = json_encode($logs);
}
public function get_record(): \stdClass {
return $this->data;
}
}
every method has a meaning. There is a method for converting logs to the new format, to be called for the migration process.
The expected classpath should be classes/merge_request.php
for the given namespace.
The use case should be like:
$record = $DB->get_record(...);
$request = merge_request::from($record);
// ....
$DB->update_record($request->get_record());
// ...
// When migrating:
$request->convert_log_to_new_format();
However, you can also implement another constructor method like merge_request::from_old_record(stdClass $record);
and in that method adapt the fields and values to the new format, and you could move code from the method convert_log_to_new_format()
into that constructor, and finally, after all conversions are done, you can then return new self($record_in_new_format);
or so.
Thanks,
Jordi
to use the constants and function defined in classes/merge_request.php in the file classes/task/merge_user_accounts.php it is sufficient to write
merge_request::STATUS_COMPLETED_WITH_SUCCESS
and include the file where constants or functions are included.
in our case
require_once( '../classes/merge_request.php');
right ?
Yes!
Moodle will autoload class when using it. Remember to put use \tool_mergeusers\merge_request;
at the top.
Thanks!
Also, if you need some specific field from an instance of merge_request
, just do $request->log
and that attribute will be returned. And for updating specific fields, just $request->useridtokeep = $user->id;
, for instance.
Thanks,
Jordi
I tried to use it in external.php file (it is in the same folder classes where merge_request.php is located)
require_once __DIR__ . '/merge_request.php';
and when I try to access a constant using this code
$reply = merge_request::TRIED_WITH_ERROR;
the program returns me that
Class "merge_request" not found"
The autoloading is not working but I don't know why
it worked adding use code
require_once __DIR__ . '/merge_request.php';
use \tool_mergeusers\merge_request;
Yes, “use …” statements are mandatory. “require_once …” not.
Try it without the require and it will work as expexted. This is the magic of Moodle autoliading.
Thanks!!!
Jordi
Hi @jpahullo, to finish the code of web services and tasks and starting to test it I need to write code for:
- exporting data from old table to new table inside update.php (I will do it with a cycle in old table of merging requests moving each record in the new table)
- failed_merge_request
// TODO: implement this new exception, extending from moodle_exception.
// Throwing exception will ensure this adhoc task is re-queued until $maxretries is reached.
throw new failed_merge_request($mergerequest->retries, $mergerequest->status);
Here my question for the 2nd point "Where to define it? Do you have an example how to implement the new exception extending from moodle_exception ?"
//require_once DIR . '/merge_request.php'; use \tool_mergeusers\merge_request; without require_once line the answer is: Class "tool_mergeusers\merge_request" not found"