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.

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

Add activity for display name change #25148

Closed
wants to merge 1 commit into from

Conversation

MorrisJobke
Copy link
Member

Looks then like this:

Bildschirmfoto 2021-01-15 um 13 12 17

@MorrisJobke MorrisJobke force-pushed the enh/noid/displayname-change-activity branch 2 times, most recently from 9dfd938 to 20499c1 Compare January 15, 2021 12:39
@blizzz
Copy link
Member

blizzz commented Jan 15, 2021

let me see what happens when the name changed on LDAP and got synced down

@blizzz
Copy link
Member

blizzz commented Jan 15, 2021

let me see what happens when the name changed on LDAP and got synced down

Screenshot_20210115_154133

That ideally should be something like "You display name was updated by the system". We had the same once with the email changed event as well.

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.

@MorrisJobke MorrisJobke force-pushed the enh/noid/displayname-change-activity branch from 20499c1 to d01d2c1 Compare January 15, 2021 16:38
@blizzz
Copy link
Member

blizzz commented Jan 18, 2021

Works now, when it is updated by the system (occ or background job for instance) it is fine.

There is another edge case:

Screenshot_20210118_115056

This was during normal operation of that user and due to cold cache. This can happen when the user is included in a search operation by someone else, then it was xyz who changed the displayname. But then the better change would be to not update the displayname on action, but leave it to the background job as well. Let me take care of that.

Users that are being soaked in for the first time do not see the activity. This is better not a race condition :D

Apart of that I expect the ajax-type job does not run with context of the logged in user.

@rullzer
Copy link
Member

rullzer commented Jan 19, 2021

So we are good to go?

@MorrisJobke
Copy link
Member Author

So we are good to go?

It has false claims for LDAP users (stuff like "Your displayname was updated by AnotherUser" if the other user searched for something and your entry was updated by the system). I don't like this that much. Also back porting then the big change by @blizzz that changes the fetching of LDAP data is also something I don't fully like. :(

Let's keep this a bit longer here. Maybe @blizzz has an idea how to nicely work around this situation.

@rullzer rullzer mentioned this pull request Jan 21, 2021
19 tasks
@rullzer rullzer mentioned this pull request Jan 29, 2021
@ChristophWurst ChristophWurst removed this from the Nextcloud 21 milestone Jan 29, 2021
@ChristophWurst ChristophWurst added this to the Nextcloud 22 milestone Jan 29, 2021
@ChristophWurst
Copy link
Member

Let's keep this a bit longer here. Maybe @blizzz has an idea how to nicely work around this situation.

Moved. We can still backport from 22 if the changes are small.

This was referenced May 20, 2021
@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@blizzz
Copy link
Member

blizzz commented Jun 2, 2021

and am afraid once more to 23. still work to be done. and conflicts to resolve.

@blizzz blizzz modified the milestones: Nextcloud 22, Nextcloud 23 Jun 2, 2021
@skjnldsv

This comment has been minimized.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@skjnldsv skjnldsv force-pushed the enh/noid/displayname-change-activity branch from d01d2c1 to 6a99f21 Compare August 9, 2021 08:32
@skjnldsv
Copy link
Member

skjnldsv commented Aug 9, 2021

Test are green. @blizzz wanna do something on top of that?

@blizzz
Copy link
Member

blizzz commented Aug 9, 2021

Test are green. @blizzz wanna do something on top of that?

Don't remember without looking into it again. Went down the prio list a lot previously. I think #25148 was a requirement.

@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 21, 2021
@MorrisJobke
Copy link
Member Author

Tricky to solve and most likely needs a more advanced way of doing it. Therefore I will close this one here. Feel free to take parts of this into a new PR.

@MorrisJobke MorrisJobke closed this Feb 7, 2022
@MorrisJobke MorrisJobke deleted the enh/noid/displayname-change-activity branch February 7, 2022 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants