From ff06a1a58168df0d10b3e8e1bca3e4c83a86cf8e Mon Sep 17 00:00:00 2001 From: CaliBrain Date: Mon, 24 Aug 2026 18:11:29 -0400 Subject: [PATCH] fix(search): follow-ups to per-user book languages (#1267) Review follow-ups to #1255, all in the code that PR touched. Drop the dead user_id from the Prowlarr retry path. ProwlarrSource.search never reads plan.languages, and _refresh_release builds a synthetic book with no titles_by_language, so the title variants came out identical with and without it. It also should not language-filter: it re-finds one exact release by its guid. Pin the tab move in tests. BOOK_LANGUAGE moved from the General tab to Search Mode with no migration, which only works because both tabs persist into the same settings.json. Nothing asserted that, so splitting the files later would silently reset every install to ["en"]. Covers the stored value, a fresh install, and ENV precedence. Stop the UI inventing a default language. An empty BOOK_LANGUAGE is a deliberate "no default filter" that the backend preserves, but the two frontend call sites replaced it with the first supported language, so the filter said English where the server filtered nothing. resolveDefaultLanguageCodes now falls back only when the value is absent. Keep the normalized value for every validated search key. validate_user_settings gated the write-back on a hand-maintained subset of the keys the search validator recognises, so METADATA_PROVIDER_COMBINED, SHOW_COMBINED_SELECTOR and FORCE_COMBINED_SEARCH were validated and then stored raw -- a padded provider name was accepted and persisted with its padding. Reuse the validator's own key set instead. Skip blank language entries rather than rejecting them, so "" and "en," mean the same as [] and ["en"] instead of erroring on an unnamed language. Extract resolveListOverride for the list-override detection that was copy-pasted between the two user-settings sections, and mention languages in the Search Preferences section description. --- shelfmark/config/users_settings.py | 18 ++++--- shelfmark/core/admin_settings_routes.py | 16 +++--- shelfmark/release_sources/prowlarr/handler.py | 3 +- src/frontend/src/App.tsx | 6 +-- .../components/UrlSearchBootstrapMount.tsx | 9 ++-- .../settings/users/UserOverridesSection.tsx | 21 +++----- .../users/UserSearchPreferencesSection.tsx | 16 +++--- .../components/settings/users/fieldHelpers.ts | 22 +++++++- .../src/tests/languageFilters.test.ts | 34 ++++++++++++ src/frontend/src/utils/languageFilters.ts | 18 +++++++ tests/config/test_search_mode_settings.py | 53 +++++++++++++++++++ tests/config/test_users_settings.py | 2 +- tests/core/test_admin_users_api.py | 40 ++++++++++++++ 13 files changed, 207 insertions(+), 51 deletions(-) diff --git a/shelfmark/config/users_settings.py b/shelfmark/config/users_settings.py index c54f36c3..3e646275 100644 --- a/shelfmark/config/users_settings.py +++ b/shelfmark/config/users_settings.py @@ -62,7 +62,7 @@ _SELF_SETTINGS_SECTION_OPTIONS = [ { "value": "search", "label": "Search Preferences", - "description": "Show personal search mode and provider settings.", + "description": "Show personal search mode, language, and provider settings.", }, { "value": "notifications", @@ -78,7 +78,7 @@ _SEARCH_PREFERENCE_PROVIDER_KEYS = { "METADATA_PROVIDER_AUDIOBOOK", "METADATA_PROVIDER_COMBINED", } -_SEARCH_PREFERENCE_VALIDATABLE_KEYS = { +SEARCH_PREFERENCE_VALIDATABLE_KEYS = { "SEARCH_MODE", "BOOK_LANGUAGE", "DEFAULT_RELEASE_SOURCE", @@ -183,9 +183,11 @@ def _get_request_policy_rule_columns() -> list[dict[str, object]]: def _validate_book_languages(value: Any) -> tuple[Any, str | None]: """Validate a per-user default language list against the known languages. - Accepts the list the settings UI sends as well as the comma-separated string the - env var uses. An empty list is a deliberate override meaning "no default language - filter", so it is kept as-is; ``None`` clears the override further up the chain. + Accepts the list the settings UI sends as well as a comma-separated string, so an + API client can spell the value the way the env var does. Blank entries are skipped + rather than rejected, which makes "" and "en," mean the same as [] and ["en"]. An + empty result is a deliberate override meaning "no default language filter", so it + is kept as-is; ``None`` clears the override further up the chain. """ entries = value.split(",") if isinstance(value, str) else value if not isinstance(entries, (list, tuple)): @@ -193,6 +195,8 @@ def _validate_book_languages(value: Any) -> tuple[Any, str | None]: normalized: list[str] = [] for entry in entries: + if entry is None or (isinstance(entry, str) and not entry.strip()): + continue code = normalize_language(entry) if code is None: return value, f"BOOK_LANGUAGE contains an unsupported language: {entry}" @@ -204,7 +208,7 @@ def _validate_book_languages(value: Any) -> tuple[Any, str | None]: def validate_search_preference_value(key: str, value: Any) -> tuple[Any, str | None]: """Validate and normalize a search preference value for user overrides.""" - if key not in _SEARCH_PREFERENCE_VALIDATABLE_KEYS: + if key not in SEARCH_PREFERENCE_VALIDATABLE_KEYS: return value, None if value is None: @@ -325,7 +329,7 @@ def _on_save_users(values: dict[str, object]) -> dict[str, object]: } values["REQUEST_POLICY_RULES"] = normalized_rules - for key in _SEARCH_PREFERENCE_VALIDATABLE_KEYS: + for key in SEARCH_PREFERENCE_VALIDATABLE_KEYS: if key not in values: continue normalized_value, validation_error = validate_search_preference_value(key, values[key]) diff --git a/shelfmark/core/admin_settings_routes.py b/shelfmark/core/admin_settings_routes.py index a5c2f0a7..b4d33905 100644 --- a/shelfmark/core/admin_settings_routes.py +++ b/shelfmark/core/admin_settings_routes.py @@ -11,7 +11,10 @@ from shelfmark.config.notifications_settings import ( is_valid_notification_url, normalize_notification_routes, ) -from shelfmark.config.users_settings import validate_search_preference_value +from shelfmark.config.users_settings import ( + SEARCH_PREFERENCE_VALIDATABLE_KEYS, + validate_search_preference_value, +) from shelfmark.core.config import config as app_config from shelfmark.core.request_policy import parse_policy_mode, validate_policy_rules from shelfmark.core.settings_registry import load_config_file @@ -91,14 +94,9 @@ def validate_user_settings( if search_validation_error: errors.append(search_validation_error) continue - if key in { - "SEARCH_MODE", - "BOOK_LANGUAGE", - "METADATA_PROVIDER", - "METADATA_PROVIDER_AUDIOBOOK", - "DEFAULT_RELEASE_SOURCE", - "DEFAULT_RELEASE_SOURCE_AUDIOBOOK", - }: + # Every key the search validator recognises keeps its normalized value; + # a hand-maintained subset here silently dropped normalization for the rest. + if key in SEARCH_PREFERENCE_VALIDATABLE_KEYS: valid[key] = normalized_search_value continue diff --git a/shelfmark/release_sources/prowlarr/handler.py b/shelfmark/release_sources/prowlarr/handler.py index 9ea51914..cf432ad7 100644 --- a/shelfmark/release_sources/prowlarr/handler.py +++ b/shelfmark/release_sources/prowlarr/handler.py @@ -297,10 +297,11 @@ class ProwlarrHandler(ExternalClientHandler): search_title=title, search_author=task.author, ) + # No language default here on purpose: this re-finds one exact release by its + # guid, and Prowlarr does not filter on plan.languages anyway. plan = build_release_search_plan( book, indexers=[indexer] if indexer is not None else None, - user_id=task.user_id, ) source = ProwlarrSource() diff --git a/src/frontend/src/App.tsx b/src/frontend/src/App.tsx index 2558edf1..65a572b7 100644 --- a/src/frontend/src/App.tsx +++ b/src/frontend/src/App.tsx @@ -87,6 +87,7 @@ import { bookSupportsTargets } from './utils/bookTargetLoader'; import { buildSearchQuery } from './utils/buildSearchQuery'; import { wasDownloadQueuedAfterResponseError } from './utils/downloadRecovery'; import { getDynamicOptionGroup } from './utils/dynamicFieldOptions'; +import { resolveDefaultLanguageCodes } from './utils/languageFilters'; import { getConfiguredMetadataProviderForContentType } from './utils/metadataProviders'; import { getEffectiveMetadataSort } from './utils/metadataSort'; import { isRecord } from './utils/objectHelpers'; @@ -1927,10 +1928,7 @@ function App() { ); const supportedFormats = config?.supported_formats || DEFAULT_SUPPORTED_FORMATS; const defaultLanguageCodes = useMemo( - () => - config?.default_language && config.default_language.length > 0 - ? config.default_language - : [bookLanguages[0]?.code || 'en'], + () => resolveDefaultLanguageCodes(config?.default_language, bookLanguages), [config?.default_language, bookLanguages], ); diff --git a/src/frontend/src/components/UrlSearchBootstrapMount.tsx b/src/frontend/src/components/UrlSearchBootstrapMount.tsx index 89591e38..68842527 100644 --- a/src/frontend/src/components/UrlSearchBootstrapMount.tsx +++ b/src/frontend/src/components/UrlSearchBootstrapMount.tsx @@ -3,6 +3,7 @@ import type { Dispatch, SetStateAction } from 'react'; import { useMountEffect } from '@/hooks/useMountEffect'; import type { AppConfig, AdvancedFilterState, ContentType, SearchMode, SortOption } from '@/types'; import { buildSearchQuery } from '@/utils/buildSearchQuery'; +import { resolveDefaultLanguageCodes } from '@/utils/languageFilters'; import { getEffectiveMetadataSort } from '@/utils/metadataSort'; import type { ParsedUrlSearch } from '@/utils/parseUrlSearchParams'; @@ -73,10 +74,10 @@ export const UrlSearchBootstrapMount = ({ } const bookLanguages = config.book_languages || []; - const defaultLanguageCodes = - config.default_language && config.default_language.length > 0 - ? config.default_language - : [bookLanguages[0]?.code || 'en']; + const defaultLanguageCodes = resolveDefaultLanguageCodes( + config.default_language, + bookLanguages, + ); if (parsedParams.searchInput) { setSearchInput(parsedParams.searchInput); diff --git a/src/frontend/src/components/settings/users/UserOverridesSection.tsx b/src/frontend/src/components/settings/users/UserOverridesSection.tsx index 20d497ec..aac46cd7 100644 --- a/src/frontend/src/components/settings/users/UserOverridesSection.tsx +++ b/src/frontend/src/components/settings/users/UserOverridesSection.tsx @@ -9,8 +9,8 @@ import { HeadingField, MultiSelectField, SelectField, TextField } from '../field import { FieldWrapper } from '../shared'; import { getFieldByKey, + resolveListOverride, toNormalizedLowercaseTextValue, - toStringListValue, toTextValue, } from './fieldHelpers'; import type { PerUserSettings } from './types'; @@ -180,12 +180,6 @@ export const UserOverridesSection = ({ label: 'Email Recipient', description: 'Email address used for this user in Email output mode.', }; - const browserDownloadGlobalValue = toStringListValue( - globalValues.DOWNLOAD_TO_BROWSER_CONTENT_TYPES, - ); - const browserDownloadUserValue = toStringListValue( - userSettings.DOWNLOAD_TO_BROWSER_CONTENT_TYPES, - ); const isOverridden = (key: DeliverySettingKey): boolean => { if ( @@ -201,10 +195,12 @@ export const UserOverridesSection = ({ return userValue !== globalValue; }; - const isBrowserDownloadOverridden = - Object.prototype.hasOwnProperty.call(userSettings, 'DOWNLOAD_TO_BROWSER_CONTENT_TYPES') && - userSettings.DOWNLOAD_TO_BROWSER_CONTENT_TYPES !== null && - JSON.stringify(browserDownloadUserValue) !== JSON.stringify(browserDownloadGlobalValue); + const { value: browserDownloadContentTypes, isOverridden: isBrowserDownloadOverridden } = + resolveListOverride( + userSettings.DOWNLOAD_TO_BROWSER_CONTENT_TYPES, + globalValues.DOWNLOAD_TO_BROWSER_CONTENT_TYPES, + Object.prototype.hasOwnProperty.call(userSettings, 'DOWNLOAD_TO_BROWSER_CONTENT_TYPES'), + ); const resetKeys = (keys: DeliverySettingKey[]) => { setUserSettings((prev) => { @@ -233,9 +229,6 @@ export const UserOverridesSection = ({ const outputModeValue = readValue('BOOKS_OUTPUT_MODE', 'folder'); const effectiveOutputMode = normalizeMode(outputModeValue); - const browserDownloadContentTypes = isBrowserDownloadOverridden - ? browserDownloadUserValue - : browserDownloadGlobalValue; const destinationValue = readValue('DESTINATION'); const destinationAudiobookValue = readValue('DESTINATION_AUDIOBOOK'); const libraryValue = readValue('BOOKLORE_LIBRARY_ID'); diff --git a/src/frontend/src/components/settings/users/UserSearchPreferencesSection.tsx b/src/frontend/src/components/settings/users/UserSearchPreferencesSection.tsx index 88183fe1..d7c7da01 100644 --- a/src/frontend/src/components/settings/users/UserSearchPreferencesSection.tsx +++ b/src/frontend/src/components/settings/users/UserSearchPreferencesSection.tsx @@ -8,8 +8,8 @@ import { HeadingField, MultiSelectField, SelectField } from '../fields'; import { FieldWrapper } from '../shared'; import { getFieldByKey, + resolveListOverride, toNormalizedLowercaseTextValue, - toStringListValue, toTextValue, } from './fieldHelpers'; import type { PerUserSettings } from './types'; @@ -141,15 +141,11 @@ export const UserSearchPreferencesSection = ({ ); const bookLanguageField = getFieldByKey(fields, 'BOOK_LANGUAGE', fallbackBookLanguageField); - const bookLanguageGlobalValue = toStringListValue(globalValues.BOOK_LANGUAGE); - const bookLanguageUserValue = toStringListValue(userSettings.BOOK_LANGUAGE); - const isBookLanguageOverridden = - Object.prototype.hasOwnProperty.call(userSettings, 'BOOK_LANGUAGE') && - userSettings.BOOK_LANGUAGE !== null && - JSON.stringify(bookLanguageUserValue) !== JSON.stringify(bookLanguageGlobalValue); - const bookLanguageValue = isBookLanguageOverridden - ? bookLanguageUserValue - : bookLanguageGlobalValue; + const { value: bookLanguageValue, isOverridden: isBookLanguageOverridden } = resolveListOverride( + userSettings.BOOK_LANGUAGE, + globalValues.BOOK_LANGUAGE, + Object.prototype.hasOwnProperty.call(userSettings, 'BOOK_LANGUAGE'), + ); const isOverridden = (key: SearchSettingKey): boolean => { if ( diff --git a/src/frontend/src/components/settings/users/fieldHelpers.ts b/src/frontend/src/components/settings/users/fieldHelpers.ts index 4e4649e6..f4d006f3 100644 --- a/src/frontend/src/components/settings/users/fieldHelpers.ts +++ b/src/frontend/src/components/settings/users/fieldHelpers.ts @@ -31,13 +31,33 @@ export const toNormalizedLowercaseTextValue = (value: unknown): string => { return toTrimmedTextValue(value).toLowerCase(); }; -export const toStringListValue = (value: unknown): string[] => { +const toStringListValue = (value: unknown): string[] => { if (!Array.isArray(value)) { return []; } return value.map((entry) => toTrimmedTextValue(entry)).filter((entry) => entry.length > 0); }; +/** + * Resolve a list-valued per-user override against its global value. + * + * A key absent from userSettings, or set to null, is not an override. A stored list + * that matches the global one is treated as inherited, matching how + * buildUserSettingsPayload clears it on save. + */ +export const resolveListOverride = ( + userValue: unknown, + globalValue: unknown, + hasUserKey: boolean, +): { value: string[]; isOverridden: boolean } => { + const globalList = toStringListValue(globalValue); + const userList = toStringListValue(userValue); + const isOverridden = + hasUserKey && userValue !== null && JSON.stringify(userList) !== JSON.stringify(globalList); + + return { value: isOverridden ? userList : globalList, isOverridden }; +}; + export const toComparableValue = (value: unknown): string => { if (value === null || value === undefined) { return ''; diff --git a/src/frontend/src/tests/languageFilters.test.ts b/src/frontend/src/tests/languageFilters.test.ts index 7e76dd8e..f05fcd17 100644 --- a/src/frontend/src/tests/languageFilters.test.ts +++ b/src/frontend/src/tests/languageFilters.test.ts @@ -7,6 +7,7 @@ import { buildLanguageNormalizer, getReleaseSearchLanguageParams, releaseLanguageMatchesFilter, + resolveDefaultLanguageCodes, } from '../utils/languageFilters'; const supportedLanguages: Language[] = [ @@ -67,3 +68,36 @@ describe('releaseLanguageMatchesFilter', () => { expect(visibleLanguages).toHaveLength(48); }); }); + +describe('resolveDefaultLanguageCodes', () => { + it('keeps an explicitly empty default as "no default filter"', () => { + // The backend stores [] to mean "do not filter"; substituting the first + // supported language here would make the UI filter where the server does not. + expect(resolveDefaultLanguageCodes([], supportedLanguages)).toEqual([]); + }); + + it('leaves a configured default untouched', () => { + expect(resolveDefaultLanguageCodes(['de', 'hu'], supportedLanguages)).toEqual(['de', 'hu']); + }); + + it('falls back to the first supported language only when nothing is configured', () => { + expect(resolveDefaultLanguageCodes(undefined, supportedLanguages)).toEqual(['en']); + expect(resolveDefaultLanguageCodes(null, [])).toEqual(['en']); + }); + + it('sends no language filter when an empty default is the whole selection', () => { + const defaults = resolveDefaultLanguageCodes([], supportedLanguages); + + expect( + getReleaseSearchLanguageParams([LANGUAGE_OPTION_DEFAULT], supportedLanguages, defaults), + ).toBe(undefined); + }); + + it('does not smuggle the first language into a Default+German selection', () => { + const defaults = resolveDefaultLanguageCodes([], supportedLanguages); + + expect( + getReleaseSearchLanguageParams([LANGUAGE_OPTION_DEFAULT, 'de'], supportedLanguages, defaults), + ).toEqual(['de']); + }); +}); diff --git a/src/frontend/src/utils/languageFilters.ts b/src/frontend/src/utils/languageFilters.ts index c7ec79c8..fba08b51 100644 --- a/src/frontend/src/utils/languageFilters.ts +++ b/src/frontend/src/utils/languageFilters.ts @@ -27,6 +27,24 @@ export const normalizeLanguageSelection = (selected: string[]): string[] => { return unique.length ? unique : [LANGUAGE_OPTION_DEFAULT]; }; +/** + * Resolve the language codes the "Default" filter option stands for. + * + * An explicitly empty list is a deliberate "no default filter" and is returned as-is; + * only a missing value falls back to the first supported language. Substituting a + * language for the empty list would make the UI filter by a language the backend + * does not apply. + */ +export const resolveDefaultLanguageCodes = ( + configuredDefault: string[] | null | undefined, + supportedLanguages: Language[], +): string[] => { + if (Array.isArray(configuredDefault)) { + return configuredDefault; + } + return [supportedLanguages[0]?.code || 'en']; +}; + export const getLanguageFilterValues = ( selection: string[], supportedLanguages: Language[], diff --git a/tests/config/test_search_mode_settings.py b/tests/config/test_search_mode_settings.py index 17c29f86..b6699297 100644 --- a/tests/config/test_search_mode_settings.py +++ b/tests/config/test_search_mode_settings.py @@ -1,8 +1,17 @@ """Tests for search mode settings definitions.""" +import json + +import pytest + from shelfmark.config.settings import search_mode_settings +def _search_mode_field(key: str): + fields = {field.key: field for field in search_mode_settings() if hasattr(field, "key")} + return fields[key] + + def test_search_mode_settings_include_release_source_links_toggle(): fields = {field.key: field for field in search_mode_settings() if hasattr(field, "key")} @@ -21,3 +30,47 @@ def test_book_language_is_user_overridable(): assert field.label == "Default Book Languages" assert field.default == ["en"] assert field.user_overridable is True + + +def test_book_language_stored_under_the_general_tab_still_resolves(tmp_path): + """BOOK_LANGUAGE moved from the General tab to Search Mode with no migration. + + That is only safe because both tabs persist into the same settings.json, so an + install that stored the value while the field lived on General keeps it. If the + two tabs ever get separate files, every existing install silently falls back to + the ["en"] default instead. + """ + from shelfmark.core.settings_registry import _get_config_file_path, get_setting_value + + (tmp_path / "settings.json").write_text(json.dumps({"BOOK_LANGUAGE": ["de", "fr"]})) + + with pytest.MonkeyPatch.context() as monkeypatch: + monkeypatch.delenv("BOOK_LANGUAGE", raising=False) + monkeypatch.setattr("shelfmark.config.env.CONFIG_DIR", tmp_path) + + assert _get_config_file_path("search_mode") == _get_config_file_path("general") + assert get_setting_value(_search_mode_field("BOOK_LANGUAGE"), "search_mode") == ["de", "fr"] + + +def test_book_language_uses_its_default_on_a_fresh_install(tmp_path): + from shelfmark.core.settings_registry import get_setting_value + + with pytest.MonkeyPatch.context() as monkeypatch: + monkeypatch.delenv("BOOK_LANGUAGE", raising=False) + monkeypatch.setattr("shelfmark.config.env.CONFIG_DIR", tmp_path) + + assert get_setting_value(_search_mode_field("BOOK_LANGUAGE"), "search_mode") == ["en"] + + +def test_book_language_env_var_beats_the_stored_value(tmp_path): + from shelfmark.core.settings_registry import get_setting_value, is_value_from_env + + (tmp_path / "settings.json").write_text(json.dumps({"BOOK_LANGUAGE": ["de", "fr"]})) + field = _search_mode_field("BOOK_LANGUAGE") + + with pytest.MonkeyPatch.context() as monkeypatch: + monkeypatch.setenv("BOOK_LANGUAGE", "es,it") + monkeypatch.setattr("shelfmark.config.env.CONFIG_DIR", tmp_path) + + assert is_value_from_env(field) is True + assert get_setting_value(field, "search_mode") == ["es", "it"] diff --git a/tests/config/test_users_settings.py b/tests/config/test_users_settings.py index 8b77b9a5..38e5ffd0 100644 --- a/tests/config/test_users_settings.py +++ b/tests/config/test_users_settings.py @@ -90,7 +90,7 @@ def test_visible_self_settings_sections_field_defaults_and_options(): { "value": "search", "label": "Search Preferences", - "description": "Show personal search mode and provider settings.", + "description": "Show personal search mode, language, and provider settings.", }, { "value": "notifications", diff --git a/tests/core/test_admin_users_api.py b/tests/core/test_admin_users_api.py index ee6d00a4..4dd27745 100644 --- a/tests/core/test_admin_users_api.py +++ b/tests/core/test_admin_users_api.py @@ -534,6 +534,46 @@ class TestAdminUserUpdateEndpoint: ) assert user_db.get_user_settings(user["id"]) == {} + def test_update_user_settings_skips_blank_book_languages(self, admin_client, user_db): + """A trailing comma or a blank slot means "nothing there", not an unknown language.""" + user = user_db.create_user(username="alice") + + resp = admin_client.put( + f"/api/admin/users/{user['id']}", + json={"settings": {"BOOK_LANGUAGE": "en,"}}, + ) + assert resp.status_code == 200 + assert user_db.get_user_settings(user["id"])["BOOK_LANGUAGE"] == ["en"] + + def test_update_user_settings_normalizes_every_validated_search_key( + self, admin_client, user_db + ): + """Keys the search validator recognises keep their normalized value. + + These three were validated but then stored raw, because the write-back was + gated on a hand-maintained subset of the validated keys. A padded provider + name was accepted and then persisted with its padding, so every later lookup + of it failed. + """ + user = user_db.create_user(username="alice") + + resp = admin_client.put( + f"/api/admin/users/{user['id']}", + json={ + "settings": { + "METADATA_PROVIDER_COMBINED": " openlibrary ", + "SHOW_COMBINED_SELECTOR": "yes", + "FORCE_COMBINED_SEARCH": "", + } + }, + ) + assert resp.status_code == 200 + + settings = user_db.get_user_settings(user["id"]) + assert settings["METADATA_PROVIDER_COMBINED"] == "openlibrary" + assert settings["SHOW_COMBINED_SELECTOR"] is True + assert settings["FORCE_COMBINED_SEARCH"] is False + def test_update_user_settings_accepts_notification_overrides(self, admin_client, user_db): user = user_db.create_user(username="alice")