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: simple failed request handling #474

Merged
merged 1 commit into from
Dec 25, 2020
Merged

Conversation

johnpyp
Copy link
Contributor

@johnpyp johnpyp commented Dec 23, 2020

Description

When a movie or series is added with radarr or sonarr, if it fails, this changes the media state to unknown and sends a notification to admins.

Client side this will look like a failed state along with a retry button, which when clicked will delete the request and re-add it.

Screenshot (if UI related)

React Request List
Discord Notification

Todos

  • Sucessfully builds yarn build
  • Translation Keys yarn i18n:extract
  • Database migration created (if required)

Issues Fixed or Closed by this PR

@sct
Copy link
Owner

sct commented Dec 23, 2020

I just thought of one more thing. If someone deletes all requests associated with any media, we keep the media but set its status back to Unknown. That would mark it as failed, but it isn't.

@johnpyp
Copy link
Contributor Author

johnpyp commented Dec 23, 2020

I just thought of one more thing. If someone deletes all requests associated with any media, we keep the media but set its status back to Unknown. That would mark it as failed, but it isn't.

So the idea is that, if all requests are deleted with any media, it wouldn't matter if a request connected to it would be marked as failed, since there's no request to actually see as failed. However, when there are requests and the media is still unknown, that signifies it's failed since that shouldn't be a possible state when the media is in Radarr/Sonarr or available.

@sct
Copy link
Owner

sct commented Dec 23, 2020

I just thought of one more thing. If someone deletes all requests associated with any media, we keep the media but set its status back to Unknown. That would mark it as failed, but it isn't.

So the idea is that, if all requests are deleted with any media, it wouldn't matter if a request connected to it would be marked as failed, since there's no request to actually see as failed. However, when there are requests and the media is still unknown, that signifies it's failed since that shouldn't be a possible state when the media is in Radarr/Sonarr or available.

Ah yeah true. Since there are no requests it won't show up in the request list so it doesn't really matter.

@johnpyp
Copy link
Contributor Author

johnpyp commented Dec 24, 2020

One thing I didn't consider about setting to Pending and then back to Approved are the notifications there... might be better just to include a small refactor for a dedicated endpoint.

@sct
Copy link
Owner

sct commented Dec 24, 2020

One thing I didn't consider about setting to Pending and then back to Approved are the notifications there... might be better just to include a small refactor for a dedicated endpoint.

The dedicated endpoint would just try to send the media back to radarr/sonarr again, yes? Because if we touch the statuses on the request it will still refire the listeners and trigger notifications.

@johnpyp johnpyp force-pushed the feat/failed-request branch 2 times, most recently from 6b91242 to b0208ed Compare December 24, 2020 03:07
@johnpyp
Copy link
Contributor Author

johnpyp commented Dec 24, 2020

Yeah, so I added a POST /request/:requestId/:command endpoint (like the status endpoint), with just retry as the only command for now. The retry itself will

  1. updateParentStatus (to effectively reset statuses as if it was a new request)
  2. Attempt to send the media to Sonarr/Radarr.

This is the same as the normal AfterUpdate hooks, with the exception of not sending notifications.

Let me know if I should make this endpoint dedicated for retry instead, I figured it might be something that comes with more uses in the future, but you might prefer many overloads rather than a single endpoint.

@sct
Copy link
Owner

sct commented Dec 24, 2020

I think having its own endpoint is better. Right now you are setting an action in an endpoint that is supposed to accept a status. Even if its an overload, the docs will be clearer in that regard.

overseerr-api.yml Outdated Show resolved Hide resolved
Comment on lines 39 to 44
case MediaStatus.UNKNOWN:
return (
<Badge badgeType="danger">
{intl.formatMessage(globalMessages.failed)}
</Badge>
);
Copy link
Owner

Choose a reason for hiding this comment

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

This will cause the failed status badge to show up in many place throughout the app, not just the RequestList.

RequestCard uses this. So does the movie/tv details pages now.


const messages = defineMessages({
requestedby: 'Requested by {username}',
seasons: 'Seasons',
notavailable: 'N/A',
failedretry: 'Something went wrong retrying that request',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
failedretry: 'Something went wrong retrying that request',
failedretry: 'Something went wrong retrying the request',

When a movie or series is added with radarr or sonarr, if it fails, this changes the media state to
unknown and sends a notification to admins. Client side this will look like a failed state along
with a retry button that will delete the request and re-queue it.
Copy link
Owner

@sct sct left a comment

Choose a reason for hiding this comment

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

LGTM!

@sct sct merged commit 02969d5 into sct:develop Dec 25, 2020
Beta Release Roadmap automation moved this from In progress to Done Dec 25, 2020
@github-actions
Copy link

🎉 This PR is included in version 1.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants