306 remove profilefields

[matthewhilton]

Changes

Remove profile fields and use profile hooks instead

Closes #306

As per discussion here #311 (comment) , it is much simpler and better idea to just expose the merging data via a profile hook instead of profile fields.

I decided to not remove the custom fields that were created previously for both simplicity and also reducing risk. Site owners can delete them manually if they wish.

I also did not include the log in the profile field. This seemed unnecessary and would pollute the profile page - platform owners can look up the logs easily directly in the database if they wish.

Switch CI

Closes #311

I've switched the main CI to reusable workflows and disabled the checks necessary to get it green. Ideally over time these checks should be re-added and the code fixed up, just not in this PR.

I left the release workflow to be configured later, see https://github.com/catalyst/catalyst-moodle-workflows?tab=readme-ov-file#how-does-this-automate-releases

Move logger class to correct location

This was part of #68 - moved this class to the proper location per https://moodledev.io/general/development/policies/codingstyle#namespaces so it could be autoloaded.

[jpahullo]

Hi @matthewhilton !

I have just read your comment on the PR. Pending to review de code. Meanwhile, I let the CI run to check the results of the included tests.

As a first impression on the profile hoook shown on the screenshots (I repeat, without looking at contributed code):

1. If the goal of the profile hook is to show the last merge operation where the user participated, there is no need to show the case None.
2. I think it could be confusing if we show both operation if the profile hook show the last merge operation, either when this user was the user toid or fromid.
3. I, instead, put the link to the related merge log.

Think about this profile hook as a need for forensic analysis from support staff. If they need to look for the merge log, they should go to the list of logs and look for the current user. And it may not be the last merge log.

I need to think about deleting custom profile fields or not. It can be a later version, with the necessary release comments to explain the importancy on that upgrade.

Above all, a huge thanks contributing all this work.

All the best,

Jordi

[matthewhilton]

Hi @jpahullo

Thanks for the feedback - it's always good to get another persons perspective 😃

I've added the log link to the profile hook so the logs can be viewed directly without checking the DB.

I think showing None when a user has no previous merge instead of just not showing anything at all is clearer, as it conveys to the viewer specifically that this user has not been merged into/out of.

Also I think that showing merges into this user and merges out of this user separately is better, as it can help convey the direction (i.e. to or from), and also will show where there is a chain merge (e.g. User A merged into User B, User B merged into User C - User B would show both User A and User C in their profile).

These are just my perspectives but I don't mind if you prefer something different, the differences are fairly trivial.

I'm also not really intending on adding anything new here; but am instead just recreating the previous customfield functionality so that customfields can be removed and core unit tests are passing again.

Let me know your thoughts

[jpahullo]

Hi @matthewhilton ,

Thanks a lot for your contribution. I like a lot the way you worked it out.

There are, however, some points that I think should be addressed before continuing on this.

I let the CI run to let you detect new things to review.

Thanks a lot again. Good job.

Jordi

[jpahullo]

I have read your last PR and it seems good to me. Let us pass the CI to check if anything is kept ok.

Thanks a lot for your work.

[jpahullo]

One question @matthewhilton :

I can see CI is passing on green. Congrats for it.

I would like to ask you how it should be make to pass tests agains PHP 8.1 that is also fully supported on Moodle 4.1 (at least starting from a certain minor version if I remember correctly).

I can see that there is a job that calculates the matrix of tests. I would like to be sure that it passes too for PHP 8.1. Is it possible with the Catalyst CI?

Thanks a lot,

Jordi

[matthewhilton]

The php versions it tests are defined here https://github.com/catalyst/catalyst-moodle-workflows/blob/main/.github/actions/matrix/matrix_includes.yml

It only uses the major version and not the minor version number, which I suspect is the reason that it does not include PHP 8.1 testing for 4.1 since 4.1.0 and 4.1.1 technically don't support it (per https://moodledev.io/general/development/policies/php)

[jpahullo]

Thanks @matthewhilton !

I already have seen this CI from Catalyst on other plugins and I liked it a lot. Thanks for adding it here.

With this CI from Catalyst, there would be some possibility to add matrix cases manually? I can see you disabled some options for the CI: maybe there is some other option to add cases into the matrix.

Let me know.

Either way, I would need to test your PR on my local Moodle instance, with a huge table merge users log.

The following week is not workable at our institution, so there would be some news for the other week.

A huge thanks for the work and love put on this PR.

Jordi

[danowar2k]

I extracted an issue to display the last merge info on the user profile page without profile fields (but without removing/hiding those for now). Would it be possible to extract that part of the PR into a new one?

I haven't looked at the code thoroughly yet, and I don't know whether @jpahullo agrees...maybe I'll just look at the commit for the profile display first :-D (I tend to compartmentalize stuff, your PR at least has separate commits for everything it tries to solve)

[danowar2k]

Okay, I looked over the commit. That commit not only displays the info in the profile via myprofile lib.php but also removes all the stuff about custom user profile fields.

I'm currently trying to do a step by step migration from custom user profile fields again using a few newly created issues (#321, #322, #323).
#321 corresponds to displaying the info using the myprofile lib.php hook, but intentionally doesn't remove the custom user profile fields yet, because there is a wider problem caused by multiple viewpoints on the issue of "display and use merge information for further work". If this PR is merged, then the people from #283 would again lose their way of working.

I think it would be much easier to split the profile display into another PR without removing anything from the custom user profile fields for now. This way we have a period where both things still work.

When the people from #283 agree on the implementation of #322 and that one is done, we could then remove the fields again in #323.

What do you think?

[matthewhilton]

Reading the original issue #283 it seems they had two requirements:

• Have merge data for building reports -> already fulfilled by existing DB tables
• Make it easier to identify users merged -> replaced customfields with profile hook

Although I'm not opposed to it, I don't really see much value in migrating the fields - given this PR introduces an equivalent way to fulfill the requirements and noting the issues the current customfield implementation causes.

[danowar2k]

Oh we're doing the same thing. It just seems to me that doing it step by step will bring earlier results by first giving the people from #283 what they need and then removing the custom fields again.

Basically, I'm trying to speed up their removal by splitting it in multiple logical units to make peer review and acceptance go by faster.

I'd idealistically expect to get my three PR's through on a day ;-) (Of course, the maintainer has to have time to do the reviews and merging, of course, which seems to be at a premium)

[jpahullo]

Thanks for working on this.

We at our institution are a bit busy these weeks, preparing next course 2025-26.

So, I will check this and test it not as soon as I would like.

I think this is a good work to be included into the plugin.

Thanks a lot to all involved on this.

Jordi