diff --git a/docs/users-and-requests.md b/docs/users-and-requests.md index 3d4ae8f9..6cdad4c3 100644 --- a/docs/users-and-requests.md +++ b/docs/users-and-requests.md @@ -98,6 +98,7 @@ Each content type (ebook, audiobook) has a default mode that sets the baseline: | Request Rules | Per-source overrides (see below) | None | | Max Pending Requests Per User | Open request limit per user | 20 | | Allow Notes on Requests | Let users attach a note when submitting | On | +| Allow book requests alongside downloads | Let users who can download also request a book without choosing a release, for a book nobody has released yet or one no source has. The request waits for an admin, and the user gets the Requests tab to follow or cancel it. Admins can always do this | Off | ### Request Rules diff --git a/shelfmark/config/users_settings.py b/shelfmark/config/users_settings.py index b4cfd060..d795dd8c 100644 --- a/shelfmark/config/users_settings.py +++ b/shelfmark/config/users_settings.py @@ -450,4 +450,16 @@ def users_settings() -> list[SettingsField]: user_overridable=True, show_when={"field": "REQUESTS_ENABLED", "value": True}, ), + CheckboxField( + key="REQUESTS_ALLOW_BOOK_REQUEST_WITH_DOWNLOAD", + label="Allow book requests alongside downloads", + description=( + "Let users who can download also request a book without choosing a release, for " + "a book nobody has released yet or one no source has. The request waits for an " + "admin. Admins can always do this." + ), + default=False, + user_overridable=True, + show_when={"field": "REQUESTS_ENABLED", "value": True}, + ), ] diff --git a/shelfmark/core/request_policy.py b/shelfmark/core/request_policy.py index 3aecec4f..18e02735 100644 --- a/shelfmark/core/request_policy.py +++ b/shelfmark/core/request_policy.py @@ -70,6 +70,7 @@ REQUEST_POLICY_KEYS = frozenset( "REQUEST_POLICY_RULES", "MAX_PENDING_REQUESTS_PER_USER", "REQUESTS_ALLOW_NOTES", + "REQUESTS_ALLOW_BOOK_REQUEST_WITH_DOWNLOAD", } ) diff --git a/shelfmark/core/request_routes.py b/shelfmark/core/request_routes.py index 53c96c80..3b6ccf0a 100644 --- a/shelfmark/core/request_routes.py +++ b/shelfmark/core/request_routes.py @@ -304,6 +304,10 @@ def _prepare_request_create_arguments( user_db: UserDB, data: dict[str, Any], ) -> dict[str, Any]: + """Check a request payload against the policy and build create_request's arguments. + + A book-level request in Download mode becomes a pending one when the actor may ask for it. + """ db_user_id, db_gate = _require_db_user_id() if db_gate is not None or db_user_id is None: msg = "User identity is unavailable for request workflow" @@ -404,6 +408,21 @@ def _prepare_request_create_arguments( ) requested_level = str(request_level).strip().lower() if isinstance(request_level, str) else "" + if ( + resolved_mode == PolicyMode.DOWNLOAD + and requested_level == "book" + and release_data is None + and ( + bool(session.get("is_admin", False)) + or coerce_bool( + effective.get("REQUESTS_ALLOW_BOOK_REQUEST_WITH_DOWNLOAD"), default=False + ) + ) + ): + # Everything can be downloaded here, but the user asked for the book itself (nothing to + # download yet, or no source has it): leave it for an admin instead of refusing it. + resolved_mode = PolicyMode.REQUEST_BOOK + if resolved_mode == PolicyMode.REQUEST_BOOK and requested_level != "book": msg = "Policy requires book-level requests" raise RequestServiceError( @@ -571,6 +590,7 @@ def register_request_routes( @app.route("/api/request-policy", methods=["GET"]) def api_request_policy() -> ResponseReturnValue: + """The signed-in user's request policy, as the frontend needs it to label actions.""" auth_gate = _require_request_endpoints_available(resolve_auth_mode) if auth_gate is not None: return auth_gate @@ -624,6 +644,9 @@ def register_request_routes( "requests_enabled": requests_enabled, "is_admin": is_admin, "allow_notes": coerce_bool(effective.get("REQUESTS_ALLOW_NOTES"), default=True), + "allow_book_request_with_download": coerce_bool( + effective.get("REQUESTS_ALLOW_BOOK_REQUEST_WITH_DOWNLOAD"), default=False + ), "defaults": { "ebook": ( default_ebook_mode.value diff --git a/src/frontend/src/App.tsx b/src/frontend/src/App.tsx index 859686a8..0343d615 100644 --- a/src/frontend/src/App.tsx +++ b/src/frontend/src/App.tsx @@ -109,6 +109,8 @@ import { import { applyDirectPolicyModeToButtonState, applyUniversalPolicyModeToButtonState, + canRequestBookFromReleaseModal, + shouldShowRequestsTab, } from './utils/requestPolicyUi'; import { getSearchByPreference, setSearchByPreference } from './utils/searchByPreference'; import { buildUrlSearchHash } from './utils/urlSearchHash'; @@ -264,6 +266,7 @@ const AdminSettingsWarmupMount = () => { return null; }; +/** The application shell: search, results, the release modal, activity and settings. */ function App() { const location = useLocation(); const { toasts, showToast, removeToast } = useToast(); @@ -322,6 +325,7 @@ function App() { getSourceMode, requestsEnabled: requestsPolicyEnabled, allowNotes: allowRequestNotes, + allowBookRequestWithDownload, refresh: refreshRequestPolicy, } = useRequestPolicy({ enabled: isAuthenticated, @@ -455,20 +459,23 @@ function App() { }; }, [activityStatus, currentStatus, dismissedDownloadTaskIds]); - const showRequestsTab = useMemo(() => { - if (requestRoleIsAdmin) { - return true; - } - if (!isAuthenticated || !requestsPolicyEnabled) { - return false; - } - if (!requestPolicy) { - return false; - } - return !( - requestPolicy.defaults.ebook === 'download' && requestPolicy.defaults.audiobook === 'download' - ); - }, [requestRoleIsAdmin, isAuthenticated, requestsPolicyEnabled, requestPolicy]); + const showRequestsTab = useMemo( + () => + shouldShowRequestsTab({ + isAdmin: requestRoleIsAdmin, + isAuthenticated, + requestsEnabled: requestsPolicyEnabled, + defaults: requestPolicy?.defaults ?? null, + allowBookRequestWithDownload, + }), + [ + requestRoleIsAdmin, + isAuthenticated, + requestsPolicyEnabled, + requestPolicy, + allowBookRequestWithDownload, + ], + ); // Search state and handlers const { @@ -2671,7 +2678,15 @@ function App() { onDownload={isBrowseFulfilMode ? handleBrowseFulfilDownload : handleReleaseDownload} onRequestRelease={isBrowseFulfilMode ? undefined : handleReleaseRequest} onRequestBook={ - isBrowseFulfilMode || !requestRoleIsAdmin ? undefined : handleReleaseBookRequest + canRequestBookFromReleaseModal({ + isBrowseFulfilMode, + isAdmin: requestRoleIsAdmin, + requestsEnabled: requestsPolicyEnabled, + allowBookRequestWithDownload, + bookRequestMode: getSourceMode('*', activeReleaseContentType), + }) + ? handleReleaseBookRequest + : undefined } getPolicyModeForSource={ isBrowseFulfilMode ? () => 'download' : (source, ct) => getSourceMode(source, ct) @@ -2776,7 +2791,7 @@ function App() { pendingRequestCount={pendingRequestCount} showRequestsTab={showRequestsTab} isRequestsLoading={isActivitySnapshotLoading} - onRequestCancel={showRequestsTab ? handleRequestCancel : undefined} + onRequestCancel={handleRequestCancel} onRequestApprove={requestRoleIsAdmin ? handleRequestApprove : undefined} onRequestReject={requestRoleIsAdmin ? handleRequestReject : undefined} onRequestReopen={requestRoleIsAdmin ? handleRequestReopen : undefined} diff --git a/src/frontend/src/components/settings/users/UserRequestPolicyOverridesSection.tsx b/src/frontend/src/components/settings/users/UserRequestPolicyOverridesSection.tsx index c96ba410..a7413bd4 100644 --- a/src/frontend/src/components/settings/users/UserRequestPolicyOverridesSection.tsx +++ b/src/frontend/src/components/settings/users/UserRequestPolicyOverridesSection.tsx @@ -31,6 +31,7 @@ const REQUEST_POLICY_OVERRIDE_KEYS: Array = [ 'REQUEST_POLICY_RULES', 'MAX_PENDING_REQUESTS_PER_USER', 'REQUESTS_ALLOW_NOTES', + 'REQUESTS_ALLOW_BOOK_REQUEST_WITH_DOWNLOAD', ]; const requestPolicyHeading: HeadingFieldConfig = { diff --git a/src/frontend/src/components/settings/users/types.ts b/src/frontend/src/components/settings/users/types.ts index f767d42a..41a6b8ae 100644 --- a/src/frontend/src/components/settings/users/types.ts +++ b/src/frontend/src/components/settings/users/types.ts @@ -22,6 +22,7 @@ export interface PerUserSettings { REQUEST_POLICY_RULES?: Array>; MAX_PENDING_REQUESTS_PER_USER?: number; REQUESTS_ALLOW_NOTES?: boolean; + REQUESTS_ALLOW_BOOK_REQUEST_WITH_DOWNLOAD?: boolean; } export interface CreateUserFormState { diff --git a/src/frontend/src/hooks/useRequestPolicy.ts b/src/frontend/src/hooks/useRequestPolicy.ts index 0337ba88..54125ca0 100644 --- a/src/frontend/src/hooks/useRequestPolicy.ts +++ b/src/frontend/src/hooks/useRequestPolicy.ts @@ -22,11 +22,15 @@ interface UseRequestPolicyReturn { isAdmin: boolean; requestsEnabled: boolean; allowNotes: boolean; + allowBookRequestWithDownload: boolean; getDefaultMode: (contentType: string) => RequestPolicyMode; getSourceMode: (source: string, contentType: string) => RequestPolicyMode; refresh: (options?: { force?: boolean }) => Promise; } +/** + * Fetch and cache the user's request policy, and resolve the mode for a source and content type. + */ export function useRequestPolicy({ enabled, isAdmin, @@ -102,6 +106,9 @@ export function useRequestPolicy({ isAdmin: enabled && policy ? policy.is_admin : isAdmin, requestsEnabled: enabled ? (policy?.requests_enabled ?? false) : false, allowNotes: enabled ? (policy?.allow_notes ?? true) : true, + allowBookRequestWithDownload: enabled + ? (policy?.allow_book_request_with_download ?? false) + : false, getDefaultMode, getSourceMode, refresh, diff --git a/src/frontend/src/tests/requestPolicyUi.test.ts b/src/frontend/src/tests/requestPolicyUi.test.ts index b2932833..47bd47d9 100644 --- a/src/frontend/src/tests/requestPolicyUi.test.ts +++ b/src/frontend/src/tests/requestPolicyUi.test.ts @@ -4,8 +4,103 @@ import type { ButtonStateInfo } from '../types/index'; import { applyDirectPolicyModeToButtonState, applyUniversalPolicyModeToButtonState, + canRequestBookFromReleaseModal, + shouldShowRequestsTab, } from '../utils/requestPolicyUi'; +describe('canRequestBookFromReleaseModal', () => { + const base = { + isBrowseFulfilMode: false, + isAdmin: false, + requestsEnabled: true, + allowBookRequestWithDownload: false, + bookRequestMode: 'download' as const, + }; + + it('is always on for an admin', () => { + expect(canRequestBookFromReleaseModal({ ...base, isAdmin: true })).toBe(true); + expect(canRequestBookFromReleaseModal({ ...base, isAdmin: true, requestsEnabled: false })).toBe( + true, + ); + }); + + it('is off for a user unless the setting allows it', () => { + expect(canRequestBookFromReleaseModal(base)).toBe(false); + expect(canRequestBookFromReleaseModal({ ...base, allowBookRequestWithDownload: true })).toBe( + true, + ); + }); + + it('is off for a user whose policy for this content type is not Download', () => { + const allowed = { ...base, allowBookRequestWithDownload: true }; + expect(canRequestBookFromReleaseModal({ ...allowed, bookRequestMode: 'request_release' })).toBe( + false, + ); + expect(canRequestBookFromReleaseModal({ ...allowed, bookRequestMode: 'request_book' })).toBe( + false, + ); + expect(canRequestBookFromReleaseModal({ ...allowed, bookRequestMode: 'blocked' })).toBe(false); + }); + + it('does not depend on the policy mode for an admin', () => { + expect( + canRequestBookFromReleaseModal({ ...base, isAdmin: true, bookRequestMode: 'blocked' }), + ).toBe(true); + }); + + it('needs requests to be on for a user', () => { + expect( + canRequestBookFromReleaseModal({ + ...base, + requestsEnabled: false, + allowBookRequestWithDownload: true, + }), + ).toBe(false); + }); + + it('is off while fulfilling a request, even for an admin', () => { + expect( + canRequestBookFromReleaseModal({ ...base, isAdmin: true, isBrowseFulfilMode: true }), + ).toBe(false); + }); +}); + +describe('shouldShowRequestsTab', () => { + const allDownload = { ebook: 'download' as const, audiobook: 'download' as const }; + const base = { + isAdmin: false, + isAuthenticated: true, + requestsEnabled: true, + defaults: allDownload, + allowBookRequestWithDownload: false, + }; + + it('is hidden for a user who can only download', () => { + expect(shouldShowRequestsTab(base)).toBe(false); + }); + + it('shows for a user who may request a book alongside downloads', () => { + expect(shouldShowRequestsTab({ ...base, allowBookRequestWithDownload: true })).toBe(true); + }); + + it('shows when either default asks for requests', () => { + expect( + shouldShowRequestsTab({ + ...base, + defaults: { ebook: 'download', audiobook: 'request_book' }, + }), + ).toBe(true); + }); + + it('is hidden with requests off or no policy, and always shown to an admin', () => { + const allowed = { ...base, allowBookRequestWithDownload: true }; + expect(shouldShowRequestsTab({ ...allowed, requestsEnabled: false })).toBe(false); + expect(shouldShowRequestsTab({ ...allowed, defaults: null })).toBe(false); + expect(shouldShowRequestsTab({ ...allowed, isAuthenticated: false })).toBe(false); + expect(shouldShowRequestsTab({ ...base, isAdmin: true, requestsEnabled: false })).toBe(true); + }); +}); + describe('requestPolicyUi', () => { const baseDownload: ButtonStateInfo = { text: 'Download', state: 'download' }; diff --git a/src/frontend/src/types/index.ts b/src/frontend/src/types/index.ts index 14729a61..d466ea9f 100644 --- a/src/frontend/src/types/index.ts +++ b/src/frontend/src/types/index.ts @@ -217,6 +217,7 @@ export interface RequestPolicyResponse { requests_enabled: boolean; is_admin: boolean; allow_notes: boolean; + allow_book_request_with_download?: boolean; defaults: RequestPolicyDefaults; rules: Array>; source_modes: RequestPolicySourceMode[]; diff --git a/src/frontend/src/utils/requestPolicyUi.ts b/src/frontend/src/utils/requestPolicyUi.ts index 24006d86..c0703f2e 100644 --- a/src/frontend/src/utils/requestPolicyUi.ts +++ b/src/frontend/src/utils/requestPolicyUi.ts @@ -1,5 +1,54 @@ import type { ButtonStateInfo, RequestPolicyMode } from '../types'; +/** + * Whether the release modal offers a book-level request ("Add to requests"). Admins always get + * it. Anyone else gets it only when requests are on, the setting lets users who can download + * also ask for a book without choosing a release, and their policy for this content type is + * Download. The server turns a book-level request into a pending one only in Download mode, so + * the action would be refused (Blocked) or mean something else (Request Release) otherwise. + */ +export const canRequestBookFromReleaseModal = ({ + isBrowseFulfilMode, + isAdmin, + requestsEnabled, + allowBookRequestWithDownload, + bookRequestMode, +}: { + isBrowseFulfilMode: boolean; + isAdmin: boolean; + requestsEnabled: boolean; + allowBookRequestWithDownload: boolean; + bookRequestMode: RequestPolicyMode; +}): boolean => { + if (isBrowseFulfilMode) return false; + if (isAdmin) return true; + return requestsEnabled && allowBookRequestWithDownload && bookRequestMode === 'download'; +}; + +/** + * Whether the activity sidebar shows a Requests tab. Admins always see it. A user sees it when + * requests are on and either default asks for requests, or when they may request a book + * alongside downloads, since those requests then need a place to be followed and cancelled. + */ +export const shouldShowRequestsTab = ({ + isAdmin, + isAuthenticated, + requestsEnabled, + defaults, + allowBookRequestWithDownload, +}: { + isAdmin: boolean; + isAuthenticated: boolean; + requestsEnabled: boolean; + defaults: { ebook: RequestPolicyMode; audiobook: RequestPolicyMode } | null; + allowBookRequestWithDownload: boolean; +}): boolean => { + if (isAdmin) return true; + if (!isAuthenticated || !requestsEnabled || !defaults) return false; + if (allowBookRequestWithDownload) return true; + return !(defaults.ebook === 'download' && defaults.audiobook === 'download'); +}; + export const applyDirectPolicyModeToButtonState = ( baseState: ButtonStateInfo, mode: RequestPolicyMode, diff --git a/tests/core/test_request_routes_api.py b/tests/core/test_request_routes_api.py index a6a1b498..6eab9efe 100644 --- a/tests/core/test_request_routes_api.py +++ b/tests/core/test_request_routes_api.py @@ -48,9 +48,12 @@ def _policy( default_audiobook: str = "download", max_pending_requests_per_user: int = 20, requests_allow_notes: bool = True, + allow_book_request_with_download: bool = False, rules: list[dict] | None = None, ) -> dict: + """A request-policy settings dict with the given values, for patching the policy loader.""" return { + "REQUESTS_ALLOW_BOOK_REQUEST_WITH_DOWNLOAD": allow_book_request_with_download, "REQUESTS_ENABLED": requests_enabled, "REQUEST_POLICY_DEFAULT_EBOOK": default_ebook, "REQUEST_POLICY_DEFAULT_AUDIOBOOK": default_audiobook, @@ -2126,6 +2129,112 @@ class TestRequestCreationEdgeCases: mock_queue_release.assert_not_called() assert main_module.user_db.list_requests(user_id=user["id"]) == [] + @staticmethod + def _book_level_request_payload() -> dict: + return { + "book_data": { + "title": "Not Released Yet", + "author": "Shelfmark", + "content_type": "ebook", + "provider": "openlibrary", + "provider_id": "not-released-yet", + }, + "context": {"source": "*", "content_type": "ebook", "request_level": "book"}, + } + + def _post_book_request(self, main_module, client, policy): + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object( + main_module, "load_users_request_policy_settings", return_value=policy + ): + with patch( + "shelfmark.core.request_routes.load_users_request_policy_settings", + return_value=policy, + ): + with patch.object(main_module.backend, "queue_release") as mock_queue_release: + resp = client.post("/api/requests", json=self._book_level_request_payload()) + return resp, mock_queue_release + + def test_a_book_request_is_kept_in_download_mode_when_the_setting_allows_it( + self, main_module, client + ): + """Download mode left no way to ask for a book nobody has released yet (#995, #1019).""" + user = _create_user(main_module, prefix="reader") + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + policy = _policy(default_ebook="download", allow_book_request_with_download=True) + + resp, mock_queue_release = self._post_book_request(main_module, client, policy) + + assert resp.status_code == 201 + assert resp.json["status"] == "pending" + assert resp.json["policy_mode"] == "request_book" + assert resp.json["request_level"] == "book" + mock_queue_release.assert_not_called() + assert len(main_module.user_db.list_requests(user_id=user["id"])) == 1 + + def test_an_admin_can_request_a_book_in_download_mode_without_the_setting( + self, main_module, client + ): + admin = _create_user(main_module, prefix="admin", role="admin") + _set_session(client, user_id=admin["username"], db_user_id=admin["id"], is_admin=True) + policy = _policy(default_ebook="download", allow_book_request_with_download=False) + + resp, mock_queue_release = self._post_book_request(main_module, client, policy) + + assert resp.status_code == 201 + assert resp.json["policy_mode"] == "request_book" + mock_queue_release.assert_not_called() + + def test_the_setting_does_not_turn_a_release_request_into_a_book_request( + self, main_module, client + ): + user = _create_user(main_module, prefix="reader") + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + policy = _policy(default_ebook="download", allow_book_request_with_download=True) + payload = self._book_level_request_payload() + payload["release_data"] = {"source": "direct_download", "source_id": "abc", "title": "T"} + payload["context"] = { + "source": "direct_download", + "content_type": "ebook", + "request_level": "release", + } + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object( + main_module, "load_users_request_policy_settings", return_value=policy + ): + with patch( + "shelfmark.core.request_routes.load_users_request_policy_settings", + return_value=policy, + ): + with patch.object( + main_module.backend, "queue_release", return_value=(True, None) + ) as mock_queue_release: + resp = client.post("/api/requests", json=payload) + + assert resp.status_code == 200 + assert resp.json["kind"] == "download" + mock_queue_release.assert_called_once() + assert main_module.user_db.list_requests(user_id=user["id"]) == [] + + @pytest.mark.parametrize("allowed", [True, False]) + def test_policy_endpoint_reports_the_book_request_setting(self, main_module, client, allowed): + user = _create_user(main_module, prefix="reader") + _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False) + policy = _policy(default_ebook="download", allow_book_request_with_download=allowed) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + with patch.object( + main_module, "load_users_request_policy_settings", return_value=policy + ): + with patch( + "shelfmark.core.request_routes.load_users_request_policy_settings", + return_value=policy, + ): + resp = client.get("/api/request-policy") + + assert resp.json["allow_book_request_with_download"] is allowed + def test_blocked_policy_returns_403(self, main_module, client): user = _create_user(main_module, prefix="reader") _set_session(client, user_id=user["username"], db_user_id=user["id"], is_admin=False)