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

Splitting of messages sent to IRC, prepending TG username to each one. #102

Merged
merged 5 commits into from Feb 8, 2019

Conversation

@michalrud
Copy link
Contributor

michalrud commented Dec 5, 2018

Should fix #53. Not tested in live environment.

Also fixed / improved some UTs. Especially hardcoded some config values in tests - some of them would fail if you don't have teleirc configured properly in the first place, what should not be required in UT IMHO.

@Tjzabel Tjzabel requested review from Tjzabel , xforever1313 and jwflory Dec 6, 2018

@Tjzabel

This comment has been minimized.

Copy link
Member

Tjzabel commented Dec 6, 2018

@michalrud first off, thanks for the PR!

At first glance

I've run a few tests off your branch. Firstly, multiline messages now work! Great work on this! 🎉 🎉
However, telegram photos don't go through to IRC, and so imgur photo uploading doesn't work.

A little more in-depth

Here is a stack trace of what happens when photos get uploaded to Telegram:

(node:4058) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'toString' of undefined
    at TeleIrc.sendMessageToIrc (/home/tim/test/teleirc/lib/TeleIrc.js:458:13)
    at TgImgurPhotoHandler.RelayPhotoMessage (/home/tim/test/teleirc/lib/TelegramHandlers/TgPhotoHandler.js:54:18)
    at process._tickCallback (internal/process/next_tick.js:68:7)
(node:4058) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:4058) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:4058) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'toString' of undefined
    at TeleIrc.sendMessageToIrc (/home/tim/test/teleirc/lib/TeleIrc.js:458:13)
    at TgImgurPhotoHandler.RelayPhotoMessage (/home/tim/test/teleirc/lib/TelegramHandlers/TgPhotoHandler.js:54:18)
    at process._tickCallback (internal/process/next_tick.js:68:7)
(node:4058) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)

@Tjzabel Tjzabel added this to the v1.3 milestone Dec 6, 2018

@Tjzabel
Copy link
Member

Tjzabel left a comment

Overall, great work! Thanks for taking the time to do this.

I've requested several changes, specifically towards the sendMessageToIrc method.

Show resolved Hide resolved docs/installation.rst Outdated
Show resolved Hide resolved docs/installation.rst
Show resolved Hide resolved lib/TeleIrc.js Outdated
Show resolved Hide resolved lib/TeleIrc.js Outdated
@Tjzabel

This comment has been minimized.

Copy link
Member

Tjzabel commented Dec 6, 2018

I've given my review. Since the changes I've requested are relatively large changes to how you've done things, I haven't taken much of a look to the unit tests you've created. Once the core functionality is working, I will take my time to go through the tests.

Thanks again 😄

@michalrud

This comment has been minimized.

Copy link
Contributor Author

michalrud commented Dec 6, 2018

@michalrud first off, thanks for the PR!

At first glance

I've run a few tests off your branch. Firstly, multiline messages now work! Great work on this! tada tada
However, telegram photos don't go through to IRC, and so imgur photo uploading doesn't work.

A little more in-depth

Here is a stack trace of what happens when photos get uploaded to Telegram:

(node:4058) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'toString' of undefined
    at TeleIrc.sendMessageToIrc (/home/tim/test/teleirc/lib/TeleIrc.js:458:13)
    at TgImgurPhotoHandler.RelayPhotoMessage (/home/tim/test/teleirc/lib/TelegramHandlers/TgPhotoHandler.js:54:18)
    at process._tickCallback (internal/process/next_tick.js:68:7)
(node:4058) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:4058) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:4058) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'toString' of undefined
    at TeleIrc.sendMessageToIrc (/home/tim/test/teleirc/lib/TeleIrc.js:458:13)
    at TgImgurPhotoHandler.RelayPhotoMessage (/home/tim/test/teleirc/lib/TelegramHandlers/TgPhotoHandler.js:54:18)
    at process._tickCallback (internal/process/next_tick.js:68:7)
(node:4058) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)

Okay - I know exactly what might have caused it. I'll fix it tomorrow + add an appropriate test. In short - now sendMessageToIrc receives two parameters, (sender, message), while it previously just received message - as sendMessageToIrc now prepends the prefix by itself.

@michalrud

This comment has been minimized.

Copy link
Contributor Author

michalrud commented Dec 7, 2018

I think I've fixed the issue in the simplest way possible, even if I don't like it that much. IMHO a cleaner way would be to expect all action callbacks to communicate using structs, but maybe I'm too broken by strongly typed languages :) Also I was thinking about providing a struct interface compatibility to queueTelegramMessage, but to be honest that wouldn't be used currently, so why add more unnecessary complication?

If you prefer, though, I can rework all Tg*Handler pieces to use the struct interface. It will IMHO make the code cleaner, but also the change bigger. You decide :)

@Tjzabel

This comment has been minimized.

Copy link
Member

Tjzabel commented Dec 7, 2018

@michalrud thanks for the updates! I will look through the updates over the weekend. I will defer the opinion of adding structs to @xforever1313 and @thenaterhood since they would be better able to answer that question.

@Tjzabel

This comment has been minimized.

Copy link
Member

Tjzabel commented Dec 8, 2018

@michalrud I've tested the new changes with uploading an image from Telegram, and it works! No errors spit out, and I can successfully see the imgur link (and it works) on IRC side.

@Tjzabel

This comment has been minimized.

Copy link
Member

Tjzabel commented Dec 8, 2018

Also, the updated tests look good to me 👍

@jwflory

jwflory approved these changes Dec 9, 2018

Copy link
Member

jwflory left a comment

I looked over this and the changes make sense to me. I'm approving this conditionally too on @Tjzabel's review to add IRC_MAX_MESSAGE_LENGTH to the env.example file. That's the only other thing I would like to see added.

Also, another question: does this close #76 about tests for NickServ authentication?

I didn't test this, but I'm going off of @Tjzabel's successful test too.

@jwflory

This comment has been minimized.

Copy link
Member

jwflory commented Dec 9, 2018

@michalrud Thanks again for your time and efforts on this PR! It is super appreciated. 🎉

@thenaterhood
Copy link
Member

thenaterhood left a comment

One comment which needs some discussion and/or revision. Other than that, looks good.

* @param {String} input.from - Name of the user from which the message was received.
* @param {String} input.message - Actual message to be sent.
*
* If `input` is provided as string, message is forwarded as-is. Otherwise, a prefix is

This comment has been minimized.

@thenaterhood

thenaterhood Dec 9, 2018

Member

I'm not sure this is the place for this logic. It would fit better in https://github.com/RITlug/teleirc/blob/master/lib/TelegramHandlers/TgMessageHandler.js#L33, which already has all the data handy for assembling a message. The message splitting logic you're adding looks like it can pretty much just be moved to the TgMessageHandler, swapping out the ircbot.say call for this._action. Moving it would also save a little work since you then don't need to worry about other data types.

As-is, this looks like a separation of concerns problem. There are two reasons this caught my attention:

  • This is a method that originally took a prebuilt piece of data and passed it along to somewhere else. Here, it gained the need to know more things and is now doing work that was originally done elsewhere. Design-wise for teleirc, message formatting should generally go in the TelegramHandlers (which would format to send to IRC) or in the IrcHandlers (which would format to send to Telegram).
  • A method that gains a need to handle an additional data type in the same parameter makes me raise an eyebrow. There can be valid reasons for this to happen (extensions to a legacy API, for example) but it's often a sign of a larger design problem.

This comment has been minimized.

@xforever1313

xforever1313 Dec 9, 2018

Contributor

I agree with Nate on this one. Teleirc.js shouldn't have any message construction or formatting, it should only be used for forwarding messages from the handlers to the desired destination(s). Message construction and formatting should be done in the various handlers. The handler's callback _action should pass in one, and one thing: The message in the form of a string to forward to the destination(s).

This comment has been minimized.

@michalrud

michalrud Jan 28, 2019

Author Contributor

I don't really agree:

Design-wise for teleirc, message formatting should generally go in the TelegramHandlers (which would format to send to IRC)

The fact, that messages have certain length limitations is the issue on the IRC side. Why should Telegram part read IRC configuration options? What if you'd like to switch IRC backend to a different one, which doesn't have that limitation or has a different value?

A method that gains a need to handle an additional data type in the same parameter makes me raise an eyebrow.

True - I left this as is to keep the change small and easily reviewable, for future refactoring.

But, in the end, it's your codebase, I will do the requested changes and you'll decide if you prefer my old approach or not :)

This comment has been minimized.

@xforever1313

xforever1313 Feb 3, 2019

Contributor

Heh, the problem with dynamic languages.

I wouldn't think of it as passing in an IRC Config object, but passing in an object that implements an interface that contains the prefix, suffix, and maximum length; an interface that IRC Config just so happens in implement. A theoretical SlackConfig object or a WhatsAppConfig object could implement the same "interface" and be passed in instead. Its why the parameter in the constructor was originally called "prefixSuffixConfig" instead of "ircConfig".

@xforever1313
Copy link
Contributor

xforever1313 left a comment

I agree with Nate's comments here: https://github.com/RITlug/teleirc/pull/102/files#r240022975

Other than that, things look good.

Show resolved Hide resolved lib/TeleIrc.js Outdated
* @param {String} input.from - Name of the user from which the message was received.
* @param {String} input.message - Actual message to be sent.
*
* If `input` is provided as string, message is forwarded as-is. Otherwise, a prefix is

This comment has been minimized.

@xforever1313

xforever1313 Dec 9, 2018

Contributor

I agree with Nate on this one. Teleirc.js shouldn't have any message construction or formatting, it should only be used for forwarding messages from the handlers to the desired destination(s). Message construction and formatting should be done in the various handlers. The handler's callback _action should pass in one, and one thing: The message in the form of a string to forward to the destination(s).

@Tjzabel

This comment has been minimized.

Copy link
Member

Tjzabel commented Dec 14, 2018

@michalrud hey, how are the changes coming along?

@jwflory

This comment has been minimized.

Copy link
Member

jwflory commented Jan 15, 2019

@michalrud Hi! Did you get a chance to revisit this PR?

@michalrud

This comment has been minimized.

Copy link
Contributor Author

michalrud commented Jan 15, 2019

Not yet - did a bit, but not yet finished. I'll try to wrap it up today and update the PR.

@Tjzabel

This comment has been minimized.

Copy link
Member

Tjzabel commented Jan 27, 2019

@michalrud how goes it?

@michalrud michalrud force-pushed the michalrud:message_splitting branch from 38df2d5 to ba010ca Jan 28, 2019

@jwflory jwflory added this to In progress in Teleirc development Feb 2, 2019

@jwflory jwflory moved this from In progress to Needs review in Teleirc development Feb 2, 2019

@jwflory jwflory requested review from xforever1313 , Tjzabel and thenaterhood Feb 2, 2019

const messagePrefix = self._ircConfig.prefix + username + self._ircConfig.suffix + " ";
const messageSplitRegex = new RegExp(`.\{1,${self._ircConfig.maxMessageLength - messagePrefix.length}}`, 'g');

userMessage.toString().split(/\r?\n/).filter(function(line) {

This comment has been minimized.

@xforever1313

xforever1313 Feb 3, 2019

Contributor

Can you add a comment here just explaining that you are splitting the string? If someone new looks at this code, it may not be obvious what is going on.

This comment has been minimized.

@Tjzabel

Tjzabel Feb 8, 2019

Member

@xforever1313 is a comment super necessary here? It is using the .split method, so the string splitting should be inherently known?

@xforever1313

This comment has been minimized.

Copy link
Contributor

xforever1313 commented Feb 3, 2019

I have one request, and that is just adding a comment (https://github.com/RITlug/teleirc/pull/102/files#r253285164). Everything thing else looks good.

Once that comment is added: Approved!

@Tjzabel

Tjzabel approved these changes Feb 8, 2019

Teleirc development automation moved this from Needs review to Reviewer approved Feb 8, 2019

@Tjzabel

This comment has been minimized.

Copy link
Member

Tjzabel commented Feb 8, 2019

As an update, I've tested these past new commits locally, and everything works as planned 👍

@jwflory

This comment has been minimized.

Copy link
Member

jwflory commented Feb 8, 2019

Excellent. I'm going to merge this one in. Thanks @michalrud for contributing this feature! 🎉

Merging. 🎬

@jwflory jwflory merged commit cb29322 into RITlug:master Feb 8, 2019

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Teleirc development automation moved this from Reviewer approved to Done Feb 8, 2019

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