jpahullo / moodle-tool_mergeusers

Merge users script for Moodle
https://moodle.org/plugins/view.php?plugin=tool_mergeusers
22 stars 52 forks source link

Find old users that have been merged #283

Open ClausSchmidtPraxis opened 1 month ago

ClausSchmidtPraxis commented 1 month ago

Hi Jordi, thanks for this great tool.

When managing users in Moodle, it can be difficult to identify users that were suspended by the merge users plugin. After succesfully merging several users, I might want to delete the "old users" suspended by the plugin, e.g. to avoid future conflicts and misunderstandings when having ambiguous accounts in Moodle. Therefore this change request is suggested:

In Plugin Settings

Add a [checkbox] enabling this:

Acceptance Criteria:

We are a Moodle Partner providing services for several organisations, and more of them are using your tool. We would like to contribute with this feature, if you would consider accepting this in a Pull Request from us.

Please let me know what you think.

Kind regards Claus

jpahullo commented 3 weeks ago

Hi @ClausSchmidtPraxis,

Sorry for this late answer.

You are very welcome and thanks for your kind words.

About your offer: I mostly maintain this plugin on my spare time and on my work time when there is something critical for our institution.

So, I had to check it firstly with by boss to just know if it can be assumed in our unit and this proposal seems reasonable. We can go on ;-)

Regarding the proposal you stated:

  1. I must admit that I agree with the funcionality you propose understood as: "be easy to delete users being merged and suspended by the plugin". Even better, we can understand the need of "be easy to identify users being merged and suspended by the plugin" so that we can do mass action on them, for instance, on the list of Moodle users.
  2. However, I have to say that I do not fully agree on the aproach. I admit that something needs to be done on the user side to "label" that user as merged and suspended by the plugin. I would prefer not to modify username.

The motivation to my not fully agreement:

We have this plugin in our production site running once every night. I have to say that we often see that user A merges into user B, and then later on, user B merges into user A. It may seem a non-sense, but it happens so often. For instance, it depends on the documentation people present on different units of our institution (a University). A part from this, some of this weird cases were related to human errors introducing the new identification into the corporate systems with some incorrect characters.

I would like to have the funcionality you propose: "be easy to detect users bein merged and suspended by the plugin" so that we could later on delete them if necessary.

Let us explore another approach, if you like:

We have some of our local plugins (not contributed into Moodle Plugins) that on the install.php generates new user custom fields, and we often set them up to be visible only by managers and administrators.

What I propose you is something like:

  1. Only on the install.php and on the specific upgrade step:
    1. Build a category of fields for merge users
    2. Build 2 custom fields.
      1. Date and time of the merging, on the suspended user. Potentially useful for building reports.
      2. Text of the merging you propose, on the suspended user. Human readable information, useful for forensic purposes from managers and administrators.
  2. Build some other event handler so that on every event of merge success:
    1. Empties the 2 fields for the kept user (for cases where A merges into B, and then later B into A).
    2. Updates the 2 fields for the suspended user to fill in the proper detail. it will alwasy show the "last" merge success.

If user deletion in your case may be an action that can be automated, it could also be possible to build a separate scheduled task to manage this, for instance, deleting users after X days/months/years of being suspended by the plugin. And this could be set up as a separate checkbox, disabled by default, so that administrators could think about it calmly.

How to detect that a user is suspended by the plugin: All conditions must apply:

  1. The user A is suspended.
  2. The custom 2 custom fields are not empty for user A.
  3. The custom field of date and time of the merging for user A must be X days/monts/years old from now.

I propose this automation since we have a rule of deleting inactive users, but this rule is very conservative, ensuring that the user was inactive the last 10 years. So, probably, other institutions may have similar automations for deleting users.

On the other hand, there are also the merging events in the logstore_standard_log, but just now I do not identify easily a proposal using just and only the logs (also, thinking about its performance). Also, the size of the logs may differ from institution to institution (every academic year a new database, keeping just last X days of logs, and so.

In summary:

  1. The part of the custom fields would set the base for the proposal to identify users suspended and merged by the plugin.
  2. The scheduled task would be additional, if it matches your needs. Just for you, as an idea.

Let me know your opinion.

Thanks,

Jordi

ClausSchmidtPraxis commented 2 weeks ago

Hi Jordi

I would also like to apologize for my late reply. Thanks a lot for your response, by reading your considerations, I agree with you. We should not alter existing user data in Moodle, especially username! If we add the custom fields you propose, we will also avoid interfering with other functionality in Moodle.

Therefore, would you then consider accepting this implementation if we make a PR?

On install and next upgrade of the plugin, create a category "Merge User Info" with two user custom fields:

  1. [custom field].[merge_date] DateTime field for registering the time of the merging. Potentially useful for building reports.
  2. [custom field].[merge_info] Text field for human readable information about the merging, useful for forensic purposes from managers and administrators.

On successful merge (from user A to user B):

Regarding your proposal on automating deleting of merged and suspended users. We would like to consider that in a future PR.

Regards Claus

jpahullo commented 2 weeks ago

I like your proposal.

I think it is also good to separate things and evaluate automating deletion of users in the future, in a separate issue.

So, in the end, this issue would help to identify merged users, with the detail of the last merging. It is an important detail to be shown in the custom fields.

Thanks,

Jordi

ClausSchmidtPraxis commented 2 weeks ago

I will notify our developers, that they can start working on it. We will get back with a pull request :-)

Claus

jpahullo commented 2 weeks ago

Hi @ClausSchmidtPraxis ,

Thanks for the feedback.

I am sharing with you some tips that you should consider while depeloping this part:

  1. The category of custom fields, and the 2 custom fields, should be by default ONLY visible by managers and admins. This is an information that should not be visible by end users, by default.
  2. The content of the custom field with the explanation should be a customizable string. It would be good to list in some part of the Moodle (at least on the README of the plugin), which fields are available within the string.
  3. It would be a good idea to include in the explanation of which was the user being kept, to include the link up to see the log of the merging operation in particular. As a forensic role, will help reduce time to get into it so quickly.

Thinking on it a bit, maybe, one easy way to fill in custom fields for user being kept and merged, would be implementing a event observer for the success event. It will decouple the merging operation with the filling operation afterwards, being easier to maintain. Just as an idea.

Thanks for the proposal and work,

Jordi