make this plugin more robust against moodle-bugs: make an option to sync cohorts every night

RowhamD commented 2 years ago

This is not really a bug-report for this plugin, but a proposal for a more robust design, and maybe an explanation for #38 #40 #30 #29 #32 . The design of this plugin, when to update cohorts, should work when the rest of Moodle, around this plugin, works as expected. But in real life Moodle or other plugins will have bugs and this could lead to incoherent cohorts, which will stay incoherent forever.

our Setup

BelWü-hosted Moodle 4.0.2 with php-caching
v4.0-r1 of this plugin

my story behind

Until a few weeks ago I used the "smartcohort"-plugin to fill my cohorts automatically. As this plugin does not work with Moodle 4 anymore, I switched to this plugin. While testing and experimenting with some test-cohorts and -rules I experienced, that, on our installation, this plugin did not work as described in the readme.

Together with the BelWü-support we found out that we are faced with (another) Moodle4-caching-problem:

upon an change in a rule, this plugin sets the "updatecohort"-config-var with "set_config" here: https://github.com/moodle-an-hochschulen/moodle-local_profilecohort/blob/0143ff80aff7ccf28f1198d04ace3400502de80d/classes/profilecohort.php#L283
in the underlying sql-database I can immediately observe that in the table "mdl_config_plugins", in the line with "plugin=local_profilecohort name=updatecohorts" the field "value" changes from "0" to "1".
the update_cohorts-task of this plugin, called by cron, checks this config-var with get_config here: https://github.com/moodle-an-hochschulen/moodle-local_profilecohort/blob/0143ff80aff7ccf28f1198d04ace3400502de80d/classes/task/update_cohorts.php#L50
...and the reproducible observation is, that, within our setup mentioned above, cohorts will never be updated and the "updatecohorts"-field in the database will never change to "0", so this if-condition must have been false

It turned out that clearing the (php-)caches solves this problem for at least the next run of the update_cohorts-task, maybe even for some hours. But at least on the next day updating cohorts upon a changed rule wont work anymore.

So the conclusion is: set_config / get_config is broken in Moodle 4.0.2 when using (php-)caching on your server. Maybe the real problem is within set_config, maybe this method writes to the database, but does not update the content of the cache correctly.

The temporary workaround from the BelWü-support is to not use get_config, but to access the database directly:

    public function execute() {
        global $DB;
        $record = $DB->get_record("config_plugins", Array("plugin" => "local_profilecohort", "name" => "updatecohorts"), $fields = "value");
        if ($record->value == "1") {
        // if (get_config('local_profilecohort', 'updatecohorts')) {
            $manager = new profilecohort();
            $manager->update_all_cohorts_from_rules();
            set_config('updatecohorts', '0', 'local_profilecohort');
        }

...which makes "update the cohorts on the change of a rule" work again.

But, I experienced another problem:

if I make a change to a Moodle-user "by hand", through the admin-interface, this change reaches the cohort-mapping. (In this case the update_cohorts-task isn't involved, this is made with the \core\event\user_updated -event)
if the Ldap-plugin makes a change to a Moodle-user, because of a change in the external user-database, this change would not reach the cohorts. Maybe this is "just" another caching-problem or maybe the Ldap-plugin does not call \core\event\user_updated correctly.

proposal

As said above I didn't consider this as a real bug of this plugin, as using set_config, get_config and \core\event\user_loggedin, \core\event\user_loggedinas, \core\event\user_created and \core\event\user_updated events is recommended in the developper-docs, e.g. here: https://docs.moodle.org/dev/Developer_FAQ#How_do_I_get.2Fset_configuration_settings.3F

But in the real wild things often do not work as they should. The Moodle-core itself or other plugins will have bugs.

The design of this plugin is in a way that it relies on the correct work of the code around it. There is no mechanism correcting incoherent cohorts, regardless why they got incoherent.

So, how about

a configurable option for the update_cohorts-task to make him call update_all_cohorts_from_rules() every night?
or, alternatively offer a second task, which could be as simple as a copy of the update_cohorts-task without the if-branch
```
public function execute() {
        $manager = new profilecohort();
        $manager->update_all_cohorts_from_rules();
        set_config('updatecohorts', '0', 'local_profilecohort');
    }
```
The original task could be called from cron e.g. every 15min, but is resource-friendly watching the updatecohorts-config-var. The second task could be called every 12 or 24 hours syncing all cohorts and correcting everything which went wrong before.

[As this is my first post here: Many thanks for this plugin and all the work with it!]

VFXpro commented 2 years ago

".... The second task could be called every 12 or 24 hours syncing all cohorts and correcting everything which went wrong before..." This would be SO USEFUL!!

abias commented 1 year ago

Hi @RowhamD ,

many thanks for taking the time to write this proposal and for giving insights into your work and challenges.

Currently, this plugin is provided "as-is" and there aren't any ressources scheduled for larger additions or changes. However, as soon as we are able to get our hands dirty with this plugin again, your proposal will be on the list of issues to be considered.

Thanks, Alex

emmarichardson commented 1 year ago

Thank you for this solution - it was driving me crazy as my ldap users were only updating when they logged in. For anyone else that is just going to hack the code to make it run, make sure that you move the last curly bracket on the commented out line down to the next line.

moodle-an-hochschulen / moodle-local_profilecohort