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

Reformat notificatiers code, add telegram library and support, add discord webhook support #101

Merged
merged 9 commits into from
Oct 2, 2021
Merged

Reformat notificatiers code, add telegram library and support, add discord webhook support #101

merged 9 commits into from
Oct 2, 2021

Conversation

mikhail5555
Copy link
Contributor

@mikhail5555 mikhail5555 commented Apr 13, 2021

New config options:

    "notificationType": "telegram",
    "telegram": {
        "token": "<TelegramBotToken>",
        "chatIds": ["<ChatIds>"]
    }
    "notificationType": "discord",
    "discord": {
        "webhookUrl": "https://discord.com/api/webhooks/...../......
    }

To generate TelegramBotToken: https://sendpulse.com/knowledge-base/chatbot/create-telegram-chatbot
Then start chat with the bot, and send it a message.
Then go to: https://api.telegram.org/bot<TelegramBotToken>/getUpdates
And get your chatId.

I have tested this local running Node v15, and in docker.
Clipboard - April 13, 2021 10_01 PM

@claabs
Copy link
Owner

claabs commented Apr 15, 2021

I really appreciate the PR! However, I'd like to fix #97 before merging this.

I like the notificationType config, and I'll probably end up extending it to add LOG and OPEN for developments.

We'll have to make sure we update the Readme and clarify these options.

@mikhail5555
Copy link
Contributor Author

Take your time! I tried to clean up code where i can in my last commit, i hope you like the changes.
I hope this should make it easier to add new notifier methods and config validations for them.
The readme indeed will need a small update, might do that a bit later if i have time.

@mikhail5555
Copy link
Contributor Author

Small update: I build and deployed this version locally, with telegram enabled, and it seems to be working as before.
(Also received notifications through telegram quickly)
I updated the node version to 15 for myself, since it was same as i had running locally, i can push this configuration if you want to cherry pick some things out of it.

@mikhail5555 mikhail5555 changed the title Reformat notificatiers code, add telegram library and support Reformat notificatiers code, add telegram library and support, add discord webhook support Apr 17, 2021
@nitanmarcel
Copy link

This PR is awesome. Tho something I might want to say. I don't know if it's just me but personally I like adding big requirements for small things that could be easily done manually with the already existing features. A simple send message over telegram could be done by easily making a post request to the bot API.

https://api.telegram.org/botTELEGRAM_BOT_TOKEN/sendMessage.

Here's a curl version for it:

https://gist.github.com/dideler/85de4d64f66c1966788c1b2304b9caf1

Anyway I'm waiting for this PR to be a thing since I'm not reading my emails so often and usually I ignore notifications from it ^^

@mikhail5555
Copy link
Contributor Author

@nitanmarcel Yeh you are completely right, got a little bit sloppy there. I removed the library and exchanged it for the post call.

@nitanmarcel
Copy link

nitanmarcel commented Jun 1, 2021

@mikhail5555 I noticed you're missing the docker configuration in Readme.md. Is it not implemented or not documented? I manage to find the correct value by reading the config.ts file. Everything works fine ^^

Also I couldn't notice you removed the markdown in the message which can be still used with "parse_mode": "markdown" ^^

@nitanmarcel
Copy link

I really appreciate the PR! However, I'd like to fix #97 before merging this.

I like the notificationType config, and I'll probably end up extending it to add LOG and OPEN for developments.

We'll have to make sure we update the Readme and clarify these options.

Until then I wanted to try this PR so I merged it and your latest changes in a fork I'm having in sr.ht.

So far it works nice excepting that the login by completing the captcha doesn't work, but I still have to try with your own to know where's the problem.

Anyway, if someone is interested :

https://git.sr.ht/~nitanmarcel/epicgames-freegames-node-telegram

@mikhail5555
Copy link
Contributor Author

mikhail5555 commented Jun 6, 2021

@mikhail5555 I noticed you're missing the docker configuration in Readme.md. Is it not implemented or not documented? I manage to find the correct value by reading the config.ts file. Everything works fine ^^

Also I couldn't notice you removed the markdown in the message which can be still used with "parse_mode": "markdown" ^^

Yeh didn't fully update the documentation yet, will do that once i have a little more time. I though i had a reason to remove the parse_mode, but i will retry adding it and see what problems i encountered.
Edit: It probably isnt currently working since its still based on an old fingerprint version, it can use a rebase on newest version.

@Wunderharke Wunderharke mentioned this pull request Jun 8, 2021
@Wunderharke
Copy link

With thoughtful documentation, the Discord webhook could also be used with custom web servers to initiate appropriate actions.

@mikhail5555
Copy link
Contributor Author

With thoughtful documentation, the Discord webhook could also be used with custom web servers to initiate appropriate actions.

Yes, any url which accepts a post request which is equal or atleast a little comparable to discord one could be the endpoint of that call. That's the reason why I let the user edit the whole webhook url and not just the discord specific webhook keys.

However as I noticed many webhooks have just a slightly different protocol, so that's why I decided to make it named 'discord' and not just type 'webhook' since then you would need some more service specific webhook configuration.

@mikhail5555
Copy link
Contributor Author

Oh and I indeed also forward the 'account' to the notification handler, so it would indeed be trivial to make a notifier per account, the biggest problem I encountered on that part was that I was very hard to make it good user configurable without hardcoding it in the code.

@mikhail5555
Copy link
Contributor Author

@Wunderharke I added a tiny bit of placeholder logic for your feature request e68c582

@claabs claabs added the v4 label Aug 10, 2021
@claabs
Copy link
Owner

claabs commented Sep 29, 2021

So I think I finished the work for this in the v4 develop branch: cb8fd0c

Since I radically redesigned the config parsing, not much of the original code was left in-tact. However, the notifier config structures and each service implementation was very useful.

I'm pretty sure this PR should mark as merged when I release v4 to master. I really appreciate your work on this, and I'm glad I could finally get a look at it. Thanks, @mikhail5555

@mikhail5555
Copy link
Contributor Author

I'm happy it was useful!

@claabs claabs mentioned this pull request Oct 2, 2021
@claabs claabs merged commit cb8fd0c into claabs:master Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants