Execute events trought adhoc task. #54

ojnadjarm · 2024-06-17T20:09:30Z

Added a new setting to enable a feature instead of executing the event handler live, doing it trought an adhoc task.
Added some unit tests.

ak4t0sh · 2024-06-17T21:16:10Z

Hi @ojnadjarm
I remember discussing about this with someone from HQ last year, it's a real pleasure to see this PR landing :)
At first sight it seems OK but I'll try to do a proper review and some tests later.
ping @emmarichardson if you have some time to tests it too

emmarichardson · 2024-07-19T15:24:53Z

@ak4t0sh have you had a chance to look at this?

classes/task/process_event.php

tests/local_autogroup_lib_test.php

ak4t0sh · 2024-07-22T10:42:21Z

Hi @emmarichardson
Again Thanks @ojnadjarm for this PR !
Technically the only "blocker" is the use of a string from an external plugin.

My only "concern" is about the multiple groups generation based on comma separated values.
I think that is a great and must have feature but it could have unwanted behaviour for sites already using autogroups.
Indeed currently if users have groups linked to custom fields containing comma it create groups with comma in their names
If we merge this it would create multiple groups.

For instance "Tester,Developer,Ops"
Currently : 1 groups "Tester,Developer,Administrator"
After merge : 3 groups. "Tester", "Developer" and "Administrator"

So IMHO to prevent this it should be an option (with a default at site level) to define when adding an autogroup into a course.

Also @ojnadjarm could you "clean" your git history please ?
Ideally could you keep only 2 commits - one for the multiple groups on comma separated value feature and the other for the adhoc task - please ?

TIA

emmarichardson · 2024-07-22T11:46:38Z

@ak4t0sh The multiple groups from comma separated values has already been implemented in a previous PR...it has been in use for some time now. So far no-one has complained..

ak4t0sh · 2024-07-22T13:14:18Z

Oh thanks for the reminder @emmarichardson you are right I totally forgot about it.
From what I can see in #49 a new sort_module (local_autogroup\sort_module\user_info_field_multivalue) has been introduced to handle multiple values field which do not cause any side effect for existing installations.
But this PR do not use this system and use an hardcoded comma to create multiple groups in local_autogroup\sort_module\user_info_field which can have side effects for existing autogroup instances.
So IMO this part should be removed as it's already implemented

emmarichardson · 2024-07-22T13:15:49Z

Thank you. That helps. Emma Richardson Distance Learning Coordinator 719-771-4022 Need to visit? Click below to schedule an appointment. https://calendar.app.google/vy9C7FEp2GCNQrbE7

…

On Mon, Jul 22, 2024, 7:14 AM Arnaud Trouvé ***@***.***> wrote: Oh thanks for the reminder @emmarichardson <https://github.com/emmarichardson> you are right I totally forgot about it. From what I can see in #49 <#49> a new sort_module (local_autogroup\sort_module\user_info_field_multivalue) has been introduced to handle multiple values field which do not cause any side effect for existing installations. But this PR do not use this system and use an hardcoded comma to create multiple groups in local_autogroup\sort_module\user_info_field which can have side effects for existing autogroup instances. So IMO this part should be removed as it's already implemented — Reply to this email directly, view it on GitHub <#54 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAP3TGOQ3JDMV5WWS3HMRCDZNUAUDAVCNFSM6AAAAABJOVDNPGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBSHEZTENRYHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ojnadjarm · 2024-07-22T13:47:43Z

Oh thanks for the reminder @emmarichardson you are right I totally forgot about it. From what I can see in #49 a new sort_module (local_autogroup\sort_module\user_info_field_multivalue) has been introduced to handle multiple values field which do not cause any side effect for existing installations. But this PR do not use this system and use an hardcoded comma to create multiple groups in local_autogroup\sort_module\user_info_field which can have side effects for existing autogroup instances. So IMO this part should be removed as it's already implemented

Ok, yep that makes sense, I will leave only the adhoc task part then.

…tiple coaches CRU-119 Documentation and unit test

ojnadjarm · 2024-07-22T17:45:47Z

Hi I have clean up the commitsand the requested changes, let me know what you think :D

classes/event_handler.php

ak4t0sh · 2024-07-29T08:47:39Z

Thanks for the fixes @ojnadjarm ! Almost good to me, could you look at my 2 new feedback please :)

ojnadjarm · 2024-08-23T16:27:30Z

Hi @ak4t0sh Sorry for the late response. Checking out.

ak4t0sh · 2024-09-09T19:32:59Z

Thanks for your patience @ojnadjarm. Code seems good to me.
But you have added an extra commit which as nothing to do with this PR.
Could you remove it please ?

Moreover I have done some quick tests and have some error appearing.

Testing environment

Moodle 4.4
1 course with 2 autogroups : 1 based on profile field and the other one on custom profile field. Tried with multi values too.
Tests done only by editing user profile.

Results

Sync mode (adhoceventhandler disable)

I have the following stack when editing an user profile. (But it works correctly)

Exception encountered in event observer '\local_autogroup\event_handler::create_adhoc_task': This ID number is already in use
    line 235 of /local/autogroup/classes/domain/group.php: call to groups_create_group()
    line 535 of /local/autogroup/classes/domain/autogroup_set.php: call to local_autogroup\domain\group->create()
    line 448 of /local/autogroup/classes/domain/autogroup_set.php: call to local_autogroup\domain\autogroup_set->get_or_create_group_by_idnumber()
    line 138 of /local/autogroup/classes/domain/course.php: call to local_autogroup\domain\autogroup_set->verify_user_group_membership()
    line 159 of /local/autogroup/classes/domain/user.php: call to local_autogroup\domain\course->verify_user_group_membership()
    line 70 of /local/autogroup/classes/usecase/verify_user_group_membership.php: call to local_autogroup\domain\user->verify_user_group_membership()
    line 165 of /local/autogroup/classes/event_handler.php: call to local_autogroup\usecase\verify_user_group_membership->invoke()
    line 347 of /local/autogroup/classes/event_handler.php: call to local_autogroup\event_handler::user_updated()
    line 46 of /local/autogroup/classes/task/process_event.php: call to local_autogroup\event_handler::process_event()
    line 335 of /local/autogroup/classes/event_handler.php: call to local_autogroup\task\process_event->execute()
    line ? of unknownfile: call to local_autogroup\event_handler::create_adhoc_task()
    line 155 of /lib/classes/event/manager.php: call to call_user_func()
    line 75 of /lib/classes/event/manager.php: call to core\event\manager::process_buffers()
    line 795 of /lib/classes/event/base.php: call to core\event\manager::dispatch()
    line 287 of /user/editadvanced.php: call to core\event\base->trigger()

I checked in DB I had a pending adhoc task but non related to this plugin. (moodle notification)

Async mode (adhoceventhandler enable)

After editing an user profile the following stack appear when running cron for the first time after a change on a user profile...and a faildelay is added to the adhoc task. Then it works as expected on the next run.

Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 54
Adhoc task custom data: {"eventname":"\\core\\event\\user_updated","component":"core","action":"updated","target":"user","objecttable":"user","objectid":"3","crud":"u","edulevel":0,"contextid":23,"contextlevel":30,"contextinstanceid":"3","userid":"2","courseid":0,"relateduserid":"3","anonymous":0,"other":null,"timecreated":1725909338}
... started 20:16:15. Current memory use 3.5 MB.
++ Fields list in snapshot record does not match fields list in 'groups'. Record is missing fields: visibility, participation ++
* line 862 of /lib/classes/event/base.php: call to debugging()
* line 239 of /group/lib.php: call to core\event\base->add_record_snapshot()
* line 213 of /local/autogroup/classes/domain/group.php: call to groups_remove_member()
* line 463 of /local/autogroup/classes/domain/autogroup_set.php: call to local_autogroup\domain\group->ensure_user_is_not_member()
* line 138 of /local/autogroup/classes/domain/course.php: call to local_autogroup\domain\autogroup_set->verify_user_group_membership()
* line 159 of /local/autogroup/classes/domain/user.php: call to local_autogroup\domain\course->verify_user_group_membership()
* line 70 of /local/autogroup/classes/usecase/verify_user_group_membership.php: call to local_autogroup\domain\user->verify_user_group_membership()
* line 165 of /local/autogroup/classes/event_handler.php: call to local_autogroup\usecase\verify_user_group_membership->invoke()
* line 347 of /local/autogroup/classes/event_handler.php: call to local_autogroup\event_handler::user_updated()
* line 46 of /local/autogroup/classes/task/process_event.php: call to local_autogroup\event_handler::process_event()
* line 519 of /lib/classes/cron.php: call to local_autogroup\task\process_event->execute()
* line 302 of /lib/classes/cron.php: call to core\cron::run_inner_adhoc_task()
* line 128 of /lib/classes/cron.php: call to core\cron::run_adhoc_tasks()
* line 80 of /admin/cron.php: call to core\cron::run_main_process()
++ Fields list in snapshot record does not match fields list in 'groups'. Record is missing fields: visibility, participation ++
* line 862 of /lib/classes/event/base.php: call to debugging()
* line 239 of /group/lib.php: call to core\event\base->add_record_snapshot()
* line 213 of /local/autogroup/classes/domain/group.php: call to groups_remove_member()
* line 463 of /local/autogroup/classes/domain/autogroup_set.php: call to local_autogroup\domain\group->ensure_user_is_not_member()
* line 138 of /local/autogroup/classes/domain/course.php: call to local_autogroup\domain\autogroup_set->verify_user_group_membership()
* line 159 of /local/autogroup/classes/domain/user.php: call to local_autogroup\domain\course->verify_user_group_membership()
* line 70 of /local/autogroup/classes/usecase/verify_user_group_membership.php: call to local_autogroup\domain\user->verify_user_group_membership()
* line 165 of /local/autogroup/classes/event_handler.php: call to local_autogroup\usecase\verify_user_group_membership->invoke()
* line 347 of /local/autogroup/classes/event_handler.php: call to local_autogroup\event_handler::user_updated()
* line 46 of /local/autogroup/classes/task/process_event.php: call to local_autogroup\event_handler::process_event()
* line 519 of /lib/classes/cron.php: call to local_autogroup\task\process_event->execute()
* line 302 of /lib/classes/cron.php: call to core\cron::run_inner_adhoc_task()
* line 128 of /lib/classes/cron.php: call to core\cron::run_adhoc_tasks()
* line 80 of /admin/cron.php: call to core\cron::run_main_process()
... used 87 dbqueries
... used 0.063717126846313 seconds
Adhoc task failed: local_autogroup\task\process_event,This ID number is already in use
Backtrace:
* line 235 of /local/autogroup/classes/domain/group.php: call to groups_create_group()
* line 535 of /local/autogroup/classes/domain/autogroup_set.php: call to local_autogroup\domain\group->create()
* line 448 of /local/autogroup/classes/domain/autogroup_set.php: call to local_autogroup\domain\autogroup_set->get_or_create_group_by_idnumber()
* line 138 of /local/autogroup/classes/domain/course.php: call to local_autogroup\domain\autogroup_set->verify_user_group_membership()
* line 159 of /local/autogroup/classes/domain/user.php: call to local_autogroup\domain\course->verify_user_group_membership()
* line 70 of /local/autogroup/classes/usecase/verify_user_group_membership.php: call to local_autogroup\domain\user->verify_user_group_membership()
* line 165 of /local/autogroup/classes/event_handler.php: call to local_autogroup\usecase\verify_user_group_membership->invoke()
* line 347 of /local/autogroup/classes/event_handler.php: call to local_autogroup\event_handler::user_updated()
* line 46 of /local/autogroup/classes/task/process_event.php: call to local_autogroup\event_handler::process_event()
* line 519 of /lib/classes/cron.php: call to local_autogroup\task\process_event->execute()
* line 302 of /lib/classes/cron.php: call to core\cron::run_inner_adhoc_task()
* line 128 of /lib/classes/cron.php: call to core\cron::run_adhoc_tasks()
* line 80 of /admin/cron.php: call to core\cron::run_main_process()

Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 55
Adhoc task custom data: {"eventname":"\\core\\event\\group_created","component":"core","action":"created","target":"group","objecttable":"groups","objectid":"11","crud":"c","edulevel":0,"contextid":29,"contextlevel":50,"contextinstanceid":"4","userid":"2","courseid":"4","relateduserid":null,"anonymous":0,"other":null,"timecreated":1725909375}
... started 20:16:15. Current memory use 4.3 MB.
... used 0 dbqueries
... used 0.00033307075500488 seconds
Adhoc task complete: local_autogroup\task\process_event
Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 56
Adhoc task custom data: {"eventname":"\\core\\event\\group_member_added","component":"core","action":"added","target":"group_member","objecttable":"groups","objectid":11,"crud":"c","edulevel":0,"contextid":29,"contextlevel":50,"contextinstanceid":"4","userid":"2","courseid":"4","relateduserid":"3","anonymous":0,"other":{"component":"local_autogroup","itemid":0},"timecreated":1725909375}
... started 20:16:15. Current memory use 4.3 MB.
... used 0 dbqueries
... used 0.00022506713867188 seconds
Adhoc task complete: local_autogroup\task\process_event
Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 57
Adhoc task custom data: {"eventname":"\\core\\event\\group_member_removed","component":"core","action":"removed","target":"group_member","objecttable":"groups","objectid":9,"crud":"d","edulevel":0,"contextid":29,"contextlevel":50,"contextinstanceid":"4","userid":"2","courseid":"4","relateduserid":"3","anonymous":0,"other":null,"timecreated":1725909375}
... started 20:16:15. Current memory use 4.3 MB.
... used 18 dbqueries
... used 0.0087099075317383 seconds
Adhoc task complete: local_autogroup\task\process_event
Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 58
Adhoc task custom data: {"eventname":"\\core\\event\\group_created","component":"core","action":"created","target":"group","objecttable":"groups","objectid":"12","crud":"c","edulevel":0,"contextid":29,"contextlevel":50,"contextinstanceid":"4","userid":"2","courseid":"4","relateduserid":null,"anonymous":0,"other":null,"timecreated":1725909375}
... started 20:16:15. Current memory use 4.3 MB.
... used 0 dbqueries
... used 0.00021100044250488 seconds
Adhoc task complete: local_autogroup\task\process_event
Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 59
Adhoc task custom data: {"eventname":"\\core\\event\\group_member_added","component":"core","action":"added","target":"group_member","objecttable":"groups","objectid":12,"crud":"c","edulevel":0,"contextid":29,"contextlevel":50,"contextinstanceid":"4","userid":"2","courseid":"4","relateduserid":"3","anonymous":0,"other":{"component":"local_autogroup","itemid":0},"timecreated":1725909375}
... started 20:16:15. Current memory use 4.3 MB.
... used 0 dbqueries
... used 0.00019001960754395 seconds
Adhoc task complete: local_autogroup\task\process_event
Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 60
Adhoc task custom data: {"eventname":"\\core\\event\\group_member_removed","component":"core","action":"removed","target":"group_member","objecttable":"groups","objectid":10,"crud":"d","edulevel":0,"contextid":29,"contextlevel":50,"contextinstanceid":"4","userid":"2","courseid":"4","relateduserid":"3","anonymous":0,"other":null,"timecreated":1725909375}
... started 20:16:15. Current memory use 4.3 MB.
... used 18 dbqueries
... used 0.010031938552856 seconds
Adhoc task complete: local_autogroup\task\process_event
Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 61
Adhoc task custom data: {"eventname":"\\core\\event\\group_deleted","component":"core","action":"deleted","target":"group","objecttable":"groups","objectid":9,"crud":"d","edulevel":0,"contextid":29,"contextlevel":50,"contextinstanceid":"4","userid":"2","courseid":"4","relateduserid":null,"anonymous":0,"other":null,"timecreated":1725909375}
... started 20:16:15. Current memory use 4.3 MB.
... used 1 dbqueries
... used 0.00078821182250977 seconds
Adhoc task complete: local_autogroup\task\process_event
Execute adhoc task: local_autogroup\task\process_event
Adhoc task id: 62
Adhoc task custom data: {"eventname":"\\core\\event\\group_deleted","component":"core","action":"deleted","target":"group","objecttable":"groups","objectid":10,"crud":"d","edulevel":0,"contextid":29,"contextlevel":50,"contextinstanceid":"4","userid":"2","courseid":"4","relateduserid":null,"anonymous":0,"other":null,"timecreated":1725909375}
... started 20:16:15. Current memory use 4.3 MB.
... used 1 dbqueries
... used 0.00045418739318848 seconds
Adhoc task complete: local_autogroup\task\process_event

ojnadjarm mentioned this pull request Jun 17, 2024

Added PR request #55

Open

ak4t0sh requested changes Jul 22, 2024

View reviewed changes

classes/task/process_event.php Outdated Show resolved Hide resolved

tests/local_autogroup_lib_test.php Show resolved Hide resolved

CRU-119: Update local registration plugin and groups to allow for mul…

02558f4

…tiple coaches CRU-119 Documentation and unit test

ojnadjarm force-pushed the master branch 2 times, most recently from 08d30a3 to f64cf63 Compare July 22, 2024 17:39

ak4t0sh requested changes Jul 29, 2024

View reviewed changes

classes/event_handler.php Outdated Show resolved Hide resolved

ojnadjarm force-pushed the master branch 2 times, most recently from 727d5cf to b958ad8 Compare August 23, 2024 16:39

DEF-1677: Changin event handler behaviour with adhoc tasks

dd9bcf6

ojnadjarm force-pushed the master branch from b958ad8 to dd9bcf6 Compare August 23, 2024 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execute events trought adhoc task. #54

Execute events trought adhoc task. #54

ojnadjarm commented Jun 17, 2024 •

edited

Loading

ak4t0sh commented Jun 17, 2024

emmarichardson commented Jul 19, 2024

ak4t0sh commented Jul 22, 2024

emmarichardson commented Jul 22, 2024

ak4t0sh commented Jul 22, 2024

emmarichardson commented Jul 22, 2024 via email

ojnadjarm commented Jul 22, 2024

ojnadjarm commented Jul 22, 2024

ak4t0sh commented Jul 29, 2024

ojnadjarm commented Aug 23, 2024

ak4t0sh commented Sep 9, 2024

Execute events trought adhoc task. #54

Are you sure you want to change the base?

Execute events trought adhoc task. #54

Conversation

ojnadjarm commented Jun 17, 2024 • edited Loading

ak4t0sh commented Jun 17, 2024

emmarichardson commented Jul 19, 2024

ak4t0sh commented Jul 22, 2024

emmarichardson commented Jul 22, 2024

ak4t0sh commented Jul 22, 2024

emmarichardson commented Jul 22, 2024 via email

ojnadjarm commented Jul 22, 2024

ojnadjarm commented Jul 22, 2024

ak4t0sh commented Jul 29, 2024

ojnadjarm commented Aug 23, 2024

ak4t0sh commented Sep 9, 2024

Testing environment

Results

Sync mode (adhoceventhandler disable)

Async mode (adhoceventhandler enable)

ojnadjarm commented Jun 17, 2024 •

edited

Loading