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

fix(logging): handle media server connection refused error/toast #748

Merged
merged 8 commits into from
May 23, 2024

Conversation

Fallenbagel
Copy link
Owner

@Fallenbagel Fallenbagel commented May 13, 2024

Description

Properly log as connection refused if the jellyfin/emby server is unreachable. Previously it used to throw a credentials error which lead to a lot of confusion.

Screenshot (if UI-related)

image
image

To-Dos

  • Successful build yarn build
  • Translation keys yarn i18n:extract
  • Database migration (if required)

Issues Fixed or Closed

  • Fixes #XXXX

Properly log as connection refused if the jellyfin/emby server is unreachable. Previously it used to
throw a credentials error which lead to a lot of confusion
server/routes/auth.ts Fixed Show fixed Hide fixed
@Fallenbagel Fallenbagel requested a review from Jumail May 13, 2024 15:47
@Fallenbagel Fallenbagel marked this pull request as draft May 13, 2024 19:44
@Fallenbagel
Copy link
Owner Author

Fallenbagel commented May 13, 2024

TODO:

  • refactor this PR to use custom error types and error codes instead of abusing error messages

@Fallenbagel Fallenbagel marked this pull request as ready for review May 17, 2024 23:25
server/types/error.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

I don't want to be annoying, but why rename to NetworkError? An authentication error isn't really a network related error?
ApiError seemed fine to me.

@Fallenbagel
Copy link
Owner Author

Fallenbagel commented May 23, 2024

Oh so for other errors that aren't api related, we make more classes and enums? 🤔

I don't want to be annoying

Don't be, any feedback is really appreciated

@gauthier-th
Copy link
Collaborator

Oh so for other errors that aren't api related, we make more classes and enums? 🤔

I was thinking that any error returned by the server would be ApiError, since our server is the api?

@Fallenbagel
Copy link
Owner Author

Oh so for other errors that aren't api related, we make more classes and enums? 🤔

I was thinking that any error returned by the server would be ApiError, since our server is the api?

What about external api requests

@gauthier-th
Copy link
Collaborator

Oh so for other errors that aren't api related, we make more classes and enums? 🤔

I was thinking that any error returned by the server would be ApiError, since our server is the api?

What about external api requests

External API requests are always made by the server anyway?
I would have thought that we have only one type of error returned by all the endpoints of the server.

@Fallenbagel
Copy link
Owner Author

External API requests are always made by the server anyway?
I would have thought that we have only one type of error returned by all the endpoints of the server.

Makes sense. Let me update real quick

Copy link
Collaborator

@gauthier-th gauthier-th left a comment

Choose a reason for hiding this comment

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

LGTM!

@Fallenbagel Fallenbagel merged commit f486fb5 into develop May 23, 2024
7 checks passed
Fallenbagel added a commit to gauthier-th/jellyseerr that referenced this pull request May 23, 2024
…lenbagel#748)

* fix(logging): handle media server connection refused error/toast

Properly log as connection refused if the jellyfin/emby server is unreachable. Previously it used to
throw a credentials error which lead to a lot of confusion

* refactor(i8n): extract translation keys

* refactor(auth): error message for a more consistent format

* refactor(auth/errors): use custom error types and error codes instead of abusing error messages

* refactor(i8n): replace connection refused translation key with invalidurl

* fix(error): combine auth and api error class into a single one called network error

* fix(error): use the new network error and network error codes in auth/api

* refactor(error): rename NetworkError to ApiError
Fallenbagel added a commit that referenced this pull request May 23, 2024
* fix(api): save user email on the first try

fix #227

* fix(api): remove todo

* fix(logging): handle media server connection refused error/toast (#748)

* fix(logging): handle media server connection refused error/toast

Properly log as connection refused if the jellyfin/emby server is unreachable. Previously it used to
throw a credentials error which lead to a lot of confusion

* refactor(i8n): extract translation keys

* refactor(auth): error message for a more consistent format

* refactor(auth/errors): use custom error types and error codes instead of abusing error messages

* refactor(i8n): replace connection refused translation key with invalidurl

* fix(error): combine auth and api error class into a single one called network error

* fix(error): use the new network error and network error codes in auth/api

* refactor(error): rename NetworkError to ApiError

---------

Co-authored-by: Fallenbagel <98979876+Fallenbagel@users.noreply.github.com>
@Fallenbagel
Copy link
Owner Author

🎉 This PR is included in version 1.9.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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants