search

RITlug / teleirc

Go implementation of a Telegram <=> IRC bridge for use with any IRC channel and Telegram group
https://docs.teleirc.com/
GNU General Public License v3.0
141 stars 45 forks source link

Fix Telegram zero-width space #334

Closed Tjzabel closed 4 years ago

Tjzabel commented 4 years ago

Overview

This issue finally puts an end to the ZWSP issues. Users who have the same Telegram and IRC nicknames should now be able to safely message from each platform, and avoid pinging oneself.

Closes #328

Details

Telegram usernames now contain a ZWSP when sent over to IRC to prevent self-pinging:

<t<200b>estUser> Hello

This resolves into:

<testUser> Hello

Questions

  1. Do we want this option to have a toggle?
jwflory commented 4 years ago

@Tjzabel Since it is a ZWSP, I might not be able to clearly see the change happening or how to validate the fix:

Screenshot of helpers.go git diff from GitHub Pull Request interface

Could you provide some context into what your change is doing to fix the bug?

@Tjzabel: Do we want this option to have a toggle?

Based on feedback we got in the community chat, I think yes. I believe we should default the ZWSP to present though. I hypothesize there are fewer use cases for this to be disabled (but enough that it is important to make it configurable).

Tjzabel commented 4 years ago

@jwflory I agree it should at least be on by default. I can work on creating a toggle for this option then.

I understand it's hard to view via the diff itself, which is why I included how it looks in the original comment. Basically, if you copy/paste the "" you see in the diff into a terminal, you will be able to see a <200b> zwsp character

jwflory commented 4 years ago

Updated this Pull Request to a draft PR.

Tjzabel commented 4 years ago

@jwflory may I ask why this was moved to a draft PR?

jwflory commented 4 years ago

Oh sorry! Is it ready for final review? I forgot where the status was on this.

The CI failing is an issue I need to look at again, right? :thinking:

Tjzabel commented 4 years ago

This PR is effectively complete, as it passes all tests locally for me, and runs as it should. The main issue is the CI itself is borked due to possibly needing a newer version. I will open up a new issue for that

jwflory commented 4 years ago

@Tjzabel I'll leave it to you to merge if you think it is ready. But it looks good on my side. :slightly_smiling_face:

Tjzabel commented 4 years ago

@jwflory I was planning to work on the function separation but my bandwidth turned out to be quite low :sweat_smile: I will do a final review of this PR, and submit an enhancement issue for creating that zwsp-check function.

codeclimate[bot] commented 4 years ago

Code Climate has analyzed commit cb5b7f0b and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 60.1% (4.1% change).

View more on Code Climate.