Merge user accounts - New web services

[nvallinoto]

Following the issue https://github.com/ndunand/moodle-tool_mergeusers/issues/218 here you find the first part concerning the two web services. The task part will follow. This is the diagram to explain the behaviour.

[nvallinoto]

Some parts have to be fixed: 1 - external.php -> function: get_data_merging_requests -> validation_parameters for an array 2 - external.php -> function: enqueue_merging_request -> line 66 and 79 (not use the foreach but access directly to id field) 3 - test file to run the 2 webservices are missing (they have been tested with postman)

[jpahullo]

Hi @nvallinoto,

You did a big effort.

I followed command line instructions and resolved git conflicts and did a lot of other things. See branch https://github.com/ndunand/moodle-tool_mergeusers/tree/nvallinoto-master:

1. It has all your changes from this PR.
2. It also has all these changes

There are also things to do for you about this contribution:

1. external.php has two web services. Moodle development recommends having every single web service in a separate class. The name of the class on the file name. You can do that by building a new directory classes/external/ and placing there the two classes. Things to do:
   1. Split the current external.php file into two classes.
   2. Update db/services.php accordingly.
2. merge_request.php has name reference to tables. It is not the way to go. They can be placed on every place it is used, or having some kind of a new class like merge_request_factory that interacts with the infrastructure ($DB and so on), loading and storing the necessary data about merge requests. So, concentrating all the database logic.
3. Consider that install.xml and upgrade.php are updated to include the mergedbyuserid field, incorporated in the latest master branch.
4. When upgrading the plugin, try it with old records there (not just an empty table tool_mergeusers): there were NOTNULL fields related to removeuser and keepuser, at least, and they were not set on the record to store after converting. When trying things manually, perform them the more real the better. Also, id field is kept from the old table to the new table. I considered it would be ok, but take a look if I missed anything important to consider.
5. When migrating old records to new table, maybe we could add some kind of note into the log record (like a $logs[0] = "Migrated from old record with id = $oldid"; or similar). The idea would be to keep the record of what is done for later search and know when the records come from the new version of plugin or from the old one.

Also:

1. Squash git commits so that they come into one commit. Update the commit message to express the whole change. The changes to incorporate are those from wip-ws-tasks up to nvallinoto-master of this repo. You can push again into your repo and this PR will be updated automatically with your new master.
2. Remove the db/tasks.php file from this squashed commit, so it comes with a new separated commit with the part of the tasks.

By addressing all this, we can go on with the rest of pieces of this development.

Thanks a lot.

Jordi

[jpahullo]

By the way,

The db/tasks.php makes some weird output when installing the plugin on the CI:

https://github.com/ndunand/moodle-tool_mergeusers/actions/runs/4647233869/jobs/8224111250

  OUT  -->tool_mergeusers
  OUT  ++ Failed to load task: \tool_mergeusers\task\get_queue_merging_requests ++
  OUT  * line 348 of /lib/classes/task/manager.php: call to debugging()
  OUT  * line 70 of /lib/classes/task/manager.php: call to core\task\manager::scheduled_task_from_record()
  OUT  * line 90 of /lib/classes/task/manager.php: call to core\task\manager::load_default_scheduled_tasks_for_component()
  OUT  * line 684 of /lib/upgradelib.php: call to core\task\manager::reset_scheduled_tasks_for_component()
  OUT  * line 1929 of /lib/upgradelib.php: call to upgrade_plugins()
  OUT  * line 489 of /lib/installlib.php: call to upgrade_noncore()
  OUT  * line 469 of /lib/phpunit/classes/util.php: call to install_cli_database()
  OUT  * line 150 of /admin/tool/phpunit/cli/util.php: call to phpunit_util::install_site()
  OUT  ++ Success ++

The same error is shown when initializing the phpunit database instance.

And, maybe for this issue or for another issue, the CI did not pass at all. Check it out at:

https://github.com/ndunand/moodle-tool_mergeusers/actions/runs/4647233869.

When updating your branch, keep an eye on the CI and ensure all tests pass. Tests on master and, of course, for wip-ws-tasks, pass ok. Keep them green.

Thanks a lot,

Jordi

[jpahullo]

For the last @nvallinoto :

When editing with XMLDB Editor, we have to define fields that are foreign keys as such. Moodle will convert them as normal indices, but it helps documenting the relationship between tables. Seen on the Moodle Dev chat.

Thanks a lot!

Jordi

[jpahullo]

I am repeating myself here, but, in general, please:

1. Follow the Moodle coding conventions. Use vscode settings from moodledev.io or use the local_codechecker plugin to do so. On existing files that you modify, you only need to keep the convention for the lines you modify.
2. Unify all terms similar to "merging request"/"merging_request" to "merge request"/"merge_request".
3. Do not use local variables for just one use, without processing, when they are just constants or attributes from a constant. Instead, use better variable and parameter names. For example, not just $record, but $mergerequest.
4. You also need to fix git conflicts expressed by github here.

In the end, I could not find the two classes for the web services. Maybe I am looking at the wrong branch?

Thanks a lot for your great work. Yes, great work. No matter the number of comments. Great work.

[nvallinoto]

Hi Jordi, thank you for your useful remarks. I'm working step by step. First of all I pushed the file concerning the declaration of the two web service following the Moodle standard and checking them using the code_checker plugin. The file names are:

1. classes/external/get_data_merge_requests.php
2. classes/external/enqueue_merge_request.php
3. db/services.php Now I will delete old files from file system and repository.

And then proceed with this request:

2. merge_request.php has name reference to tables. It is not the way to go. They can be placed on every place it is used, or having some kind of a new class like merge_request_factory that interacts with the infrastructure ($DB and so on), loading and storing the necessary data about merge requests. So, concentrating all the database logic

Nicola

[nvallinoto]

Hi Jordi, most of your comments have been resolved and committed. I need a further explanation of this comment or better the code to be added. https://github.com/jpahullo/moodle-tool_mergeusers/pull/242/files/d85e574aa058fc1ccb9a01914a427385a8397d90#r1223527718 I uncommented this line in get_data_merge_request

        $params = self::validate_parameters(self::execute_parameters(),
                                                array('id' => $mergeusers->id));

and executing the web service with postman I receive this error:

"debuginfo": "Missing required key in single structure: mergeusers"

[nvallinoto]

4. You also need to fix git conflicts expressed by github here.

which is the best way to fix git conflicts expressed by gituhb here? git merge or git rebase ?

[jpahullo]

4. You also need to fix git conflicts expressed by github here.

which is the best way to fix git conflicts expressed by gituhb here? git merge or git rebase ?

At some time, I wrote a comment here that I built a branch with name nvallinoto-master in this same repo, with all conflicts being solved. And I suggested you to start from there with new changes. Have you proceed in this way as I suggested? This would be very benefitial for both nowadays. If not, it would be better to repeat conflict solving again from your branch. If you look to the nvallinoto-master branch and identify easily the commits there, maybe you can git cherry-pick there the rest of the commits.

In general, I prefer to make a git rebase, so that you put all your changes on top of the jpahullo/master branch, and solve any conflict appears during the process.

[jpahullo]

Hi Jordi, most of your comments have been resolved and committed. I need a further explanation of this comment or better the code to be added. https://github.com/jpahullo/moodle-tool_mergeusers/pull/242/files/d85e574aa058fc1ccb9a01914a427385a8397d90#r1223527718 I uncommented this line in get_data_merge_request

        $params = self::validate_parameters(self::execute_parameters(),
                                                array('id' => $mergeusers->id));

and executing the web service with postman I receive this error:

"debuginfo": "Missing required key in single structure: mergeusers"

See the comment within the code that I talk about this. It is actually an error on the body of the call to this webservice.

[jpahullo]

@nvallinoto , to keep a progress in the comments, please, if they refer to code, leave it as a comment of the code, so that both can see the thread from start to end. Comments on the issue are hard to follow when having so many comments from code and general comments from the issue. It will help us.

[jpahullo]

And more importanly: Very good job @nvallinoto! Thanks a lot for this contribution we are building.

[nvallinoto]

@nvallinoto , to keep a progress in the comments, please, if they refer to code, leave it as a comment of the code, so that both can see the thread from start to end. Comments on the issue are hard to follow when having so many comments from code and general comments from the issue. It will help us. Ok Jordi. Which is the way to leave a comment of the code ?

[nvallinoto]

4. You also need to fix git conflicts expressed by github here.

which is the best way to fix git conflicts expressed by gituhb here? git merge or git rebase ?

At some time, I wrote a comment here that I built a branch with name nvallinoto-master in this same repo, with all conflicts being solved. And I suggested you to start from there with new changes. Have you proceed in this way as I suggested? This would be very benefitial for both nowadays. If not, it would be better to repeat conflict solving again from your branch. If you look to the nvallinoto-master branch and identify easily the commits there, maybe you can git cherry-pick there the rest of the commits.

In general, I prefer to make a git rebase, so that you put all your changes on top of the jpahullo/master branch, and solve any conflict appears during the process.

My proposal after I finish the updates and commit to this pull request is the following

0) Backup of plugin code on my local hard disk

1) Create a new repository: nvallinoto/moodle-tool_mergeusers_webservice forked from jpahullo/moodle-tool_mergeusers

2) Create a new pull request on jpahullo/moodle-tool_mergeusers (or better on jpahullo:wip-ws-tasks)

3) Git add/commit/push new files and updates existing files on nvallinoto/moodle-tool_mergeusers_webservice

[jpahullo]

4. You also need to fix git conflicts expressed by github here.

which is the best way to fix git conflicts expressed by gituhb here? git merge or git rebase ?

At some time, I wrote a comment here that I built a branch with name nvallinoto-master in this same repo, with all conflicts being solved. And I suggested you to start from there with new changes. Have you proceed in this way as I suggested? This would be very benefitial for both nowadays. If not, it would be better to repeat conflict solving again from your branch. If you look to the nvallinoto-master branch and identify easily the commits there, maybe you can git cherry-pick there the rest of the commits. In general, I prefer to make a git rebase, so that you put all your changes on top of the jpahullo/master branch, and solve any conflict appears during the process.

My proposal after I finish the updates and commit to this pull request is the following

0. Backup of plugin code on my local hard disk
1. Create a new repository: nvallinoto/moodle-tool_mergeusers_webservice forked from jpahullo/moodle-tool_mergeusers
2. Create a new pull request on jpahullo/moodle-tool_mergeusers (or better on jpahullo:wip-ws-tasks)
3. Git add/commit/push new files and updates existing files on nvallinoto/moodle-tool_mergeusers_webservice

This is another approach. Use the method you feel more comfortable.

However, think that you can build a branch in your current repo, pointint the the master branch and then copy there all your files. You do not need to build a new repository. And yes, you will need to build a zip or a recursive copy of all your files that you will use to make your new branch and MR. But, as I said, proceed in the way you feel more comfortable.

Thanks

[jpahullo]

@nvallinoto , to keep a progress in the comments, please, if they refer to code, leave it as a comment of the code, so that both can see the thread from start to end. Comments on the issue are hard to follow when having so many comments from code and general comments from the issue. It will help us. Ok Jordi. Which is the way to leave a comment of the code ?

I think you can click on the changes page, and use the + icon that you should see beside the row number.

In addition, please, do not resolve conversations for your own. I asked them, and it is more difficult to me to know what have you done and I reviewed and what not. The idea is that who requires changes, could confirm that that request was resolved.

So now on, plese, I confirm resolution for every request. Thanks a lot.

I know you want to have it done asap. I do so too.