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
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/components/services/Radarr.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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.

.then((queue) => {
for (var i = 0; i < queue.length; i++) {
if (queue[i].trackedDownloadStatus == "warning") {
this.warnings++;
} else if (queue[i].trackedDownloadStaus == "error") {
this.errors++;
}
}
})
.catch(handleError);
this.fetch(`${this.apiPath}/queue?apikey=${this.item.apikey}`)
.then((queue) => {
this.activity = 0;
Expand All @@ -93,11 +104,14 @@ export default {
})
.catch(handleError);
if (!this.item.legacyApi) {
this.fetch(`${this.apiPath}/movie?apikey=${this.item.apikey}`)
.then((movies) => {
this.missing = movies.filter(
(m) => m.monitored && !m.hasFile
).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.

.then((movies) => {
this.missing = movies.records.filter(
(m) => m.monitored && m.isAvailable && !m.hasFile
).length;
})
})
.catch(handleError);
}
Expand Down