[12.0][MIG] users_ldap_groups

[alexey-pelykh]

No description provided.

[alexey-pelykh]

@hbrunn if you don't mind, please take a look

[alexey-pelykh]

Frankly speaking, not much code left :|

[alexey-pelykh]

@pedrobaeza if you don't mind, I'll bump this to your attention

[pedrobaeza]

Well, I can't be very helpful here as I don't use this module and don't know about this area. What I can say is about the guidelines, as this module contains plurals, being dragged since long, but I won't force to change it if contributors don't want.

[alexey-pelykh]

Frankly speaking, I'm thinking of making a bit revised (an more UX-friendly) one as an alternative

[pedrobaeza]

Looking at the description, auth_ldap_attribute_sync can't be adapted for this purpose also?

[alexey-pelykh]

It may, and actually they do share some code. I think that's up to reviewers if to keep modules lean-and-mean or to make complex ones. Why auth_ldap_attribute_sync appeared w/o groups - just because I had to start one-step-at-a-time.

[pedrobaeza]

@hbrunn you were the original author of this. What do you think?

[hbrunn]

I think it's great that @alexey-pelykh does all the efforts, thanks for that. Group stuff is very different from attributes like whatever is not a group, so I think this surely merits its own module.
I don't think being the original author of something entitles me of anything. So thanks for the consideration, but let's just discuss

[alexey-pelykh]

At Brainbean Apps, all our role management is based on LDAP groups, which are matched to groups in Odoo. And takin into account that usually one LDAP group has multiple permissions (groups in Odoo), the resulting table in users_ldap_groups is very long and very hard maintain, e.g.
and there are 17 other lines like that, and I've mapped only like 20% of what's needed. So there are 2 suggestion where to improve:

1. UX: It seems to me that one LDAP group can safely be granted 1+ Odoo group via many2many - yet I haven't evaluated how to do that
2. Feature: add contains-regexp as operation
3. Somehow allow to strip ,cn=groups,cn=accounts,dc=bbapps,dc=org from every line

[pedrobaeza]

Can we rename this at least to auth_ldap_group_sync or similar if it can't be added to the other module?

[alexey-pelykh]

Indeed was issue with the code and with tests

[alexey-pelykh]

Decided to keep the original behaviour, thus changed the PR

[alexey-pelykh]

Good people of 10.0 PR (@sebalix @NL66278), I summon you in order to request a review of this PR as well, since you're aware of the topic 🔥

[nikul-serpentcs]

Minor change Otherwise LGTM

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[oca-clabot]

Hey @alexey-pelykh, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

• Sandy Carter sandy.carter@savoirfairelinux.com (no github login found)

Appreciation of efforts,
OCA CLAbot

[hbrunn]

@alexey-pelykh thanks a lot for this.

I would advise you to ping people as little as possible, we're all doing our best in reviews. In the best case, your ping doesn't create more time for people and is basically a no-op, in the worst case it creates noise.
I for example have a sieve rule that marks mails where I'm mentioned with the Imap flag important, because it's often people asking for help about technical stuff. So if people @mention me, that goes to the look-at-it-at-first-convenience folder. If I then see the same person pinging me repeatedly for reviews that are not on fire, I'll take their ping less seriously, with the extreme of ignoring them.
So I'd advise to just trust people will eventually come back for review, after a round of fixes a message in the veins of 'all done' creates a new (and for people like me less intrusive) notification, that seems fine to me. A reminder after a week of two seems fine to me too.