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.
This commit is contained in:
CaliBrain
2026-08-24 18:11:29 -04:00
committed by GitHub
parent 463ef49ac3
commit ff06a1a581
13 changed files with 207 additions and 51 deletions
+11 -7
View File
@@ -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])
+7 -9
View File
@@ -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
@@ -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()
+2 -4
View File
@@ -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],
);
@@ -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);
@@ -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');
@@ -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 (
@@ -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 '';
@@ -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']);
});
});
+18
View File
@@ -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[],
+53
View File
@@ -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"]
+1 -1
View File
@@ -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",
+40
View File
@@ -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")