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

Don't highlight mention of unknown user #1006

Closed
schiessle opened this issue Jun 27, 2018 · 8 comments
Closed

Don't highlight mention of unknown user #1006

schiessle opened this issue Jun 27, 2018 · 8 comments

Comments

@schiessle
Copy link
Member

This is what happens if I mention a user who doesn't exists:

talk - mention

I think instead we shouldn't format the mention and show it as a normal string. Imagine I want to tell someone I'm @schiessle on Twitter, this shouldn't be changed to I'm @UnknownUser at Twitter

@mario
Copy link
Contributor

mario commented Jun 27, 2018

I opened the same issue and it got closed :P

@schiessle
Copy link
Member Author

I discussed it yesterday with @jancborchardt and @Ivansss and we all agreed... So let's see if it survives this time 😉

@mario
Copy link
Contributor

mario commented Jun 27, 2018

cc @blizzz

@blizzz
Copy link
Member

blizzz commented Jun 27, 2018

Nope, not just doing this is not the nice way to deal with it. With default LDAP config, as soon as a user vanishes, you'll have not only the userids exposed to end users, but also in a technical way: @e8a7e882-5c5e-4f61-b6bb-bc5fd9b3cb4b.

But I am getting there is a problem. To tackle it, I see to alternatives here:

  1. don't use Twitter
  2. we apply markup for mentions. At the moment in the raw comment it is just @userid. It already does not support spaces in the username, and with that we would lay foundations to support other types, e.g. groups. Then, a plain @whoami would be printed like this. That't of course a lot more work and old comments containing mentions would need to be converted. This can take some time on big instances (alternatively it would be done on the fly, a version would neet to be added to the database table in that case). Downside: comment format/markup changes are not backwards compatible, unless we teach the clients to announce which comments version they understand and get it formatted accordingly.

@schiessle
Copy link
Member Author

schiessle commented Jun 27, 2018

Just discussed this with @Ivansss, I'm not sure if I understand the problem correctly.

Someone types I'm @schiessle on Twitter, then @schiessle is send to the end-point to check for the user and returns the display name of the user (and maybe additional information). The UI renders it as a mention and store it in the database in plain text I'm @schiessle on Twitter. (Or people type I'm @Veggie Voodoo King to say they are at some place – this is not exclusive to Twitter ;)

The end-point could return additionally a flag like detected = true|false which is set to false if the user doesn't exists. The UI doesn't render it as a mention and store it again in plain text in the database I'm @schiessle on Twitter

For already existing comments/chats I don't care that much if we can display it correctly. But fixing it for new comments/messages from now on should be feasible, right?

Do I miss something?

@jancborchardt
Copy link
Member

Agree with @schiessle here. There’s some other cases additionally:

  • You simply have a typo in the username: @schiesle or @blizzzz would be completely made unreadable to @Unknown user, which is worse than showing them with a typo
  • The case of LDAP you bring up we can easily catch as the LDAP ID is a format we can parse. For those non-existent LDAP users we can indeed show @Unknown user
  • In the case where you do not use LDAP but Nextcloud user management and a user is deleted, then showing @Unknown user will make the context invisible too. With a non-marked-up @ivan you at least know who it was addressed too, even though they aren’t in the system anymore.

@nickvergessen
Copy link
Member

I opened the same issue and it got closed :P

#895

Yes, it got closed, for the same reason I will close it now again.

"I agree, however that needs fixing in the comments manager in the server repo …"

So please open an issue in the server repo so we can fix comments and chat in one go.

@nickvergessen
Copy link
Member

nickvergessen commented Jun 27, 2018

And it would be nice to report and fix this soon, so it get's merged for 14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants