refactor(jellyfin): abstract jellyfin hostname, updated ui to reflect it, better validation #773
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… reflect it, better validation This PR refactors and abstracts jellyfin hostname into, jellyfin ip, jellyfin port, jellyfin useSsl, and jellyfin urlBase. This makes it more consistent with how plex settings are stored as well. In addition, this improves validation as validation can be applied seperately to them instead of as one whole regex doing the work to validate the url. UI was updated to reflect this. BREAKING CHANGE: Jellyfin settings now does not include a hostname. Instead it abstracted it to ip, port, useSsl, and urlBase. However, migration of old settings to new settings should work automatically.
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
gauthier-th
requested changes
May 25, 2024
github-actions
bot
removed
the
merge conflict
…alid This should fix the issue where settings are saved even if the url was invalid. Now the settings will only be saved if the url is valid. Sort of like a test connection.
fallenbagel
force-pushed
the
refactor-jellyfin-settings
branch
from
e84d5f2 to
43e0a29
Compare
gauthier-th
previously approved these changes
May 27, 2024
I will be creating a preview tag for this and try to get people to test this |
3 tasks
|
This pr should be available to test using the tag: |
gauthier-th
reviewed
May 28, 2024
|
I am getting a |
Could you share logs? |
|
No logs get generated in the logs folder. |
|
In console log I get this |
A new preview tag should be available in about half an hour with a fix (when all the checks in this PR turns green). You'll just need to pull it. |
|
Sounds good, let me know when it's updated. |
3 tasks
Gauvino
approved these changes
May 31, 2024
fallenbagel
force-pushed
the
refactor-jellyfin-settings
branch
from
74a522d to
ce04315
Compare
fallenbagel
changed the title
cypress settings were lacking some of the jobs so when the startJobs() is called when the app starts, it was failing to schedule the jobs where their cron timings were not specified in the cypress settings. Therefore, this commit adds those jobs back. In addition, other setting options were added to keep cypress settings consistent with a normal user.
… it does not need prettier
…error format check locally passes on this file. However, it fails during the github actions format check. Therefore, json language features formatter was run instead of prettier to see if that fixes the issue.
fallenbagel
force-pushed
the
refactor-jellyfin-settings
branch
from
5ad99ee to
62f97b3
Compare
fallenbagel
added a commit
that referenced
this pull request
Jun 1, 2024
This is to try and fix formatting issues on #773 on a file that should be ignored.
3 tasks
fallenbagel
added a commit
that referenced
this pull request
Jun 1, 2024
This is to try and fix formatting issues on #773 on a file that should be ignored.
added back the rest of the cypress settings and removed cypress settings from .prettierignore
|
@sgtcoder could you test it one more time please? |
gauthier-th
reviewed
Jun 1, 2024
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
github-actions
bot
removed
the
merge conflict
…t fails in settings page
gauthier-th
approved these changes
Jun 13, 2024
bonswouar
pushed a commit
to bonswouar/jellyseerr
that referenced
this pull request
Jun 14, 2024
… it, better validation (fallenbagel#773) * refactor(jellyfinsettings): abstract jellyfin hostname, updated ui to reflect it, better validation This PR refactors and abstracts jellyfin hostname into, jellyfin ip, jellyfin port, jellyfin useSsl, and jellyfin urlBase. This makes it more consistent with how plex settings are stored as well. In addition, this improves validation as validation can be applied seperately to them instead of as one whole regex doing the work to validate the url. UI was updated to reflect this. BREAKING CHANGE: Jellyfin settings now does not include a hostname. Instead it abstracted it to ip, port, useSsl, and urlBase. However, migration of old settings to new settings should work automatically. * refactor: remove console logs and use getHostname and ApiErrorCodes * fix: store req.body jellyfin settings temporarily and store only if valid This should fix the issue where settings are saved even if the url was invalid. Now the settings will only be saved if the url is valid. Sort of like a test connection. * refactor: clean up commented out code * refactor(i18n): extract translation keys * fix(auth): auth failing with jellyfin login is disabled * fix(settings): jellyfin migrations replacing the rest of the settings * fix(settings): jellyfin hostname should be carried out if hostname exists * fix(settings): merging the wrong settings source * refactor(settings): use migrator for dynamic settings migrations * refactor(settingsmigrator): settings migration handler and the migrations * test(cypress): fix cypress tests failing cypress settings were lacking some of the jobs so when the startJobs() is called when the app starts, it was failing to schedule the jobs where their cron timings were not specified in the cypress settings. Therefore, this commit adds those jobs back. In addition, other setting options were added to keep cypress settings consistent with a normal user. * chore(prettierignore): ignore cypress/config/settings.cypress.json as it does not need prettier * chore(prettier): ran formatter on cypress config to fix format check error format check locally passes on this file. However, it fails during the github actions format check. Therefore, json language features formatter was run instead of prettier to see if that fixes the issue. * test(cypress): add only missing jobs to the cypress settings * ci: attempt at trying to get formatter to pass on cypress config json file * refactor: revert the changes brought to try and fix formatter added back the rest of the cypress settings and removed cypress settings from .prettierignore * refactor(settings): better erorr logging when jellyfin connection test fails in settings page
|
🎉 This PR is included in version 2.0.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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Warning
BREAKING CHANGE: Jellyfin settings now does not include a hostname. Instead it abstracted it to ip, port, useSsl, and urlBase. However, migration of old settings to new settings should work automatically.
Description
This PR refactors and abstracts jellyfin hostname into, jellyfin ip, jellyfin port, jellyfin useSsl, and jellyfin urlBase. This makes it more consistent with how plex settings are stored as well. In addition, this improves validation as validation can be applied seperately to them instead of as one whole regex doing the work to validate the url.
UI was updated to reflect this.
Screenshot (if UI-related)
To-Dos
yarn buildyarn i18n:extract- [ ] Database migration (if required)Issues Fixed or Closed