Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

new Group-Member association attribute (zimbraMailForwardingAddress) #16737

Merged
merged 1 commit into from
Aug 7, 2020
Merged

new Group-Member association attribute (zimbraMailForwardingAddress) #16737

merged 1 commit into from
Aug 7, 2020

Conversation

tofuSCHNITZEL
Copy link
Contributor

added "zimbraMailForwardingAddress" as a Group-Member association attribute to enable the use of Zimbra Distribution Lists as groups in nextcloud when connecting to a zimbra LDAP

@tofuSCHNITZEL tofuSCHNITZEL mentioned this pull request Aug 13, 2019
Closed
@georgehrke georgehrke requested a review from blizzz August 14, 2019 07:41
@skjnldsv skjnldsv added 3. to review Waiting for reviews feature: ldap labels Sep 28, 2019
@skjnldsv
Copy link
Member

Bump :)

@skjnldsv skjnldsv added this to the Nextcloud 18 milestone Sep 28, 2019
@blizzz
Copy link
Member

blizzz commented Oct 11, 2019

the group backend is already quite complex and this is growing more with this addition. That is, I have nothing against it per se, but it would be cool if we could split it up a bit. So, for instance all the association-attriubte-specific code could go into specialized classes, adaptor like. And we just load whatever we need. We can have smaller units with less conditions and code paths. Easier to test and understand.

@tofuSCHNITZEL
Copy link
Contributor Author

So, for instance all the association-attriubte-specific code could go into specialized classes, adaptor like.

this is unfortunately beyond the scope of my time and abilities.
but if someone could start the refactor I could implement this feature with the new pattern. (because then I have reference on how it should be organised)

This was referenced Dec 11, 2019
Merged
Merged
@rullzer rullzer mentioned this pull request Dec 19, 2019
Merged
18 tasks
@rullzer rullzer removed this from the Nextcloud 18 milestone Dec 23, 2019
@tofuSCHNITZEL
Copy link
Contributor Author

sorry I didnt get notified about the potential merge into 18beta. I updated the branch to be up to date with the master.. any chance for v19?

@blizzz
Copy link
Member

blizzz commented May 5, 2020

sorry I didnt get notified about the potential merge into 18beta. I updated the branch to be up to date with the master.. any chance for v19?

We're now in Beta, which essentially also is a feature freeze. 20 then.

@blizzz blizzz added this to the Nextcloud 20 milestone May 5, 2020
@blizzz
Copy link
Member

blizzz commented Jun 16, 2020

Tests are failing. Perhaps related to the submodule changes? Please take them out.

@tofuSCHNITZEL
Copy link
Contributor Author

tofuSCHNITZEL commented Jun 16, 2020 •
edited
Loading

Tests are failing. Perhaps related to the submodule changes? Please take them out.

I think I fixed it somehow.

@tofuSCHNITZEL tofuSCHNITZEL force-pushed the feature-zimbraldap branch 2 times, most recently from 98888bc to b64a0c7 Compare June 16, 2020 23:59
blizzz
blizzz approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by catch, looks good otherwise. Only, please sign your commits https://github.com/nextcloud/server/pull/16737/checks?check_run_id=778568009 :)

@tofuSCHNITZEL
Copy link
Contributor Author

sorry, I missed the sign-off on some older commits... I tried the suggested rebase command but it just blows everything up - not beeing able to resolve conflicts in files I never touched... Is it acceptable if they stay unsigned? or could you help me with the rebase?

@MorrisJobke MorrisJobke force-pushed the feature-zimbraldap branch from de7a918 to bd37b4c Compare July 6, 2020 10:21
@MorrisJobke
Copy link
Member

sorry, I missed the sign-off on some older commits... I tried the suggested rebase command but it just blows everything up - not beeing able to resolve conflicts in files I never touched... Is it acceptable if they stay unsigned? or could you help me with the rebase?

That was due to the merge commits. I rebased the whole branch here and squashed everything into one commit. I also resolved some issues that arose from the updated code in the master branch. I noticed that you deleted one block during one of your merges of master. I added it here again. If this is not needed anymore we could delete it again.

Beside that all your commits already contained the sign-off messages.

MorrisJobke
MorrisJobke reviewed Jul 6, 2020
View reviewed changes
apps/user_ldap/lib/Group_LDAP.php Outdated
// DNs are returned as keys; this is probably only the case if
// nested groups are used and/or group member attributes are DNs
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block went missing by one of the merges from master. I don't know if this was intentional or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm I have no idea why or when this was removed... it was still present in 18.0.4. unfortunately I cant really test since this is only called when non LDAP backend is used (and I dont have a test setup for that) also I'm not even sure when the inGroup function is called....

Copy link
Member

@blizzz blizzz Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC it was a bugfix: 1d48c03#diff-994e1d45c127dc64e57744e2bf2eff3d

@MorrisJobke
Copy link
Member

Please give this a good try and then it is ready for merge IMO.

@MorrisJobke
Copy link
Member

Conflicts because of recently merged #21171 ... let me rebase again :)

@MorrisJobke MorrisJobke force-pushed the feature-zimbraldap branch from bd37b4c to de799e6 Compare July 6, 2020 12:03
@tofuSCHNITZEL
Copy link
Contributor Author

Please give this a good try and then it is ready for merge IMO.

my local test install seems fine.

MorrisJobke
MorrisJobke approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good 👍

@MorrisJobke MorrisJobke force-pushed the feature-zimbraldap branch from de799e6 to aee36c4 Compare July 6, 2020 21:22
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 6, 2020
@MorrisJobke
Copy link
Member

I also fixed the code style and this is now ready to go in.

@MorrisJobke MorrisJobke mentioned this pull request Jul 6, 2020
Merged
@MorrisJobke MorrisJobke force-pushed the feature-zimbraldap branch from aee36c4 to b583b61 Compare July 7, 2020 07:39
@MorrisJobke
Copy link
Member

LDAP integration tests don't like it. @blizzz to the rescue

@blizzz
Copy link
Member

blizzz commented Aug 7, 2020

LDAP integration tests don't like it. @blizzz to the rescue

It seems it is just "Notice: Undefined index: label in /drone/src/build/integration/features/bootstrap/Sharing.php line 740"? Let me rebase once more.

@blizzz blizzz force-pushed the feature-zimbraldap branch from b583b61 to ad31493 Compare August 7, 2020 14:04
@MorrisJobke
Copy link
Member

LDAP integration tests don't like it. @blizzz to the rescue

There are still some that fail.

@blizzz
Copy link
Member

blizzz commented Aug 7, 2020 •
edited
Loading

now they are running through :) It was the block as mentioned ^ let me squash, then merge

@tofuSCHNITZEL @blizzz
added "zimbraMailForwardingAddress" as a Group-Member association att…
551d904 
…ribute to enable the use of Zimbra Distribution Lists as groups in nextcloud when connecting to a zimbra LDAP

Signed-off-by: Tobias Perschon <tobias@perschon.at>

fix cs:check

Signed-off-by: Tobias Perschon <tobias@perschon.at>

Update apps/user_ldap/lib/Group_LDAP.php

Co-authored-by: blizzz <blizzz@arthur-schiwon.de>
Signed-off-by: Tobias Perschon <tobias@perschon.at>
@blizzz blizzz force-pushed the feature-zimbraldap branch from 42cbe26 to 551d904 Compare August 7, 2020 21:31
@blizzz blizzz merged commit 579c707 into nextcloud:master Aug 7, 2020
@welcome
Copy link

welcome bot commented Aug 7, 2020

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@tofuSCHNITZEL
Copy link
Contributor Author

Thanks for all your help!

@blizzz blizzz mentioned this pull request Aug 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blizzz blizzz blizzz approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: ldap
Projects
None yet
Milestone
Nextcloud 20
Development

Successfully merging this pull request may close these issues.
5 participants
@tofuSCHNITZEL @skjnldsv @blizzz @MorrisJobke @rullzer