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

change displayName with IEventDispatcher #581

Closed
wants to merge 1 commit into from
Closed

change displayName with IEventDispatcher #581

wants to merge 1 commit into from

Conversation

Superjamp
Copy link
Contributor

No description provided.

@Superjamp
displayname switch to eventdispatcher
0cbe1c0 
Signed-off-by: Sebastian Biller <s.biller@tu-braunschweig.de>
@juliusknorr
Copy link
Member

juliusknorr commented Feb 17, 2022 •
edited
Loading

@Superjamp Thanks for your pull request. Could you add some additional details on if that was due to any specific issues you encountered? I've just been debugging an issue that potential display name changes were not propagated to the user account data, to be more specific, the display name shown in the user profile. Is that similar to what you encountered?

Steps I had for running into this:

  • Have a SAML user who already was logged in
  • The display name mapping value changes
  • The user logged in again through SAML
    -> Now the display name was only changed in the saml database table, not on the oc_accounts entry

juliusknorr
juliusknorr approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Tested and fixes the issue I was hunting 👍

@juliusknorr juliusknorr requested a review from blizzz February 17, 2022 15:43
@blizzz
Copy link
Member

blizzz commented Feb 17, 2022

CI phpunit fails, seems like an issue with the tests themselves though.

@Superjamp
Copy link
Contributor Author

@juliushaertl 👍
I came across this some time ago and now I have not found a good explanation of the way you did it for a pull request.
But it is the same Issue/steps to reproduce I had last year.

@juliusknorr
Copy link
Member

Looks like it is actually a regression from nextcloud/server#27474

Test failures seem expected due to the fact that the event dispatcher is not injected and therefore not mocked in the unit test cases. Will see if I can push a fix for that.

@juliusknorr juliusknorr mentioned this pull request Feb 18, 2022
Merged
@juliusknorr
Copy link
Member

I've pushed your commit with an additional one for the tests to #582

@blizzz
Copy link
Member

blizzz commented Apr 11, 2022

superseded by #582

@blizzz blizzz closed this Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@blizzz blizzz blizzz approved these changes

Assignees
No one assigned
Labels
3. to review bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Superjamp @juliusknorr @blizzz