Setting preservemanual keeps track of every group membership of all courses

[Kemmotar83]

Hi,

I'm not sure if this is a bug or a choice.

If "preservemanual" setting is set (by default) Autoenrol keeps track of every group membership even in courses where Autogroup has not a set enabled. In large sites, where Autogroup is used in small amount of courses, this behavior insert a lot of useless records in the local_autogroup_manual table.

Since this plugin should not work on other groups, my understanding was that "preservemanual" setting should keep track only of users manually added to groups created and managed by autogroup.

I'm missing something?

Thanks, Giorgio

[emmarichardson]

Do you add the plugin to every course though? Even if it is not enabled?

[Kemmotar83]

Hi Emma, the plugin is enabled and has the default "General configuration" settings. In every course there's the Auto Group menu item but only few courses have a set configured.

I've made a bit of code analysis and records are inserted in "local_autogroup_manual" table by default when "group_member_added" event is triggered, even if "preservemanual" setting is disable. This setting is used only when autogroup is synchronizing memberships.

Am I understanding this right?

Regards, Giorgio

[emmarichardson]

Honestly, I am not sure. It has been a long time since I had Mark Ward write this plugin for me. I will take a look at the code and my implementations and see what I can figure out (bearing in mind that I am not actually a code writer). If you have a more elegant solution and want to put in a pull request, I am happy to test and incorporate if I do not see any negative outcomes. For example, if the setting is off and then turned on, if the records are not there, how would it know which groups should be kept...

[Kemmotar83]

Hi,

I'm an admin and a developer. I'll look into the code more deeply and eventually I'll propose a solution.

"if the records are not there, how would it know which groups should be kept..." If the plugin is not activated or no set is defined in a course, you should not bother of group memberships. Generally, you should be interested only of groups created by autogroup, identified by a specific id number (autogroup|##|...).

Giorgio

[emmarichardson]

That makes sense. Thanks for looking at this for me!

[ak4t0sh]

Indeed a check using \local_autogroup\domain\group::is_valid_autogroup should be done before the insert at https://github.com/emmarichardson/local_autogroup/blob/6b2a10cc130dcd77a96714e5e4b8620f12e610da/classes/event_handler.php#L88 I currently have no time to work on it but do not hesitate to open a pull request I'll try to review your changes.

[emmarichardson]

Arnoud! Thanks for being willing to review - I was going to ping you to see if you were around. I have looked at code (just checking for anything glaring that an untrained eye might see as an issue!!) and plan to test on dev site.

[Kemmotar83]

Hi @ak4t0sh and @emmarichardson ,

it should be an easy fix.

The "problem" is how to clean unnecessary saved data. In upgrade.php we need specific upgrade code that delete data that doesn't belong to autogroup.

I'll work on a fix (branching from the refactored code).

[Kemmotar83]

Hi, while I'm waiting for your review of my other pull requests, I'm working on this issue. It's a really simple fix but requires a version update, in order to clean _local_autogroupmanual table from unnecessary rows.

[ak4t0sh]

you can use current stable version with decimal notation : 2022062500.xx for example 2022062500.01

[Kemmotar83]

Thanks!

[emmarichardson]

After releasing this on the plugins database, I have a PostSQL person that is having issues with the upgrade - could you take a look at that ticket and see if maybe you can figure out what the issue is? I use MySQL and Maria and cannot replicate...

[Kemmotar83]

Hi @emmarichardson ,

in file db/upgrade.php (row: 94), the sql should start like "DELETE FROM ..." and not like "DELETE LAM FROM...". My bad, that is a trivial error that MySQL apparently ignore.

Do you need a pull request or you can fix it by yourself?

Thank you and sorry again for the error.

[ak4t0sh]

I am closing this issue in favor of #44