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

Improve warning and missing reports for Radarr connections #809

Merged
merged 5 commits into from
Nov 2, 2024

Conversation

Bothari
Copy link
Contributor

@Bothari Bothari commented Oct 4, 2024

Description

Radarr uses a different method of reporting "missing" content, which tracks the availability of the items. Before this change, the Radarr service would display "missing" count for movies which are years away from release. With this change, it will now only display "missing" count for movies which have been released and should be available per the user settings defined in Radarr.

I also improved the "warning" handling to include queue warnings and errors. For example if a movie download has stalled for some reason, it will show the warning.

Fixes #638, "Add Wanted count to Arrs"
#638

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've read & comply with the contributing guidelines
  • I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • I have made corresponding changes to the documentation (README.md).
  • I've checked my modifications for any breaking changes, especially in the config.yml file

Copy link
Owner

@bastienwirtz bastienwirtz left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @Bothari, a few question but it looks fine 👍

).length;
this.fetch(`${this.apiPath}/wanted/missing?pageSize=1&apikey=${this.item.apikey}`)
.then((overview) => {
this.fetch(`${this.apiPath}/wanted/missing?pageSize=${overview.totalRecords}&apikey=${this.item.apikey}`)
Copy link
Owner

Choose a reason for hiding this comment

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

Not a fan of the double request to avoid handling the pagination but it doesn't seems to have any limit on the pageSize, so I guess it should be fine!

Indentation looks funny, could you rebase your PR to get the latest changes and run pnpm lint ? Thanks 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't love the double request method, but it seemed like it would be more responsive that handling the pagination. Do you have a policy of respecting pagination in other parts of the codebase? If so then I am happy to update it to do so.

I think I've fixed the indentation (I was using tabs instead of spaces!) but full disclosure, this is the first time I've ever done a PR on GitHub. 20+ years experience as a software engineer but I've never done a PR in Git.

Copy link
Owner

Choose a reason for hiding this comment

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

There is no specific policy regarding pagination, let keep it like that, implementing the proper pagination would be overkill.

Congrats on your first PR @Bothari 🎉! And thanks A LOT for contributing to open source. It's really appreciated, especially for Homer, I can't test and maintain all the custom cards myself because I don't use all the services (this is a good example, I don't use Radarr!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I also just committed some bad code and managed to revert it. 😂

I think Homer is a fantastic tool BTW, so congratulations on it. Really simple to use and extend. Love it.

@@ -77,6 +77,17 @@ export default {
}
})
.catch(handleError);
this.fetch(`${this.apiPath}/queue/details?apikey=${this.item.apikey}`)
Copy link
Owner

Choose a reason for hiding this comment

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

Just to be sure, is the /queue/details endpoint exists on the old API ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've no idea where to find documentation on the old API. Should I just wrap this new section in if (!this.item.legacyApi) in case?

Copy link
Owner

Choose a reason for hiding this comment

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

I couldn't find it either :(
I don't even know if the legacy API is used anymore. Let's put the if as you suggest for now if that's ok for you 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done this! Cheers.

Copy link
Owner

@bastienwirtz bastienwirtz left a comment

Choose a reason for hiding this comment

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

Thanks for your work @Bothari !

@bastienwirtz bastienwirtz merged commit 1febbad into bastienwirtz:main Nov 2, 2024
5 checks passed
@Bothari
Copy link
Contributor Author

Bothari commented Nov 2, 2024

Thanks for your work @Bothari !

Hell yes! Thank you!

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.

Add Wanted count to Arrs
2 participants