search

LafColITS / moodle-local_ldap

Various synchronization scripts between Moodle and LDAP directories (see https://tracker.moodle.org/browse/MDL-25011 )
9 stars 7 forks source link

Cohorts not fully populating when AD sub-group has 0 members #29

Closed jtsafran closed 7 months ago

jtsafran commented 3 years ago

Looking at this line, https://github.com/LafColITS/moodle-local_ldap/blob/main/locallib.php#L395, the code fully exits and returns a false value if no members of a group are returned. Changing that to a continue statement fixes the issue as it returns the code to start looking at the next sub-group and a 0 member group is not an error.

mackensen commented 3 years ago

@jtsafran good catch, interesting. I think I want to make the test more explicit as it could return false on a legitimate failure.

jtsafran commented 2 years ago

@mackensen any updates on this one for long term fix?

mackensen commented 2 years ago

@jtsafran I got stuck because I couldn't find a way to craft a unit test for the fix, because the PHP LDAP library wouldn't let me create an empty group. I haven't had a chance to circle back.

jtsafran commented 2 years ago

@mackensen I missed your reply here! Sorry about that!

Would a real life test help? I have a site that this fix works on, but not sure what you would need/want for verification?

mackensen commented 2 years ago

@jtsafran I'm happy to take your word for it. I've just refactored the branch to remove the tests; all that's left is the four places in locallib.php with the explicit comparison.

jtsafran commented 2 years ago

OK. I was just changing the false return to a continue, so let me get this in place on the same site and verify all looks good before being merged into master! Give me a couple of days!

Thanks!

mackensen commented 1 year ago

@jtsafran checking back in on this, I'm getting ready to release a new version ahead of Moodle 4.1 and I'd like to include this.

jtsafran commented 1 year ago

@mackensen sorry for the long delay here. The change that was added helps in that we saw fewer users get dropped/ignored, but there were still users who were not added. I did some digging, but was not able to find out why.

Changing the previous line still works for those following along, which is now on line https://github.com/LafColITS/moodle-local_ldap/blob/main/locallib.php#L377

mackensen commented 1 year ago

@jtsafran this may be resolved in the new release (v3.11.1).

AdamosD7 commented 7 months ago

Hello, has this problem moved on? If I have one empty child group in the main group, the main group still doesn't sync properly. Can anyone help me with it?