jpahullo / moodle-tool_mergeusers

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

Search by custom user profile fields #231

Open Tsheke opened 1 year ago

Tsheke commented 1 year ago

Hi @ndunand ,

Would you like to merge this request please? We added support to make search by custom user profile fields:

How to test the new fonctionnality:

Thank you.

Tsheke commented 1 year ago

mergeusers form screenshots

jpahullo commented 1 year ago

Thanks @Tsheke for your contribution.

It seems quite good.

However, before integrating your PR into this plugin, please, consider the following comments and ammendments suggestions:

  1. index_form.php and profile fields: user_filter_profilefield::get_profile_fields() always return a first element named 0 => 'any field'. This item is also present on the plugin settings. So:
    1. When setting any field in the plugin settings, index_form.php should consider any existing profile field.
    2. If user_filter_profilefield::get_profile_fields() only returns an array of one element 0 => 'any field', this should be considered as additional profile field is not available. settings.php should show only None and nothing else.
    3. If user_filter_profilefield::get_profile_fields() returns more than one element, and in settings.php is selected any field (value 0), index_form.php should gather all real profile fields from user_filter_profilefield::get_profile_fields() (excluding 0 => 'any field' as if admin selected all existing profile fields from the list.
    4. index_form.php:77: $advanced is undefined. I suppose you meant to be true.
  2. Since you have modified settings.php, you need to update version.php to bump up its version. Consider only the version on the current master branch. I had to do this when testing locally.
  3. Syntax have to be revisited. Please, use tools like local_codechecher Moodle plugin to validate that your code follows Moodle code conventions. At least, your modified parts of the document. Please, do no alter parts of a document just for syntax correction. Just your own parts, the parts you contribute in this PR. There are so many bits to fix.
  4. Revisit code twice again, to fix any problematic item. For instance, there is some comment that seems to comment out a pre-existing line, like this.

In addition, I think we could improve your proposal by adding any allowed user profile field to be added into the existing initial selector, as with the rest Idnumber, Email address and so on. The name could be of the form profile_field_3, visible name Custom profile 1 and value search pattern, when looking for the user profile field with id = 3 (i.e., having profile_field_ as prefix always, and 3 or any given number, as the field id). Then, when submitting the form, we would look into the pattern profile_field_ and if match we will take the given number as the field.id being search by.

This way, this part would not be hidden as an advanced item, but as a main part, included into the existing selector for id, idnumber, email and so on. I think it would be very benefitial for admin users indeed.

Let me know if you can afford this change.

Thanks for your contribution,

Jordi

jpahullo commented 1 year ago

By the way,

Please, @Tsheke, add some PHPUnit tests to validate your new part. Thanks!

Jordi

Tsheke commented 1 year ago

Hi @jpahullo , Thanks you for the feedback. I made changes you suggested. Some variables was renammed for validation with the codechecker. This is mainly removing Camel Style and/or underscrore to have all in lower case. For the last improvement you suggeted, it's a good idea, but we can check it later when we get some free time. I could add the PHP unit test later too.

Thank you so much and let me know what you think about the pull request. Johnny.

jpahullo commented 1 year ago

Once you @Tsheke make the requested changes, we will run the workflow to make the latest checkings.

Please, add tests for this profile fields part too.

Many thanks for the work!

Tsheke commented 1 year ago

Hi @jpahullo , Here are changes you requested + php unit test. Would you like to have look please? Thank you.

jpahullo commented 11 months ago

Hi @Tsheke,

Thanks for the effort.

It seems quite good right now. You do not considered a comment about the settings.php file though.

In addition, I have to test it locally and validate the whole solution. I will be back once I can with more information.

Thanks again,

Jordi