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

Improve ldap avatar handling #50162

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Improve ldap avatar handling #50162

wants to merge 6 commits into from
+44 −193

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Jan 13, 2025 •
edited
Loading

  • Resolves: #

Summary

  • Improve typing in user_ldap User class
  • Remove obsolete limitation of only updating avatar after login

Checklist

@come-nc come-nc added the 3. to review Waiting for reviews label Jan 13, 2025
@come-nc come-nc added this to the Nextcloud 31 milestone Jan 13, 2025
@come-nc come-nc self-assigned this Jan 13, 2025
@come-nc come-nc requested a review from blizzz January 13, 2025 17:27
@come-nc come-nc requested review from a team, ArtificialOwl, artonge and yemkareems and removed request for a team January 13, 2025 17:27
blizzz
blizzz approved these changes Jan 13, 2025
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.

A question, code looks good.

apps/user_ldap/lib/User/User.php
// external mounts are mounted properly (e.g. with login
// credentials from the session).
Util::connectHook('OC_User', 'post_login', $this, 'updateAvatarPostLogin');
$this->updateAvatar();
Copy link
Member

Choose a reason for hiding this comment

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

just to be sure, whether you tested this with a clean user, who has not logged in before, and has an avatar set on LDAP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested with the check-user command and the background job Sync, it does make the avatar from LDAP appear for users even if they never logged in.
Loading the user list does not make the avatar sync (on purpose I expect). I do not remember if loading the profile page does it.

At least with this PR when the avatar is fetched from LDAP it is synchronized. What the PR does not change is in which cases the avatar attribute is fetched.

I may try to backport a simplified version without the code cleaning.

Copy link
Member

Choose a reason for hiding this comment

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

Loading the user list does not make the avatar sync (on purpose I expect).

Indeed, this is left out with responsiveness in mind.

I do not remember if loading the profile page does it.

It does.

@skjnldsv skjnldsv mentioned this pull request Jan 14, 2025
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

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@artonge artonge Awaiting requested review from artonge artonge was automatically assigned from nextcloud/server-backend

@yemkareems yemkareems Awaiting requested review from yemkareems yemkareems was automatically assigned from nextcloud/server-backend

At least 2 approving reviews are required to merge this pull request.

Assignees

@come-nc come-nc

Labels
3. to review Waiting for reviews feature: ldap
Projects
None yet
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
2 participants
@come-nc @blizzz