search

emmarichardson / local_autogroup

A local plugin for Moodle 2.7 onwards which handles the dynamic creation, population and cleanup of groups on courses.
6 stars 19 forks source link

Execute events trought adhoc task. #54

Open ojnadjarm opened 5 months ago

ojnadjarm commented 5 months ago

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 commented 5 months ago

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 commented 3 months ago

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

ak4t0sh commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 https://github.com/emmarichardson/local_autogroup/pull/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 https://github.com/emmarichardson/local_autogroup/pull/54#issuecomment-2242932689, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAP3TGOQ3JDMV5WWS3HMRCDZNUAUDAVCNFSM6AAAAABJOVDNPGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBSHEZTENRYHE . You are receiving this because you were mentioned.Message ID: @.***>

ojnadjarm commented 3 months ago

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.

ojnadjarm commented 3 months ago

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

ak4t0sh commented 3 months ago

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

ojnadjarm commented 2 months ago

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

ak4t0sh commented 2 months ago

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