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

add "/shoutout" command #4638

Merged
merged 20 commits into from
May 20, 2023
Merged

add "/shoutout" command #4638

merged 20 commits into from
May 20, 2023

Conversation

olafyang
Copy link
Contributor

@olafyang olafyang commented May 15, 2023

Description

This pr adds a new "/shoutout" command which calls the helix api and shouts out the specified user.

Add:

  • /shoutout <username> calls chat/shoutouts of helix api and gives specified user a twitch shoutout.

It seems like twitch is returning a 500 for all errors excluding 429 rate limit . Trying to shoutout an user who's not a mod gives back to following in the log. Therefore I can not provide more detailed error message for now.

chatterino.twitch: Helix send shoutout, unhandled error data: 500 "{\"error\":\"Internal Server Error\",\"status\":500,\"message\":\"\"}" QJsonObject({"error":"Internal Server Error","message":"","status":500})

@Felanbird
Copy link
Collaborator

Felanbird commented May 15, 2023

It seems like twitch is returning a 500 for all errors excluding 429 rate limit

I'm only able to replicate this issue when the shoutout sender is not a mod, I receive back a correct 401 when the moderator_id does not match the relevant oauth token, as well as a 400 when from_broadcaster_id is not live/has no viewers.

twitchdev/issues#779

src/providers/twitch/api/Helix.hpp Outdated Show resolved Hide resolved
tests/src/HighlightController.cpp Outdated Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Outdated Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Outdated Show resolved Hide resolved
@olafyang
Copy link
Contributor Author

It seems like twitch is returning a 500 for all errors excluding 429 rate limit

I'm only able to replicate this issue when the shoutout sender is not a mod, I receive back a correct 401 when the moderator_id does not match the relevant oauth token, as well as a 400 when from_broadcaster_id is not live/has no viewers.

twitchdev/issues#779

i'll check on my end too and make changes, thanks!!

@olafyang
Copy link
Contributor Author

Typos are fixed in e52f8bb.

I've also added some error handling for the api.

@olafyang olafyang requested a review from Felanbird May 16, 2023 00:17
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Looks good overall, some nitpicks with error handling & where the code is located

src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Outdated Show resolved Hide resolved
src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved
src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved
@pajlada pajlada added the Waiting for author PR bounced back to author label May 19, 2023
@olafyang olafyang requested review from pajlada and Mm2PL May 19, 2023 18:07
@Felanbird Felanbird added Waiting for review PR bounced back to reviewer and removed Waiting for author PR bounced back to author labels May 20, 2023
@olafyang
Copy link
Contributor Author

oops i forgot to return at the anon check and the argument check, i'll fix them in the evening once i get home

@pajlada
Copy link
Member

pajlada commented May 20, 2023

Currently fixing the merge conflicts, will solve the returns in the same batch

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Looks good to me, works as expected in the few scenarios I tested it

When you've taken a look at the changes @olafyang we can go ahead and merge this in

@pajlada pajlada added Waiting for author PR bounced back to author and removed Waiting for review PR bounced back to reviewer labels May 20, 2023
@olafyang
Copy link
Contributor Author

yep looks fantastic!! please go ahead and merge it. i learned a lot from this, thanks ❤️❤️ would love to contribute some more in the future

@pajlada pajlada enabled auto-merge (squash) May 20, 2023 11:35
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@pajlada pajlada added Waiting for review PR bounced back to reviewer and removed Waiting for author PR bounced back to author labels May 20, 2023
@pajlada pajlada merged commit 21d4b2c into Chatterino:master May 20, 2023
@pajlada
Copy link
Member

pajlada commented May 20, 2023

Thank you! As a first-time contributor, you can now add yourself to the contributors list that's shown inside the About page in Chatterino.

If you want this, you can open a new PR where you modify the resources/contributors.txt file and add yourself to the list. Make sure to read the comments at the top for instructions.

@Felanbird
Copy link
Collaborator

Also you'll want to do it on a branch after you clean up your master branch, as creating this PR from your master branch has made a mess for you 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for review PR bounced back to reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants