Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-24082: Add profile picture attribute to admin console settings #5457

Merged
merged 5 commits into from
May 14, 2020
Merged

MM-24082: Add profile picture attribute to admin console settings #5457

merged 5 commits into from
May 14, 2020

Conversation

sbishel
Copy link
Member

@sbishel sbishel commented May 7, 2020 •
edited
Loading

Summary

Adds Profile Picture Attribute to Admin Console Settings

Ticket Link

https://mattermost.atlassian.net/browse/MM-24082

Related Pull Requests

Screenshots

Screen Shot 2020-05-12 at 4 39 37 PM

@sbishel sbishel mentioned this pull request May 7, 2020
Merged
@sbishel sbishel requested review from fmunshi and hahmadia May 8, 2020 21:03
@sbishel sbishel added 1: UX Review Requires review by a UX Designer 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels May 8, 2020
@sbishel sbishel requested a review from michaelgamble May 8, 2020 21:04
@srkgupta srkgupta self-requested a review May 11, 2020 15:05
@hahmadia hahmadia removed the 2: Dev Review Requires review by a core commiter label May 11, 2020
michaelgamble
michaelgamble suggested changes May 12, 2020
View reviewed changes
Copy link
Contributor

@michaelgamble michaelgamble left a comment

Choose a reason for hiding this comment

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

I would suggest changing the help text to say "The attribute in the AD/LDAP server used to populate the profile picture in Mattermost."

@amyblais amyblais added this to the v5.24.0 milestone May 13, 2020
@sbishel
Copy link
Member Author

sbishel commented May 14, 2020

/update-branch

srkgupta
srkgupta suggested changes May 14, 2020
View reviewed changes
Copy link
Contributor

@srkgupta srkgupta left a comment

Choose a reason for hiding this comment

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

Hi @sbishel

I had tested this PR along with the dependent server and enterprise PR and I am seeing the following issues:

  1. Inspite of configuring the profile picture correctly, when the user logs in, the user's profile picture is not updated. Please check this GIF to demonstrate this issue:

https://community-release.mattermost.com/files/r6e47uca7ibdtmkokac7rfwt5c/public?h=j8qLWHyuzQ9_sYpqsX9We1sqLEKHOCFUm70_Xp_o1YI

  1. If the sysadmin configures the LDAP user to use profile picture, the user is still allowed to change their profile picture from Account Settings. This option should be locked just like any other options (username, first name, last name etc).

@michaelgamble or @thefactremains can you please confirm if the expected behavior in issue #2 is correct and this needs to be locked?

@srkgupta
Copy link
Contributor

srkgupta commented May 14, 2020 •
edited
Loading

Issue #3
When we click on AD/LDAP Synchronize Now button multiple times even without changing any attributes, the status still shows as "Updated 4 users" like this:

Screenshot 2020-05-14 at 9 30 17 PM

Expected: Since no attributes were updated, the status log should not include any updates to the users as well.

michaelgamble
michaelgamble approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@michaelgamble michaelgamble left a comment

Choose a reason for hiding this comment

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

The content change seems good

@sbishel
Copy link
Member Author

sbishel commented May 14, 2020

@srkgupta
#1 this PR simply adds the UI for setting the attribute
https://github.com/mattermost/enterprise/pull/650
mattermost/mattermost#14540
Actually have the changes for the update.

#2 above in handled here - #5465

srkgupta
srkgupta approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@srkgupta srkgupta left a comment

Choose a reason for hiding this comment

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

Approving the PR based on above comments. All issues mentioned earlier will be addressed in the future PRs. Did a smoke testing on the LDAP features on this PR and it's dependent PRs and the feature worked fine without any issues.

@srkgupta srkgupta added 4: Reviews Complete All reviewers have approved the pull request and removed 1: UX Review Requires review by a UX Designer 3: QA Review Requires review by a QA tester labels May 14, 2020
@sbishel sbishel merged commit fd3aeb8 into mattermost:master May 14, 2020
@amyblais amyblais added the Changelog/Not Needed Does not require a changelog entry label May 14, 2020
@amyblais amyblais added the Docs/Not Needed Does not require documentation label May 14, 2020
bradjcoughlin pushed a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 19, 2020
@sbishel @mattermod @bradjcoughlin
MM-24082: Add profile picture attribute to admin console settings (ma…
4c90af9 
…ttermost#5457)

* add profile picture attribute to admin console settings

* update translation file

* update text

* remove blank line

Co-authored-by: mattermod <[email protected]>
bradjcoughlin pushed a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 19, 2020
@sbishel @mattermod @bradjcoughlin
MM-24082: Add profile picture attribute to admin console settings (ma…
805870d 
…ttermost#5457)

* add profile picture attribute to admin console settings

* update translation file

* update text

* remove blank line

Co-authored-by: mattermod <[email protected]>
@sbishel sbishel deleted the MM-24082-add-profile-picture branch June 1, 2020 17:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@srkgupta srkgupta srkgupta approved these changes

@fmunshi fmunshi fmunshi approved these changes

@hahmadia hahmadia hahmadia approved these changes

@michaelgamble michaelgamble michaelgamble approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation
Projects
None yet
Milestone
v5.24.0
7 participants
@sbishel @srkgupta @fmunshi @hahmadia @michaelgamble @amyblais @mattermod