Telegram, initial language setup (for future changes in frontend), don't remove last buy when under amount, reworked notifications. #204

pedrohusky · 2021-05-28T22:35:31Z

Right now it has:

-Telegram notifications;
-Don't remove last buy when under X amount (minimum 1);
-Reworked notifications to be readable in telegram and slack.
-I've added a language switch to receive the updates in one of the two current languages (enUS and ptBR).
-It should spam less checking buy.

SETUP TELEGRAM:

To setup telegram, you just need to edit .env file and fill the form. The bot you can create talking with @Botfather in telegram and he will tell the steps to do.
The chat_ID you can get by talking with @userinfobot.

Motivation and Context

Those changes was required by a lot of people in discord and telegram. It solves the problem that the bot would loose its last price, the coin drops and the bot buys more. Then it sells every coin bought at the last coin bough profit.

Telegram is necessary, not everyone uses slack.

I've reworked the messages to be more readable for the user. It is a work in progress. It will be polished later.

Multilanguage is very appreciated by everyone. :)

How Has This Been Tested?

I've tested the notifications from telegram, the last buy price is working 100% (the bot removes when it sell successfully), and the notification language too.

Screenshots (if appropriate):

-- This is using the ptBR language!

@Botfather

Right now it has: -Telegram notifications; -Don't remove last buy when under X amount (minimum 1); -Reworked notifications to be readable in telegram and slack. -I've added a language switch to receive the updates in one of the two current languages (enUS and ptBR). -It should spam less checking buy. To setup telegram, you just need to edit .env file and fill the form. The bot you can create talking with @Botfather in telegram and he will tell the steps to do. The chat_ID you can get by talking with @userinfobot. Thanks. I'll open a push request.

chrisleekr · 2021-05-29T12:23:44Z

Hey,

Thanks for your contributions. This is pretty good.

It looks like you try to cover two features:

Remove the last buy price when it is under configured amount. #190
Improve notificaitons. #106

Again that is awesome, thanks for that.

However, I see some issues with your PR.

General:

Too many changes for a single PR. Since I am the one who should review the changes, it's kind of troublesome to see massive changes for multiple features. I understand notification changes are pretty big, but removing the last buy price is pretty small. Can you separate PR to each feature?
All changes must be written with the test. I see lots of failing when I run npm run test.

Remove the last buy price when it is under configured amount:

I can see your change is having a single configuration for the last buy remove threshold. As I mentioned in Coin price <10$ bot deletes last buy price #190, it should support multiple quote assets. Something like max purchase amount - https://github.com/chrisleekr/binance-trading-bot/blob/master/public/js/SettingIconMaxPurchaseAmount.js
I also found some redundant code but I can review that once you open new PR with this feature.

Improve notificaitons:

I understand the current Slack message is quite long, which there is a reason for that. The bot itself generates a massive log to make sure it runs correctly. It's not possible to trace with the log because they are just too much. I am using Slack to trace/debug the issues when I couldn't see the log. It must be improved as you said, it is too long and spamming. At this point, simply reducing the content of the message won't be helpful to debug what happened with the trade. To improve the notification, it first needs to find a way to record debug messages so we can trace later on whether the bot was working as expected or not.
I understand you need Telegram notification because it's your main messenger. If you see issue Notifications #106, someone suggested Apprise, which can support multiple services. So my idea is rather than supporting a single extra notification service (because others may need another service), research about this notification service might be a good idea.
In addition to Apprise, the notification type must be selected. For example, if I select to receive buy/sell action results, then it should only send those notifications.
little typo alert (messager -> messenger)

I appreciate your effort for the PR, let's continue to discuss.

pedrohusky · 2021-05-29T12:44:32Z

Too many changes for a single PR. Since I am the one who should review the changes, it's kind of troublesome to see massive changes for multiple features. I understand notification changes are pretty big, but removing the last buy price is pretty small. Can you separate PR to each feature?

Of course! Sorry for that. It was the replace all fault. I needed to replace every 'slack' reference and then it changed even the .test files. I will separate them.

Remove the last buy price when it is under configured amount:

I can see your change is having a single configuration for the last buy remove threshold. As I mentioned in Coin price <10$ bot deletes last buy price #190, it should support multiple quote assets. Something like max purchase amount - https://github.com/chrisleekr/binance-trading-bot/blob/master/public/js/SettingIconMaxPurchaseAmount.js

Sure! I didn't think about that. I thought the symbols would save the last amount when we changed. But they don't. I will do that!

Improve notificaitons:

I understand the current Slack message is quite long, which there is a reason for that. The bot itself generates a massive log to make sure it runs correctly. It's not possible to trace with the log because they are just too much. I am using Slack to trace/debug the issues when I couldn't see the log. It must be improved as you said, it is too long and spamming. At this point, simply reducing the content of the message won't be helpful to debug what happened with the trade. To improve the notification, it first needs to find a way to record debug messages so we can trace later on whether the bot was working as expected or not.

And if the bot send the debug message as a .txt file through telegram (don't know if slack support it) ? It would be readable and we can even open the file to see what gone wrong.

little typo alert (messager -> messenger)

Kkk, sorry for the messager 😞. I will rename it. It was my fault.

I understand you need Telegram notification because it's your main messenger. If you see issue Notifications #106, someone suggested Apprise, which can support multiple services. So my idea is rather than supporting a single extra notification service (because others may need another service), research about this notification service might be a good idea.

I will study apprise to implement it. Thanks for the head's up.

In addition to Apprise, the notification type must be selected. For example, if I select to receive buy/sell action results, then it should only send those notifications.

Oh. Again, didn't tought about that. Sure I will implement the notification options!

I appreciate your effort for the PR, let's continue to discuss.

I'm very happy to help. Thanks, again and sorry for the newbie mistakes. ❤️

chrisleekr · 2021-05-29T12:46:44Z

@pedrohusky 👍

I'm very happy to help. Thanks, again and sorry for the newbie mistakes. heart

No problem. Please don't be sorry. It's always welcome to get any contributions. :)

FYI, I just edited my last message. Just in case you missed.

All changes must be written with the test. I see lots of failing when I run `npm run test.`

pedrohusky · 2021-05-29T12:48:29Z

@pedrohusky 👍

I just edited my last message. Just in case you missed.
All changes must be written with the test. I see lots of failing when I run `npm run test.`

Sorry, didn't know that. I will fix it asap. Didn't know the test files was useful. Now I know. Sorry. They are giving failure because I didn't fix the slack replace in them.

chrisleekr · 2021-05-29T12:49:44Z

@pedrohusky

Yeah, writing tests are a horrible experience, but if I also find lots of my mistakes by writing the tests.
It helps to make sure your code runs as expected.

chrisleekr · 2021-05-29T13:15:00Z

@pedrohusky Btw, for the language code, please follow ISO language code - https://www.andiamo.co.uk/resources/iso-language-codes/

pedrohusky · 2021-05-29T13:19:31Z

Btw, for the language code, please follow ISO language code - https://www.andiamo.co.uk/resources/iso-language-codes/

@chrisleekr sure! I'm making some changes right now. :)

@chrisleekr

Edited the .test files to their original state. Messager is now messenger. Last buy threshold now is like max purchase amount, thanks @chrisleekr

pedrohusky · 2021-05-29T21:11:19Z

@chrisleekr

Hi! I've made the last buy threshold work like max purchase amount, the language now follows the ISO standard.

How to work with test files? I only get this:

binance-trading-bot@0.0.66 test
cross-env NODE_ENV=test jest --coverage --detectOpenHandles

sh: 1: cross-env: not found

Don't know what to do.

I still don't know how what to do with apprise, will look more into it. Could you please point me a direction?

chrisleekr · 2021-05-30T02:34:25Z

Closed in favour of #206

pedrohusky changed the title ~~Telegram, initial language setup (for future changes in frontend), reworked notifications.~~ Telegram, initial language setup (for future changes in frontend), don't remove last buy when under amount, reworked notifications. May 28, 2021

pedrohusky added 4 commits May 29, 2021 14:42

Language now follows ISO 639-1 standards.

7c6f4a4

Edited the .test files to their original state. Messager is now messenger. Last buy threshold now is like max purchase amount, thanks @chrisleekr

updated readme

4d55cfd

polished some code

367e034

test

b7b7260

pedrohusky force-pushed the telegram-language-lastbuy branch from 417fe24 to b7b7260 Compare May 29, 2021 21:05

pedrohusky and others added 2 commits May 29, 2021 18:06

test

8752906

Merge branch 'chrisleekr:master' into telegram-language-lastbuy

8450d78

chrisleekr closed this May 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Telegram, initial language setup (for future changes in frontend), don't remove last buy when under amount, reworked notifications. #204

Telegram, initial language setup (for future changes in frontend), don't remove last buy when under amount, reworked notifications. #204

pedrohusky commented May 28, 2021 •

edited

Loading

chrisleekr commented May 29, 2021 •

edited

Loading

pedrohusky commented May 29, 2021 •

edited

Loading

chrisleekr commented May 29, 2021 •

edited

Loading

pedrohusky commented May 29, 2021

chrisleekr commented May 29, 2021

chrisleekr commented May 29, 2021

pedrohusky commented May 29, 2021

pedrohusky commented May 29, 2021 •

edited

Loading

chrisleekr commented May 30, 2021

Telegram, initial language setup (for future changes in frontend), don't remove last buy when under amount, reworked notifications. #204

Telegram, initial language setup (for future changes in frontend), don't remove last buy when under amount, reworked notifications. #204

Conversation

pedrohusky commented May 28, 2021 • edited Loading

SETUP TELEGRAM:

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

chrisleekr commented May 29, 2021 • edited Loading

pedrohusky commented May 29, 2021 • edited Loading

chrisleekr commented May 29, 2021 • edited Loading

pedrohusky commented May 29, 2021

chrisleekr commented May 29, 2021

chrisleekr commented May 29, 2021

pedrohusky commented May 29, 2021

pedrohusky commented May 29, 2021 • edited Loading

chrisleekr commented May 30, 2021

pedrohusky commented May 28, 2021 •

edited

Loading

chrisleekr commented May 29, 2021 •

edited

Loading

pedrohusky commented May 29, 2021 •

edited

Loading

chrisleekr commented May 29, 2021 •

edited

Loading

pedrohusky commented May 29, 2021 •

edited

Loading