- 
                Notifications
    You must be signed in to change notification settings 
- Fork 135
fix self-mention in comments #5115
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
Conversation
| Thanks a lot, I've restarted cypress as it seemed like an unrelated failure. I'll have a closer look later today | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, there is just a small comment regarding display names and a typo left from my perspective.
| Thank you very much for the comments. Right, checking for the display name makes much more sense, I’ve added that. I also had a quick look at the server-side implementation, and it seems that UID checks are exact match checks there (which might explain why exact match actually works if uid===displayname). So to be consistent, maybe it makes sense to change the UID check to an exact match check against the input text? Also, please let me know if I should add some unit tests (or even Cypress?). I couldn’t find a unit test setup in the project, so I skipped it this weekend due to time constraints (and some frustration with my currently very slow development setup). | 
| Sorry for the delayed response and thanks for checking this further in depth. I think the current approach implemented is good for this. I think not limiting uid check is more suitable, also comparing how for example Talk does the matches against the uid. Though I see it is not consistent with the autocomplete endpoint. So far we only have cypress tests for the frontend code parts in this repo, however I fear those would be to much dependent on the implementation within Collabora Online, so hesitant adding them. It might be nice to add vitest to this repo and have some actual unit tests for parts like this where the full integration would be overkill, but seems to exceed the scope of this PR. | 
| Just one minor bit that I just noticed when giving this another test run, we probably should check case-insensitive? | 
| I updated the check to be case-insensitive. The failing tests appear to be unrelated again, but I’ll take a closer look to confirm. | 
| Yes, those tests seem to also fail on main, so agree that they are unrelated https://github.com/nextcloud/richdocuments/actions/runs/18894277933 | 
| Mind to squash your commits into one and rebase them to current main? Then it is good to go from my side 🚀 | 
| With #5130 the tests should also pass again after the rebase | 
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@gmx.de>
d62c7b6    to
    25051e5      
    Compare
  
    | I’ve squashed all commits into one and rebased onto the latest main. All tests are passing again. Thanks for your constructive feedback! | 
| Thanks a lot 🤝 | 
Summary
This workaround fixes the self-mention issue described in #4184 until nextcloud/server#48180 is implemented. The current user is now allowed to mention himself in the comments.