Improved error messaging for Update Channel API #5429
Conversation
| if(title.size() > 140) | ||
| { | ||
| ctx.channel->addMessage( | ||
| makeSystemMessage("Unable to set title: Title must be 140 characters or fewer.")); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Nerixyz is correct - the reason we try using server-side errors wherever possible is to ensure compatibility with API changes (e.g. if Twitch changes their max title length to 120 or 160)
There was a problem hiding this comment.
Removed the client side check, and added error handling for both the settitle and setgame since they are both utilizing the Update Channel API.
JakeRYW
changed the title
There was a problem hiding this comment.
This looks good, just a few things.
- Make sure you run
clang-formaton the files you edited (currently, it's not formatted correctly) - The Helix mock declaration needs to be updated - since the type now contains a comma, it has to be enclosed in parentheses:
(FailureCallback<HelixUpdateChannelError, QString> failureCallback).
src/providers/twitch/api/Helix.cpp
Outdated
| } | ||
| break; | ||
|
|
||
| case 409: |
There was a problem hiding this comment.
This seems wrong. Shouldn't 409 forward the message?
There was a problem hiding this comment.
According to the Modify Channel Information docs, 409 for this endpoint will give a Too Many Requests response when trying to see the Branded Content flag too frequently, which wouldn't be hit by /settitle or /settgame of course but I just tried to handle all errors currently provided in the docs.
There was a problem hiding this comment.
Oh, right. I read Too Many Requests and thought it would be 429. Probably should say 409 Conflict in the docs.
There was a problem hiding this comment.
Since it looks like it's just a typo in the docs, I'll remove the 409, and leave it to just 429.
|
|
||
| case Error::Unknown: | ||
| default: { | ||
| errorMessage += "An unknown error has occured."; |
There was a problem hiding this comment.
This should include the message - like "Failed to set title - An unknown error has occurred ({message})" (there's also a typo in occured → occurred, which is also present in the error handling of manageAutoModMessages and the comment for secondsSinceDisconnect).
| using Error = HelixUpdateChannelError; | ||
|
|
||
| switch(error) | ||
| { | ||
| case Error::UserMissingScope: { | ||
| errorMessage += "Missing required scope. " | ||
| "Re-login with your " | ||
| "account and try again."; | ||
| } | ||
| break; | ||
|
|
||
| case Error::UserNotAuthorized: { | ||
| errorMessage += "You must be the broadcaster " | ||
| "to set the game."; | ||
| } | ||
| break; | ||
|
|
||
| case Error::Ratelimited: { | ||
| errorMessage += | ||
| "You are being ratelimited by Twitch. Try " | ||
| "again in a few seconds."; | ||
| } | ||
| break; | ||
|
|
||
| case Error::Forwarded: { | ||
| errorMessage += message; | ||
| } | ||
| break; | ||
|
|
||
| case Error::Unknown: | ||
| default: { | ||
| errorMessage += "An unknown error has occured."; | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
This is almost identical to the code above and should be refactored into a function (in an anonymous namespace above chatterino::commands).
There was a problem hiding this comment.
Thank you for your contribution! 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.
pajlada
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| std::function<void(NetworkResult)> successCallback, | ||
| HelixFailureCallback failureCallback), | ||
| (override)); | ||
| MOCK_METHOD( |
There was a problem hiding this comment.
warning: invalid case style for function 'MOCK_METHOD' [readability-identifier-naming]
| MOCK_METHOD( | |
| mockMethod( |
| /// Error type for Helix::updateChannel | ||
| /// | ||
| /// Used in the /settitle and /setgame commands | ||
| enum class HelixUpdateChannelError { |
There was a problem hiding this comment.
warning: enum 'HelixUpdateChannelError' uses a larger base type ('int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
enum class HelixUpdateChannelError {
^
Adding appropriate error handling for the
/settitleand/setgamecommands. Handles trying to set the title/game in unauthorized channels, missing scope, rate limits, and title being too long by providingFailed to set title - Title is too long.error message.Also removed the
statusvariable declaration from thesetTitlefunction as it was not used.Fixes #5332