Merge user accounts - add web service functionality - #248
Conversation
- using correct test class names - removed deprecated legacy functions - updated renamed autoloader class calls - replaced deprecated function recompute_all_final_grades() - updated moodle_ci.yml
|
Hi @nvallinoto, I have revisited your new PR. It is mainly what we had last time, hoping this time is cleaner the git history. I detected some things:
Would you mind to go on with the work, please, and complete this part? Thank you a lot for your work. Jordi |
| if ($mergerequestresult->status != merge_request::COMPLETED_WITH_ERRORS && | ||
| $mergerequestresult->status != merge_request::COMPLETED_WITH_SUCCESS) { | ||
| // Throwing exception will ensure this adhoc task is re-queued until $maxretries is reached. | ||
| throw new moodle_exception(get_string('failedmergerequest', 'tool_mergeusers')); |
There was a problem hiding this comment.
I would also add the current $mergerequest->id and $mergerequest->retries into the message of the exception, into the failedmergerequest string. This way, we provide enough context into the exception message and int he trace log.
There was a problem hiding this comment.
I don't the the parameters ont the get_string() yet.
There was a problem hiding this comment.
revisit the API of the constructor of moodle_exception, function __construct($errorcode, $module='', $link='', $a=NULL, $debuginfo=null) {
the instantiation should be something like:
throw new moodle_exception('failedmergerequest', 'tool_mergeusers');
and if this, imagine, has to have some parameters on the string, it should be called like this:
throw new moodle_exception('failedmergerequest', 'tool_mergeusers', '', $a);
Revisit all moodle_exception to be invoked this way.
|
Hi @nvallinoto, Try to address most of the proposed changes. If you feel that certain proposed changes are not fully understood, please do not do any change, just in case. I propose you to make all or most of the proposed changes, update the PR, and let me check it again. I would run it locally with the merge request log from my production system to check if it behaves properly. We have thousands of merge requests from the old format. We need to close this PR, this part of the task, to follow on the next steps: review generating merge request from web, review the cli interface, and reformat the web log of requests, to show now the merge requests. This way, only having all these jobs completed, we will be able to release this new feature. Thanks a lot for your work. Jordi |
nvallinoto
left a comment
nvallinoto
left a comment
There was a problem hiding this comment.
I have modified and integrated all the requests from the previous commit. I code checked the files and I did a test using the web service to enqueue a merge request, running the task to move the request to adhoc task and executed it.
The log of the merge request is the following:
{"1689252308":["For logging or security reasons we are skipping <strong>my_pages, user_preferences, user_info_data, user_private_key<\/strong>.\n <br \/>To remove these entries, delete the old user once this script has run successfully.","Started merging at gioved\u00ec, 13 luglio 2023, 14:44","UPDATE mdl_user_enrolments SET userid = '24' WHERE id IN (103)","UPDATE mdl_role_assignments SET userid = '24' WHERE id IN (102)","UPDATE mdl_logstore_standard_log SET relateduserid = '24' WHERE id IN (11768, 11769, 11770, 11774, 11775, 11777)","Finished merging at gioved\u00ec, 13 luglio 2023, 14:44","Merge took 0 seconds"],"1689252369":["For logging or security reasons we are skipping <strong>my_pages, user_preferences, user_info_data, user_private_key<\/strong>.\n <br \/>To remove these entries, delete the old user once this script has run successfully.","Started merging at gioved\u00ec, 13 luglio 2023, 14:46","Finished merging at gioved\u00ec, 13 luglio 2023, 14:46","Merge took 1 seconds"]}
Calling the webservice enqueue_merge_request I tried too a case where the keep user returned more than one record. It finished with an error as expected:
{ "exception": "Error", "errorcode": null, "message": "Class \"tool_mergeusers\\external\\Exception\" not found"}
Perhaps there is a way to return an understandable message. In our code we raise an exception like this:
throw new Exception(get_string('toomanyusers', 'tool_mergeusers', (object)['userfield' => $keepuserfield, 'uservalue' => $keepuservalue]));
}`.
|
Yes, you need to add
at the beginning of the file, or throw it adding a backlslash |
|
All this "class not found" are mostly similar to this. I prefer adding |
|
It worked. This is the answer of the web service call: |
| $string['toomanyusers'] = 'More than one user found with {$a->userfield} {$a->uservalue}.'; | ||
| $string['maxattempts'] = 'Max number of attempts of a merge request.'; | ||
| $string['maxattempts_desc'] = 'This is the maximum number of attempts that a merge request will be tried to merge. After that number of attempts without success, the request will not be processed anymore and it will be updated as ended with errors.'; | ||
| $string['failedmergerequest'] = 'Failed merge request n° {$a->id}. Number of attempts: {$a->retries}'; |
There was a problem hiding this comment.
do not use º, instead use the word, in this case, Failed merge request with id {$a->id}, on the attempt number {$a->retries}..
There was a problem hiding this comment.
You left 2 spaces between reqeust with. Leave just a single space.
There was a problem hiding this comment.
I committed a new version with all requests you done during the weekend. I codechecked the files. We need to solve and decide how to act in the following cases:
- merge action for users that need only to update username
- add a new variable successful retries in merge_requests.php class
I make some test. Please wait before reviewing again the code.
For the merge request processed as an update, it would need to refactor a bit the business logic. Leave this part to the end. If there is availability, analyse the case given the conditions I gave you, in a different commit from the rest. To add a new variable successful retries, just proceed as you believe. Give yourself a try. You can. Revisit the whole code to consider this new field (datbase field, install.xml, upgrade.php, code, web services). I think it is more important to conclude with this, than the update for username/idnumber/etc. Thanks a lot! |
I have seen inside mod/quiz/classes and don't have both files. What I see are the following files and folders: |
I have the settings.php file inside the folder in file quizattemptsmerger.php I have |
|
mod/quiz/settings.php is not what you are looking for. It should be another class. Could you look for this class on Moodle 3.11 or Moodle 3.10, please? |
now I have Moodle 4.1.4 |
|
what is your actual situation (Moodle version and position of quiz_settings.php) ? |
|
I have discovered that this is something specific for Moodle 4.2. So, we have to work on Moodle 4.2 onwards for master branch. Since then, we have to work with the branch MOODLE_310_STABLE or MOODLE_401_STABLE of this plugin, depending on what Moodle version you have. You have to do: git checkout yourbranch So that the version of the stable branch is the lowest Moodle version that this branch gives support to, till the next one (excluded). So that MOODLE_310_STABLE covers M3.10, 3.11, 4.0, and MOODLE_401_STABLE covers only M4.1, since master branch covers M4.2 onwards. Sorry for the confusion. |
|
since I have Moodle 4.1.4 I need to rebase MOODLE_401_STABLE. right ? |
|
this is what I need to do right ? MOODLE_401_STABLE is a constant ? |
|
I tried git rebase MOODLE_401_STABLE and received this message: |
Yeah, how are you defining this repo? Supposo you named it like With this assumption you have to do like
|
|
a local level I have two branches:
|
|
Mmmmm.... Actually, you are not pointing to master, I realized you're pointing to the custom branch jpahullo:wip-ws-tasks. So, I will rebase it onto MOODLE_401_STABLE and so you should get the whole work operating properly again. |
I did
and now ? |
ok tell me what to do after your rebase onto MOODLE_401_STABLE |
|
Updated branch to point to MOODLE_401_STABLE. I suggest you to remove your local wip-ws-tasks branch, and then put your commits on to that branch (git checkout master; git rebase mua/wip-ws-tasks) |
I deleted my local wip-ws-task (I removed ws branch). Then I switched to master branch. git status on branch master gave your branch is up to date with mua/master. After that I tried to git rebase mua/wip-ws-task but the answer was fatal: invalid upstream mua/wip-ws-task. At local level git branch gave: While git remote -v gave |
|
I have created the branch https://github.com/jpahullo/moodle-tool_mergeusers/tree/nvallinoto-master on this repo that is equivalent to your nvallinoto/master branch, but pointing onto the wip-ws-tasks branch. To complete this command, I had to run this command: You can use this repo branch with name nvallinoto-master to be your new nvallinoto/master branch. For you of the future, I recommend you to build a new branch for any single development. I use to use the issue number as the prefix of the branch, like 218-ws for the issue number #218. This way, you leave master branch intact and can be updated from the upstream repository without problem (important!). And you have your work separated by branches. This way, if you have to reprioritize any other work, save your changes into the branch, and change to the other branch (or create a new one) to address the other more important job. |
| // Append logs to the list. | ||
| // $baseitem->log[$item->timemodified] = json_decode($item->log, false); | ||
| $logs[$record->timeadded] = $log; | ||
| $this->update_status_and_log_in_table($record->id, |
There was a problem hiding this comment.
I think we should add a new method to the merge_request class, like this:
public function get_log_as_json(): string {
return $this->data->log;
}And update the __set() method to consider this, by adding this piece of code:
if ($name == 'status &&
($value == merge_request::COMPLETED_WITH_SUCCESS ||
$value == merge_request::COMPLETED_WITH_ERRORS)) {
$this->timecompleted = time();
}Then, to simplify the operation of the update_status_and_log method, by having this:
private function update_status_and_log(merge_request $mergerequest): void {
...
$update = (object)[
'id' => $mergerequest->id,
'status' => $mergerequest->status,
'timecompleted' => $mergerequest->timecompleted,
'log' => $mergerequest->get_log_as_json(),
];
$DB->....
}so we fully delegate on the merge_request class to update any field according to the business logic, internally, and not externally.
Also, the 2 lines have to be in the reverse order:
$mergerequest->status = $status;
$this->update_status_and_log($mergerequest);So the status is up-to-date when updating it inside the update_status_and_log method.
| $users = $DB->get_records(merge_request::TABLE_USERS, [$userfield => $uservalue]); | ||
| if (count($users) == 0) { | ||
| throw new moodle_exception(get_string('cannotfinduser', 'tool_mergeusers', | ||
| (object)['userfield' => $userfield, 'uservalue' => $uservalue])); |
| ['status' => merge_request::QUEUED_NOT_PROCESSED], | ||
| $sort = '', | ||
| $fields = 'id'); | ||
| $conta = 0; |
There was a problem hiding this comment.
according to the recordset API, you have to close the recordset at the end, out from the foreach, like $mergerequestsnotyetscheduled->close();
| if ($mergerequestresult->status != merge_request::COMPLETED_WITH_ERRORS && | ||
| $mergerequestresult->status != merge_request::COMPLETED_WITH_SUCCESS) { | ||
| // Throwing exception will ensure this adhoc task is re-queued until $maxretries is reached. | ||
| throw new moodle_exception(get_string('failedmergerequest', 'tool_mergeusers')); |
There was a problem hiding this comment.
revisit the API of the constructor of moodle_exception, function __construct($errorcode, $module='', $link='', $a=NULL, $debuginfo=null) {
the instantiation should be something like:
throw new moodle_exception('failedmergerequest', 'tool_mergeusers');
and if this, imagine, has to have some parameters on the string, it should be called like this:
throw new moodle_exception('failedmergerequest', 'tool_mergeusers', '', $a);
Revisit all moodle_exception to be invoked this way.
| */ | ||
| public function __get($name) { | ||
| if (isset($this->data->{$name})) { | ||
| $value = $this->data->{$name}; |
There was a problem hiding this comment.
It is happing something similar yet:
check tabulars and characters, only spaces have to exist there.
| } | ||
| $mergerequest->append_log($log, time()); | ||
| if ($mergerequest->retries > $maxattempts) { | ||
| $log_attempts = "Reached the number of maximum attempts"; |
There was a problem hiding this comment.
lines like $log_attempts = get_string('maxattemptsreached', 'tool_mergeusers');
should be like:
$log_attempts = get_string('maxattemptsreached', 'tool_mergeusers', (object)['retries'=>$mergerequest->retries, 'maxretries' => $maxattempts]);
in order to report both numbers in the resulting string. Update the lang file properly too.
I commented this in some other comment, but I cannot find it over here.
| ])); | ||
| } | ||
| if (!empty($errorfound)) { | ||
| $mergerequest->appendlog([$errorfound], time()); |
| $string['toomanyusers'] = 'More than one user found with {$a->userfield} {$a->uservalue}.'; | ||
| $string['maxattempts'] = 'Max number of attempts of a merge request.'; | ||
| $string['maxattempts_desc'] = 'This is the maximum number of attempts that a merge request will be tried to merge. After that number of attempts without success, the request will not be processed anymore and it will be updated as ended with errors.'; | ||
| $string['failedmergerequest'] = 'Failed merge request n° {$a->id}. Number of attempts: {$a->retries}'; |
There was a problem hiding this comment.
You left 2 spaces between reqeust with. Leave just a single space.
|
I created #250 for the other part of the work, to address it apart from this PR. |
|
@nvallinoto, you asked me how to proceed with this PR now. This is more related to git branch management that anything else. What I recommend you is the following: Lines with |
I did the first step until this one:
it gave me an error because I don't have mua/nvallinoto-master but only mua/master If I exec
I obtain this: Perhaps I have to add a remote repository with this url ?
thanks |
|
Ok, @nvallinoto I edited my comment, I was confused about the name of the remotes. So to unify remotes naming, I added two first commands. It will be essier to interact with you in case of doubts |
so now I have as remote repo: and now to start in the right way I need to |
|
I put it in the comment: git checkout -b 218-ws upstream/nvallinoto-master this will create a local branch pointing to the remote nvallinoto-master |
done. now I can start again to update branch on this PR. |
I tried to push upstream/nvallinoto-master but authentication failed |
|
Nops, you cannot push to this repository. You are working now onto your local branch 218-ws, you have to push it to your repo, and then update this PR with this new branch or create a new PR. We can continue there the revision if necessary. So, you need to do something like: git checkout 218-ws
git push --set-upstream origin 218-wsthen, visit this PR and try to update the referenced branches. It it is not possible, create a new PR. It does not matter. In any case, PR should be between wip-ws-tasks and your 218-ws branch. |
2 participants
This PR is a follow up of a previous PR (#242).
It is the development of the issue #218 to implement web service tool for merge users plugin.
In the above image you will find a visual representation of the changes needed to implement the new web service functionality.
The code has been tested only in local environment. It should be checked with behat or phpunit test running the files included in test folder.
In file revisited_file.php are listed the new files (added) and the update files (modified).
The files' code has been checked with code checker plugin. Except for mergeusertool.php that already existed and has names of variables in upper case.
Lot of job is already done and more has to come. Many thanks to Jordi for solving troubles step by step.