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.

Sign up for GitHub

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

Jump to bottom

fix(requests): handle when tvdbid is null #657

Merged
merged 12 commits into from
Jan 22, 2021
Merged

fix(requests): handle when tvdbid is null #657

merged 12 commits into from
Jan 22, 2021

Conversation

ankarhem
Copy link
Contributor

@ankarhem ankarhem commented Jan 17, 2021 •
edited

Todos

  • Handle missing tvdb id's errors and show them in frontend
  • Add get endpoint for looking up sonarr data with tmdb id
  • Add fallback posters
  • Add second step in requestmodal for media missing tvdbid, to confirm manual match
  • Translation Keys yarn i18n:extract
  • Sucessfully builds yarn build
  • Successfully starts yarn start 😉

Issues Fixed or Closed by this PR

@ankarhem ankarhem changed the title fix(requests): catch errors and show them in the frontend fix(requests): handle when tvdbid is null Jan 17, 2021
@ankarhem ankarhem force-pushed the handle-tvdbid-null-request-frontend branch from 31bd7a5 to 52e6016 Compare January 17, 2021 10:56
@ankarhem ankarhem marked this pull request as ready for review January 17, 2021 11:12
@ankarhem ankarhem force-pushed the handle-tvdbid-null-request-frontend branch 2 times, most recently from 813d29d to 7912862 Compare January 17, 2021 19:22
@lgtm-com
Copy link

lgtm-com bot commented Jan 18, 2021

This pull request introduces 1 alert when merging ef3c237 into b0ce040 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jan 19, 2021

This pull request introduces 2 alerts when merging a058de8 into 70229d1 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@ankarhem ankarhem force-pushed the handle-tvdbid-null-request-frontend branch from a058de8 to 8b8813f Compare January 19, 2021 21:54
@ankarhem
Copy link
Contributor Author

About the fallback posters, I was thinking maybe we could add it to the actual returned data from the api's instead of optionally at each use case? That would however limit us to using one fallback poster, instead of different ones like it is now. (With / without text). Let me know what you think.

@sct
Copy link
Owner

sct commented Jan 20, 2021

About the fallback posters, I was thinking maybe we could add it to the actual returned data from the api's instead of optionally at each use case? That would however limit us to using one fallback poster, instead of different ones like it is now. (With / without text). Let me know what you think.

Should be up to the client to handle missing posters. Otherwise, it's difficult to determine on the client that a poster is actually missing.

sct
sct reviewed Jan 21, 2021
View reviewed changes
server/entity/MediaRequest.ts Outdated Show resolved Hide resolved
server/entity/MediaRequest.ts Outdated Show resolved Hide resolved
server/routes/service.ts Outdated Show resolved Hide resolved
server/routes/service.ts Outdated Show resolved Hide resolved
src/components/RequestModal/SearchByNameModal/index.tsx Outdated Show resolved Hide resolved
src/components/RequestModal/SearchByNameModal/index.tsx Outdated Show resolved Hide resolved
@sct

This comment has been minimized.

Sign in to view
@sct
Copy link
Owner

sct commented Jan 21, 2021 •
edited

Also, there is no loading state while the Sonarr data is being fetched. Maybe add the loading spinner into the modal while it's fetching from Sonarr?

@ankarhem ankarhem force-pushed the handle-tvdbid-null-request-frontend branch from 4da34ed to e9aa920 Compare January 21, 2021 16:42
@sct sct force-pushed the handle-tvdbid-null-request-frontend branch from e9aa920 to 4da34ed Compare January 21, 2021 22:03
@lgtm-com
Copy link

lgtm-com bot commented Jan 21, 2021

This pull request introduces 1 alert when merging 67e94da into a3fe4e6 - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

@sct sct force-pushed the handle-tvdbid-null-request-frontend branch from 31e31bc to 935443f Compare January 22, 2021 01:28
@sct sct merged commit 2da0da8 into sct:develop Jan 22, 2021
@github-actions
Copy link

🎉 This PR is included in version 1.18.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
Reviewers

@sct sct sct approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No frontend handling of requests where tvdbId is null
2 participants
@ankarhem @sct