Merge user accounts - add web service functionality

[nvallinoto]

This PR is a follow up of a previous PR (https://github.com/jpahullo/moodle-tool_mergeusers/pull/242). It is the development of the issue https://github.com/jpahullo/moodle-tool_mergeusers/issues/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.

[jpahullo]

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:

• Things that I commented on the old PR but that were not addressed, like using a merge_request instance instead of a stdclass instance.
• New things, related to scenarios that may present infinite loops for the adhoc task due to failing finding some of the users.
• Other things that, maybe because today I have a fresh mind, I could see today, and not before. Sorry for this.

Would you mind to go on with the work, please, and complete this part?

Thank you a lot for your work.

Jordi

[jpahullo]

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

[jpahullo]

Yes, you need to add

use Exception;

at the beginning of the file, or throw it adding a backlslash throw new \Exception(...);

[jpahullo]

All this "class not found" are mostly similar to this.

I prefer adding use Exception; (or whatever the class is) at the beginning of the file, just in case it is used several times within the file. This way preventing more runtime errors of the same type.

[nvallinoto]

It worked. This is the answer of the web service call: { "exception": "Exception", "errorcode": null, "message": "More than one user found with email [email protected]." } I'll push the file

[jpahullo]

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!

[jpahullo]

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!

which is the main reason why you would like to add the new variable successful_retries ? If it is useful I can revisit the whole code. If we can do the same with a unique variable perhaps is better to leave the things as they are.

Yeah, the important thing is to let administrator know the status of the merge request. If we only show that the last attempt went ok, but it took 4 attempts, is something difficult to understand. Also, if it shows that the last attempt was failed, but it took 3 attempts.

According to our design, in a perfect scenario, there should a total of 2 retries, and both of them ok. At least, the last 2 retries going ok.

The status is somehow necessary, and this number of successful retries would be really useful to have a perfect control of what happened.

Having said all this, let's conclude this PR without these two points. I'll create two issues for them, and they will be addressed later on.

Thanks for asking.

So, right now, forget about this 2 aspects, and revisit any comment I made. I do not why, sometimes you cannot get to all the comments I made and I do not see any change on them. It happened several times during these weeks.

Let me know when you have this PR for complete. Then I'll make a new review.

Thanks for your work.

[nvallinoto]

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!

which is the main reason why you would like to add the new variable successful_retries ? If it is useful I can revisit the whole code. If we can do the same with a unique variable perhaps is better to leave the things as they are.

Yeah, the important thing is to let administrator know the status of the merge request. If we only show that the last attempt went ok, but it took 4 attempts, is something difficult to understand. Also, if it shows that the last attempt was failed, but it took 3 attempts.

According to our design, in a perfect scenario, there should a total of 2 retries, and both of them ok. At least, the last 2 retries going ok.

The status is somehow necessary, and this number of successful retries would be really useful to have a perfect control of what happened.

Having said all this, let's conclude this PR without these two points. I'll create two issues for them, and they will be addressed later on.

Thanks for asking.

So, right now, forget about this 2 aspects, and revisit any comment I made. I do not why, sometimes you cannot get to all the comments I made and I do not see any change on them. It happened several times during these weeks.

Let me know when you have this PR for complete. Then I'll make a new review.

Thanks for your work.

ok. I miss these two points - for the moment - and I'll conclude of all your comments. In the last few hours I done some commits that took in account some of your previous comments. I went into the page of 'files changed' to view all of them.

[nvallinoto]

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!

Let's consider that we have another field that indicates us the status of the merge request: status . Perhaps we can use it to give more information ?

[nvallinoto]

after having changed all files I made a new cycle of tests starting from calling the new two web service. After that I launched a merge operation via cli command php admin/cli/adhoc_task.php -e in order to run the adhoc task. I received an error that surprised me.

`Execute adhoc task: tool_mergeusers\task\merge_user_accounts Adhoc task id: 176 Adhoc task custom data: {"mergerequestid":"4"}... started 17:26:59. Current memory use 14.3 MB. Database transaction aborted automatically in tool_mergeusers\task\merge_user_accounts... used 645 dbqueries ... used 142.84206485748 seconds Adhoc task failed: tool_mergeusers\task\merge_user_accounts,Class "mod_quiz\quiz_settings" not found Backtrace:

• line 285 of /admin/tool/mergeusers/lib/table/quizattemptsmerger.php: call to QuizAttemptsMerger->updateQuizzes()
• line 251 of /admin/tool/mergeusers/lib/table/quizattemptsmerger.php: call to QuizAttemptsMerger->updateAllQuizzes()
• line 127 of /admin/tool/mergeusers/lib/table/quizattemptsmerger.php: call to QuizAttemptsMerger->renumber()
• line 266 of /admin/tool/mergeusers/lib/mergeusertool.php: call to QuizAttemptsMerger->merge()
• line 191 of /admin/tool/mergeusers/lib/mergeusertool.php: call to MergeUserTool->_merge()
• line 89 of /admin/tool/mergeusers/classes/task/merge_user_accounts.php: call to MergeUserTool->merge()
• line 59 of /admin/tool/mergeusers/classes/task/merge_user_accounts.php: call to tool_mergeusers\task\merge_user_accounts->merge()
• line 359 of /lib/cronlib.php: call to tool_mergeusers\task\merge_user_accounts->execute()
• line 198 of /lib/cronlib.php: call to cron_run_inner_adhoc_task()
• line 131 of /admin/cli/adhoc_task.php: call to cron_run_adhoc_tasks() Ran 1 adhoc tasks found at Mon, 17 Jul 2023 17:26:59 +0200`

The only change I made in mergeusertool.php file is to remove $logid from

   $logid = $this->logger->log($toid, $fromid, $success, $log);
        return array($success, $log, $logid);

to return array($success, $log); I don't think could be the cause.

[jpahullo]

This error due to the quiz related class is due to the Moodle version. I think you are using Moodle 3.10 or later, and this class exists only on Moodle 4.0 onwards.

[nvallinoto]

This error due to the quiz related class is due to the Moodle version. I think you are using Moodle 3.10 or later, and this class exists only on Moodle 4.0 onwards.

today I updated the version of the plugin and also the moodle version that now is $version = 2022112800.01;
$release = '4.1+ (Build: 20221201)'; // Human-friendly version name $branch = '401'; // This version's branch.

This is what I read from version.php file

so ?

[jpahullo]

This error due to the quiz related class is due to the Moodle version. I think you are using Moodle 3.10 or later, and this class exists only on Moodle 4.0 onwards.

today I updated the version of the plugin and also the moodle version that now is $version = 2022112800.01; $release = '4.1+ (Build: 20221201)'; // Human-friendly version name $branch = '401'; // This version's branch.

This is what I read from version.php file

so ?

Totally unexpected. Take a look at the namespace and classes, maybe there is something wrong on M4.1. It is strange, since we have tests for up to M4.2. Take a look please.

[nvallinoto]

This error due to the quiz related class is due to the Moodle version. I think you are using Moodle 3.10 or later, and this class exists only on Moodle 4.0 onwards.

today I updated the version of the plugin and also the moodle version that now is $version = 2022112800.01; $release = '4.1+ (Build: 20221201)'; // Human-friendly version name $branch = '401'; // This version's branch. This is what I read from version.php file so ?

Totally unexpected. Take a look at the namespace and classes, maybe there is something wrong on M4.1. It is strange, since we have tests for up to M4.2. Take a look please.

I tried again to merge via adhoc task and receiving the same error. Now I have this version of Moodle:

$version  = 2022112804.06;              // 20221128      = branching date YYYYMMDD - do not modify!
                                                          //         RR    = release increments - 00 in DEV branches.
                                                         //           .XX = incremental changes.
$release  = '4.1.4+ (Build: 20230714)'; // Human-friendly version name
$branch   = '401';                     // This version's branch.

Where I have to look for namespace and classes ? Perhaps here: line 285 of /admin/tool/mergeusers/lib/table/quizattemptsmerger.php: call to QuizAttemptsMerger->updateQuizzes()

[jpahullo]

This error due to the quiz related class is due to the Moodle version. I think you are using Moodle 3.10 or later, and this class exists only on Moodle 4.0 onwards.

today I updated the version of the plugin and also the moodle version that now is $version = 2022112800.01; $release = '4.1+ (Build: 20221201)'; // Human-friendly version name $branch = '401'; // This version's branch. This is what I read from version.php file so ?

Totally unexpected. Take a look at the namespace and classes, maybe there is something wrong on M4.1. It is strange, since we have tests for up to M4.2. Take a look please.

I tried again to merge via adhoc task and receiving the same error. Now I have this version of Moodle:

$version  = 2022112804.06;              // 20221128      = branching date YYYYMMDD - do not modify!
                                                          //         RR    = release increments - 00 in DEV branches.
                                                         //           .XX = incremental changes.
$release  = '4.1.4+ (Build: 20230714)'; // Human-friendly version name
$branch   = '401';                     // This version's branch.

Where I have to look for namespace and classes ?

You have to look inside the Moodle project. If you are using an IDE, like VSCode, I think you have tools to access to classes definition (like Ctrl + click on the class name, or method) or some way of looking for strings like "class quiz_settings" or whatever.

[jpahullo]

Actually, mod_quiz\quiz_settings means the expected path is this one: mod/quiz/classes/quiz_settings.php or mod/quiz/classes/settings.php

[nvallinoto]

Actually, mod_quiz\quiz_settings means the expected path is this one: mod/quiz/classes/quiz_settings.php or mod/quiz/classes/settings.php

I have seen inside mod/quiz/classes and don't have both files. What I see are the following files and folders:
admin_review_setting.php adminpresets event local question admin_setting_browsersecurity.php analytics external navigation repaginate.php admin_setting_grademethod.php cache external.php output search admin_setting_overduehandling.php completion form plugininfo structure.php admin_setting_user_image.php dates.php group_observers.php privacy task

[nvallinoto]

Actually, mod_quiz\quiz_settings means the expected path is this one: mod/quiz/classes/quiz_settings.php or mod/quiz/classes/settings.php

I have the settings.php file inside the folder mod/quiz

in file quizattemptsmerger.php I have use mod_quiz\quiz_settings;

[jpahullo]

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?

[nvallinoto]

settings.php

now I have Moodle 4.1.4

[nvallinoto]

what is your actual situation (Moodle version and position of quiz_settings.php) ? I found a quiz_settings.php file here ./mod/quiz/accessrule/seb/classes/quiz_settings.php

[jpahullo]

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 git remote update --prune git rebase MOODLE_310_STABLE (or MOODLE_401_STABLE).

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.

[nvallinoto]

since I have Moodle 4.1.4 I need to rebase MOODLE_401_STABLE. right ?

[nvallinoto]

this is what I need to do right ? MOODLE_401_STABLE is a constant ? git checkout master git remote update --prune git rebase MOODLE_401_STABLE

[nvallinoto]

I tried git rebase MOODLE_401_STABLE and received this message: fatal: invalid upstream 'MOODLE_401_STABLE'

[jpahullo]

I tried git rebase MOODLE_401_STABLE and received this message: fatal: invalid upstream 'MOODLE_401_STABLE'

Yeah, how are you defining this repo? Supposo you named it like upstream (it's usual to name your repo like origin and then the other main repo like upstream to know where main changes come from and where are you contributing to).

With this assumption you have to do like git rebase upstream/MOODLE_401_STABLE. This has to work. Double check the name of this repo to check if you have to name it as upstream or whatever you have.

git rebase MOODLE_401_STABLE is assuming you did git checkout MOODLE_401_STABLE first, this is why it didn't work.

[nvallinoto]

a local level I have two branches: master and ws. I use master to push changes to remote repository https://github.com/nvallinoto/moodle-tool_mergeusers

• git remote -v gives: mua https://github.com/nvallinoto/moodle-tool_mergeusers (fetch) mua https://github.com/nvallinoto/moodle-tool_mergeusers (push) origin https://github.com/jpahullo/moodle-tool_mergeusers.git (fetch) origin https://github.com/jpahullo/moodle-tool_mergeusers.git (push)

[jpahullo]

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.

[nvallinoto]

I tried git rebase MOODLE_401_STABLE and received this message: fatal: invalid upstream 'MOODLE_401_STABLE'

Yeah, how are you defining this repo? Supposo you named it like upstream (it's usual to name your repo like origin and then the other main repo like upstream to know where main changes come from and where are you contributing to).

With this assumption you have to do like git rebase upstream/MOODLE_401_STABLE. This has to work. Double check the name of this repo to check if you have to name it as upstream or whatever you have.

git rebase MOODLE_401_STABLE is assuming you did git checkout MOODLE_401_STABLE first, this is why it didn't work.

I did

• git checkout MOODLE_401_STABLE Branch 'MOODLE_401_STABLE' set up to track remote branch 'MOODLE_401_STABLE' from 'origin'. Switched to a new branch 'MOODLE_401_STABLE'

• git rebase MOODLE_401_STABLE Current branch MOODLE_401_STABLE is up to date.

and now ?

[nvallinoto]

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.

ok tell me what to do after your rebase onto MOODLE_401_STABLE