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

feat: twitch chat notification, multiple telegram chat ids and german web shop overhaul #528

Merged
merged 67 commits into from Oct 21, 2020
Merged

Conversation

PaddseL
Copy link
Contributor

@PaddseL PaddseL commented Oct 15, 2020

Description

Resolves #530

feat: allow multiple telegram chats to be notified
feat: add notifications to twitch channel chats

feat: add computeruniverse (de)
feat: add cyberport (de) shop
feat: add mindfactory (de) shop
feat: add saturn (de) shop (see @fubu2k's pr)

chore: normalize model names for various german stores
chore: add and sort links for various german stores
chore: removed dead links from various german stores
chore: shortened links for proshop-de

sorry for this huge pr, i somehow forgot to create one after each feature as i was constantly adding and testing.

Testing

telegram notifications work this way for either a single chat id or comma seperated chat ids,
as telegram.chatId.split(',') returns the original string if no comma is found.

tested twitch notifications and found this way to be most stable. first i tried to only connect in an event of an alert and
immediately disconnect when the message got sent, but this introduced some weird behavior and the ChatClient with
the RefreshableAuthProvider should maintain the connection or reconnect in case of an connection loss.
the setup of this notification method is a bit laborious though, maybe this should be mentioned in the README.
i'm open for discussion on this one and would like to hear your opinion.

i found it to be more reliable to check for outOfStock instead of inStock for most german webshops, as they rarely list new items as "in stock" rather than more something like "available in x days" and accept preorders, so they don't even get to the "in stock" state.

New dependencies

"twitch": "^4.2.6",
"twitch-auth": "^4.2.6",
"twitch-chat-client": "^4.2.6"

@PaddseL
Copy link
Contributor Author

PaddseL commented Oct 17, 2020

i think i came up with a good workaround:
using events now to send messages to twitch chat, npm run test:notification works now like production und it is overall a more cleaner solution.

no need to wrap the instantiation of chatClient in a check now as it only matters when executing chatClient.connect().

Please let me know what you think :)

@johnconnor2020
Copy link

THIS IS AWESOME

jef
jef previously approved these changes Oct 20, 2020
Copy link
Owner

@jef jef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HOLY smokes that's a lot of products... Thanks for the great add. I have a few comments there, but that shouldn't hold back this PR any futher.

Thanks for implementing this!

I don't know all of the use cases for Twitch, but I also don't use it that often, so if it makes sense, then great!

logger.error('✖ couldn\'t send telegram message', error);
for (const chatId of telegram.chatId) {
try {
results.push(client.sendMessage(chatId, `${Print.inStock(link, store)}\n${givenUrl}`));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should make this async as well. But that's OK, we can do that in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 16 - 30 is already wrapped in an async, do you mean the body of the for loop should be async aswell?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I mean that we should wrap with Promise or something.

Kind of like this:

https://github.com/jef/nvidia-snatcher/blob/8098a310925edb5c8738b00259856a2d64e2fec7/src/notification/discord.ts#L30-L35

But I don't even know if sendMessage returns a Promise, so I really don't know if this would work.

src/store/model/alternate.ts Show resolved Hide resolved
@jef
Copy link
Owner

jef commented Oct 20, 2020

If you're able to resolve these conflicts, we can get this in!

Thank you!

@johnconnor2020
Copy link

There seems to be an error when checking the zotac card on MediaMarkt and Saturn store:

warn :: ✖ [mediamarkt] [zotac (3080)] amp extreme holo :: STATUS CODE ERROR 404

@PaddseL
Copy link
Contributor Author

PaddseL commented Oct 21, 2020

I don't know all of the use cases for Twitch, but I also don't use it that often, so if it makes sense, then great!

I saw a streamer on twitch who is streaming this project for the na/ca market, i thought it would be good idea to do this for the german market as well.
There are many people who want a rtx 30 series card, but don't have enough technical knowledge to run this project, this way they also have a chance of getting one. :)
It's much easier for the viewers to click on a link in the chat instead of copying the link char by char from the stream and someone in my chat requested this, so i implemented it.
Now, people can stream this for various other markets/regions! :)

@johnconnor2020
Copy link

I don't know all of the use cases for Twitch, but I also don't use it that often, so if it makes sense, then great!

I saw a streamer on twitch who is streaming this project for the na/ca market, i thought it would be good idea to do this for the german market as well.
There are many people who want a rtx 30 series card, but don't have enough technical knowledge to run this project, this way they also have a chance of getting one. :)
It's much easier for the viewers to click on a link in the chat instead of copying the link char by char from the stream and someone in my chat requested this, so i implemented it.
Now, people can stream this for various other markets/regions! :)

that sounds great. is he streaming 24/7? do you have a link?

@PaddseL
Copy link
Contributor Author

PaddseL commented Oct 21, 2020

that sounds great. is he streaming 24/7? do you have a link?

as far as i know, 24/7

stream for na/ca: https://www.twitch.tv/falcodrin
my stream for germany: https://www.twitch.tv/PaddseL

@johnconnor2020
Copy link

asus store gives me error, i think they changed some stuff on their shop.

@jef
Copy link
Owner

jef commented Oct 21, 2020

Thank you very much!

@jef jef merged commit 675f13a into jef:main Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Twitch support?
5 participants