From 87f71ebdbac34c6805000fa94f9851ef62172839 Mon Sep 17 00:00:00 2001 From: Sanket17052006 Date: Sat, 13 Jun 2026 14:38:41 +0530 Subject: [PATCH 1/5] Migrate merge_authors_json to FastAPI --- openlibrary/asgi_app.py | 2 + openlibrary/fastapi/merge_authors.py | 71 +++++ openlibrary/plugins/upstream/edits.py | 5 +- openlibrary/plugins/upstream/merge_authors.py | 19 +- .../upstream/tests/test_merge_authors.py | 4 + .../tests/fastapi/test_merge_authors.py | 281 ++++++++++++++++++ 6 files changed, 372 insertions(+), 10 deletions(-) create mode 100644 openlibrary/fastapi/merge_authors.py create mode 100644 openlibrary/tests/fastapi/test_merge_authors.py diff --git a/openlibrary/asgi_app.py b/openlibrary/asgi_app.py index bca6386817a..207e29be913 100644 --- a/openlibrary/asgi_app.py +++ b/openlibrary/asgi_app.py @@ -222,6 +222,7 @@ def health() -> dict[str, str]: from openlibrary.fastapi.internal.api import router as internal_router from openlibrary.fastapi.languages import router as languages_router from openlibrary.fastapi.lists import router as lists_router + from openlibrary.fastapi.merge_authors import router as merge_authors_router from openlibrary.fastapi.partials import router as partials_router from openlibrary.fastapi.public_my_books import router as public_my_books_router from openlibrary.fastapi.publishers import router as publishers_router @@ -240,6 +241,7 @@ def health() -> dict[str, str]: app.include_router(internal_router) app.include_router(languages_router) app.include_router(lists_router) + app.include_router(merge_authors_router) app.include_router(partials_router) app.include_router(public_my_books_router) app.include_router(publishers_router) diff --git a/openlibrary/fastapi/merge_authors.py b/openlibrary/fastapi/merge_authors.py new file mode 100644 index 00000000000..54f195c724b --- /dev/null +++ b/openlibrary/fastapi/merge_authors.py @@ -0,0 +1,71 @@ +"""FastAPI endpoint for merging authors. + +Migrated from openlibrary.plugins.upstream.merge_authors.merge_authors_json. +""" + +from __future__ import annotations + +import json +from typing import Any + +from fastapi import APIRouter, HTTPException +from pydantic import BaseModel + +from infogami.infobase.client import ClientException +from openlibrary.fastapi.auth import LibrarianDep # noqa: TC001 +from openlibrary.plugins.upstream.edits import process_merge_request +from openlibrary.plugins.upstream.merge_authors import ( + AuthorMergeEngine, + AuthorRedirectEngine, +) +from openlibrary.utils.retry import MaxRetriesExceeded, RetryStrategy + +router = APIRouter(tags=["merge-authors"]) + + +class MergeAuthorsBody(BaseModel): + master: str + duplicates: list[str] + mrid: str | None = None + comment: str | None = None + olids: str = "" + + +@router.post("/authors/merge.json") +def merge_authors_json(_: LibrarianDep, data: MergeAuthorsBody) -> Any: + def merge_records() -> Any: + try: + engine = AuthorMergeEngine(AuthorRedirectEngine()) + return engine.merge(data.master, data.duplicates) + except ClientException as e: + raise HTTPException( + status_code=400, + detail=json.loads(e.json), + ) + + merge_result = merge_records() + + def update_request() -> None: + if data.mrid: + rtype = "update-request" + update_data: dict[str, Any] = {"action": "approve", "mrid": data.mrid} + else: + rtype = "create-request" + update_data = {"mr_type": 2, "olids": data.olids, "action": "create-merged"} + if data.comment: + update_data["comment"] = data.comment + process_merge_request(rtype, update_data) + + try: + RetryStrategy( + [ClientException], + max_retries=5, + delay=2, + )(update_request) + except MaxRetriesExceeded as e: + raise HTTPException( + status_code=400, + detail=str(e.last_exception), + ) + + return merge_result diff --git a/openlibrary/plugins/upstream/edits.py b/openlibrary/plugins/upstream/edits.py index 5df31a66e51..30018161160 100644 --- a/openlibrary/plugins/upstream/edits.py +++ b/openlibrary/plugins/upstream/edits.py @@ -9,6 +9,7 @@ from infogami.utils.view import render_template from openlibrary import accounts from openlibrary.core.edits import ApiMode, CommunityEditsQueue, get_status_for_view +from openlibrary.utils.request_context import site # Usergroups that may use create or update merge requests ALLOWED_USERGROUPS: list[str] = [ @@ -203,12 +204,12 @@ def create_url(mr_type: int, olids: list[str], primary: str | None = None) -> st def create_title(mr_type: int, olids: list[str]) -> str: if mr_type == CommunityEditsQueue.TYPE["WORK_MERGE"]: for olid in olids: - book = web.ctx.site.get(f"/works/{olid}") + book = site.get().get(f"/works/{olid}") if book and book.title: return book.title elif mr_type == CommunityEditsQueue.TYPE["AUTHOR_MERGE"]: for olid in olids: - author = web.ctx.site.get(f"/authors/{olid}") + author = site.get().get(f"/authors/{olid}") if author and author.name: return author.name return "Unknown record" diff --git a/openlibrary/plugins/upstream/merge_authors.py b/openlibrary/plugins/upstream/merge_authors.py index 46d9a5e60cc..a55eb08faca 100644 --- a/openlibrary/plugins/upstream/merge_authors.py +++ b/openlibrary/plugins/upstream/merge_authors.py @@ -3,6 +3,7 @@ import json import re from typing import Any +from warnings import deprecated import web @@ -13,6 +14,7 @@ from openlibrary.plugins.upstream.edits import process_merge_request from openlibrary.plugins.worksearch.code import top_books_from_author from openlibrary.utils import dicthash, uniq +from openlibrary.utils.request_context import site from openlibrary.utils.retry import MaxRetriesExceeded, RetryStrategy @@ -87,7 +89,7 @@ def do_merge(self, master: str, duplicates: list[str]) -> list: docs_to_save.extend(self.redirect_engine.make_redirects(master, duplicates)) # Merge all the duplicates into the master. - master_doc = web.ctx.site.get(master).dict() + master_doc = site.get().get(master).dict() dups = get_many(duplicates) for d in dups: master_doc = self.merge_docs(master_doc, d) @@ -125,12 +127,12 @@ def merge_property(self, a, b): class AuthorRedirectEngine(BasicRedirectEngine): def find_references(self, key): q = {"type": "/type/edition", "authors": key, "limit": 10000} - edition_keys = web.ctx.site.things(q) + edition_keys = site.get().things(q) editions = get_many(edition_keys) work_keys_1 = [w["key"] for e in editions for w in e.get("works", [])] q = {"type": "/type/work", "authors": {"author": {"key": key}}, "limit": 10000} - work_keys_2 = web.ctx.site.things(q) + work_keys_2 = site.get().things(q) return edition_keys + work_keys_1 + work_keys_2 @@ -146,7 +148,7 @@ def merge_docs(self, master, dup): return master def save(self, docs, master, duplicates): - return web.ctx.site.save_many( + return site.get().save_many( docs, comment="merge authors", action="merge-authors", @@ -200,7 +202,7 @@ def process(doc): doc["table_of_contents"] = fix_table_of_contents(doc["table_of_contents"]) return doc - return [process(thing.dict()) for thing in web.ctx.site.get_many(list(keys))] + return [process(thing.dict()) for thing in site.get().get_many(list(keys))] def make_redirect_doc(key, redirect): @@ -211,11 +213,11 @@ class merge_authors(delegate.page): path = "/authors/merge" def is_enabled(self): - user = web.ctx.site.get_user() + user = site.get().get_user() return "merge-authors" in web.ctx.features or (user and user.is_admin()) def filter_authors(self, keys): - docs = web.ctx.site.get_many(["/authors/" + k for k in keys]) + docs = site.get().get_many(["/authors/" + k for k in keys]) d = {doc.key: doc.type.key for doc in docs} return [k for k in keys if d.get("/authors/" + k) == "/type/author"] @@ -305,6 +307,7 @@ def POST(self): raise web.seeother(redir_url) +@deprecated("migrated to fastapi") class merge_authors_json(delegate.page): """JSON API for merge authors. @@ -315,7 +318,7 @@ class merge_authors_json(delegate.page): encoding = "json" def is_enabled(self): - user = web.ctx.site.get_user() + user = site.get().get_user() return "merge-authors" in web.ctx.features or (user and user.is_admin()) def POST(self): diff --git a/openlibrary/plugins/upstream/tests/test_merge_authors.py b/openlibrary/plugins/upstream/tests/test_merge_authors.py index 9406563dd0d..2d9c8d9a9c3 100644 --- a/openlibrary/plugins/upstream/tests/test_merge_authors.py +++ b/openlibrary/plugins/upstream/tests/test_merge_authors.py @@ -12,6 +12,7 @@ space_squash_and_strip, ) from openlibrary.utils import dicthash +from openlibrary.utils.request_context import site def setup_module(mod): @@ -137,6 +138,7 @@ def test_merge_property(self): def test_get_many(): web.ctx.site = MockSite() + site.set(web.ctx.site) # get_many should handle bad table_of_contents in the edition. edition = { "key": "/books/OL1M", @@ -158,6 +160,7 @@ def test_get_many(): class TestAuthorRedirectEngine: def setup_method(self, method): web.ctx.site = MockSite() + site.set(web.ctx.site) def test_fix_edition(self): update_references = AuthorRedirectEngine().update_references @@ -223,6 +226,7 @@ class TestAuthorMergeEngine: def setup_method(self, method): self.engine = AuthorMergeEngine(AuthorRedirectEngine()) web.ctx.site = MockSite() + site.set(web.ctx.site) def test_redirection(self): web.ctx.site.add([TEST_AUTHORS.a, TEST_AUTHORS.b, TEST_AUTHORS.c]) diff --git a/openlibrary/tests/fastapi/test_merge_authors.py b/openlibrary/tests/fastapi/test_merge_authors.py new file mode 100644 index 00000000000..be0a17ca5d0 --- /dev/null +++ b/openlibrary/tests/fastapi/test_merge_authors.py @@ -0,0 +1,281 @@ +"""Tests for POST /authors/merge.json (FastAPI merge_authors_json endpoint).""" + +from unittest.mock import MagicMock, patch + +import pytest + +from infogami.infobase.client import ClientException +from openlibrary.utils.request_context import RequestContextVars, req_context, site + + +@pytest.fixture(autouse=True) +def _setup_request_context(): + """Set ContextVars so require_librarian -> get_current_user() works.""" + site.set(MagicMock()) + req_context.set( + RequestContextVars( + x_forwarded_for=None, + user_agent=None, + lang="en", + solr_editions=True, + print_disabled=False, + ) + ) + + +@pytest.fixture +def mock_user_factory(monkeypatch): + """Factory fixture to create and mock users with configurable roles. + + Usage: + mock_user_factory(is_admin=True) + """ + + def create_user( + is_admin: bool = False, + is_librarian: bool = False, + is_super_librarian: bool = False, + ): + user = MagicMock() + user.is_admin.return_value = is_admin + user.is_librarian.return_value = is_librarian + user.is_super_librarian.return_value = is_super_librarian + monkeypatch.setattr( + "openlibrary.fastapi.auth.get_current_user", + lambda: user, + ) + return user + + return create_user + + +@pytest.fixture +def mock_merge_engine(): + """Mock the AuthorMergeEngine to avoid actual DB calls.""" + with patch( + "openlibrary.fastapi.merge_authors.AuthorMergeEngine", + autospec=True, + ) as mock: + instance = mock.return_value + instance.merge.return_value = [ + {"key": "/authors/OL1A", "revision": 2}, + ] + yield instance + + +@pytest.fixture +def mock_process_merge_request(): + """Mock process_merge_request to avoid actual merge request creation.""" + with patch( + "openlibrary.fastapi.merge_authors.process_merge_request", + autospec=True, + ) as mock: + yield mock + + +class TestMergeAuthorsJson: + def test_merge_success( + self, + fastapi_client, + mock_authenticated_user, + mock_user_factory, + mock_merge_engine, + mock_process_merge_request, + ): + """Admin user can merge authors successfully.""" + mock_user_factory(is_admin=True) + + response = fastapi_client.post( + "/authors/merge.json", + json={ + "master": "/authors/OL1A", + "duplicates": ["/authors/OL2A", "/authors/OL3A"], + "comment": "merging duplicates", + }, + ) + assert response.status_code == 200 + assert response.json() == [ + {"key": "/authors/OL1A", "revision": 2}, + ] + mock_merge_engine.merge.assert_called_once_with( + "/authors/OL1A", + ["/authors/OL2A", "/authors/OL3A"], + ) + mock_process_merge_request.assert_called_once() + + def test_merge_success_with_mrid( + self, + fastapi_client, + mock_authenticated_user, + mock_user_factory, + mock_merge_engine, + mock_process_merge_request, + ): + """Admin user can merge authors with an existing merge request ID.""" + mock_user_factory(is_admin=True) + + response = fastapi_client.post( + "/authors/merge.json", + json={ + "master": "/authors/OL1A", + "duplicates": ["/authors/OL2A"], + "mrid": "123", + "comment": "approving merge request", + }, + ) + assert response.status_code == 200 + mock_process_merge_request.assert_called_once_with( + "update-request", + {"action": "approve", "mrid": "123", "comment": "approving merge request"}, + ) + + def test_merge_success_without_mrid_creates_request( + self, + fastapi_client, + mock_authenticated_user, + mock_user_factory, + mock_merge_engine, + mock_process_merge_request, + ): + """When no mrid is provided, a new merge request record is created.""" + mock_user_factory(is_admin=True) + + response = fastapi_client.post( + "/authors/merge.json", + json={ + "master": "/authors/OL1A", + "duplicates": ["/authors/OL2A"], + "olids": "/authors/OL1A,/authors/OL2A", + }, + ) + assert response.status_code == 200 + mock_process_merge_request.assert_called_once_with( + "create-request", + { + "mr_type": 2, + "olids": "/authors/OL1A,/authors/OL2A", + "action": "create-merged", + }, + ) + + def test_merge_unauthenticated(self, fastapi_client): + """Unauthenticated request returns 401 (from require_authenticated_user).""" + response = fastapi_client.post( + "/authors/merge.json", + json={ + "master": "/authors/OL1A", + "duplicates": ["/authors/OL2A"], + }, + ) + assert response.status_code == 401 + + def test_merge_forbidden_non_librarian( + self, + fastapi_client, + mock_authenticated_user, + mock_user_factory, + ): + """Regular user without librarian/admin role cannot merge authors.""" + mock_user_factory( + is_admin=False, + is_librarian=False, + is_super_librarian=False, + ) + + response = fastapi_client.post( + "/authors/merge.json", + json={ + "master": "/authors/OL1A", + "duplicates": ["/authors/OL2A"], + }, + ) + assert response.status_code == 403 + + def test_merge_allowed_for_librarian( + self, + fastapi_client, + mock_authenticated_user, + mock_user_factory, + mock_merge_engine, + mock_process_merge_request, + ): + """Librarian (not admin) can merge authors.""" + mock_user_factory( + is_admin=False, + is_librarian=True, + is_super_librarian=False, + ) + + response = fastapi_client.post( + "/authors/merge.json", + json={ + "master": "/authors/OL1A", + "duplicates": ["/authors/OL2A"], + }, + ) + assert response.status_code == 200 + + def test_merge_allowed_for_super_librarian( + self, + fastapi_client, + mock_authenticated_user, + mock_user_factory, + mock_merge_engine, + mock_process_merge_request, + ): + """Super librarian can merge authors.""" + mock_user_factory( + is_admin=False, + is_librarian=False, + is_super_librarian=True, + ) + + response = fastapi_client.post( + "/authors/merge.json", + json={ + "master": "/authors/OL1A", + "duplicates": ["/authors/OL2A"], + }, + ) + assert response.status_code == 200 + + def test_merge_raises_400_on_client_exception( + self, + fastapi_client, + mock_authenticated_user, + mock_user_factory, + mock_merge_engine, + mock_process_merge_request, + ): + """ClientException from AuthorMergeEngine returns 400 with parsed error body.""" + mock_user_factory(is_admin=True) + mock_merge_engine.merge.side_effect = ClientException( + "400 Bad Request", + "internal error", + '{"error": "not_found", "message": "internal error"}', + ) + response = fastapi_client.post( + "/authors/merge.json", + json={"master": "/authors/OL1A", "duplicates": ["/authors/OL999A"]}, + ) + assert response.status_code == 400 + assert response.json() == {"detail": {"error": "not_found", "message": "internal error"}} + + def test_merge_raises_400_when_retries_exhausted( + self, + fastapi_client, + mock_authenticated_user, + mock_user_factory, + mock_merge_engine, + mock_process_merge_request, + monkeytime, + ): + """When process_merge_request keeps failing, retries are exhausted and 400 is returned.""" + mock_user_factory(is_admin=True) + mock_process_merge_request.side_effect = ClientException("400 Bad Request", "retry error") + response = fastapi_client.post( + "/authors/merge.json", + json={"master": "/authors/OL1A", "duplicates": ["/authors/OL2A"]}, + ) + assert response.status_code == 400 + assert "retry error" in response.json()["detail"] From e2dc59d12ab7c4ec95be4e701254655e1567f3c7 Mon Sep 17 00:00:00 2001 From: RayBB Date: Tue, 16 Jun 2026 18:03:57 -0700 Subject: [PATCH 2/5] inline one function --- openlibrary/fastapi/merge_authors.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/openlibrary/fastapi/merge_authors.py b/openlibrary/fastapi/merge_authors.py index 54f195c724b..16bb4bfc3fd 100644 --- a/openlibrary/fastapi/merge_authors.py +++ b/openlibrary/fastapi/merge_authors.py @@ -33,17 +33,14 @@ class MergeAuthorsBody(BaseModel): @router.post("/authors/merge.json") def merge_authors_json(_: LibrarianDep, data: MergeAuthorsBody) -> Any: - def merge_records() -> Any: - try: - engine = AuthorMergeEngine(AuthorRedirectEngine()) - return engine.merge(data.master, data.duplicates) - except ClientException as e: - raise HTTPException( - status_code=400, - detail=json.loads(e.json), - ) - - merge_result = merge_records() + try: + engine = AuthorMergeEngine(AuthorRedirectEngine()) + merge_result = engine.merge(data.master, data.duplicates) + except ClientException as e: + raise HTTPException( + status_code=400, + detail=json.loads(e.json), + ) def update_request() -> None: if data.mrid: From 3ee568a561b687f301511b9057d00955feecb954 Mon Sep 17 00:00:00 2001 From: RayBB Date: Tue, 16 Jun 2026 18:15:20 -0700 Subject: [PATCH 3/5] use web_ctx_ip so merging is successful --- openlibrary/fastapi/merge_authors.py | 60 ++++++++++++++-------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/openlibrary/fastapi/merge_authors.py b/openlibrary/fastapi/merge_authors.py index 16bb4bfc3fd..a0d3a998ea8 100644 --- a/openlibrary/fastapi/merge_authors.py +++ b/openlibrary/fastapi/merge_authors.py @@ -18,6 +18,7 @@ AuthorMergeEngine, AuthorRedirectEngine, ) +from openlibrary.utils.request_context import req_context, web_ctx_ip from openlibrary.utils.retry import MaxRetriesExceeded, RetryStrategy router = APIRouter(tags=["merge-authors"]) @@ -33,36 +34,37 @@ class MergeAuthorsBody(BaseModel): @router.post("/authors/merge.json") def merge_authors_json(_: LibrarianDep, data: MergeAuthorsBody) -> Any: - try: - engine = AuthorMergeEngine(AuthorRedirectEngine()) - merge_result = engine.merge(data.master, data.duplicates) - except ClientException as e: - raise HTTPException( - status_code=400, - detail=json.loads(e.json), - ) + with web_ctx_ip(req_context.get().x_forwarded_for or "127.0.0.1"): + try: + engine = AuthorMergeEngine(AuthorRedirectEngine()) + merge_result = engine.merge(data.master, data.duplicates) + except ClientException as e: + raise HTTPException( + status_code=400, + detail=json.loads(e.json), + ) - def update_request() -> None: - if data.mrid: - rtype = "update-request" - update_data: dict[str, Any] = {"action": "approve", "mrid": data.mrid} - else: - rtype = "create-request" - update_data = {"mr_type": 2, "olids": data.olids, "action": "create-merged"} - if data.comment: - update_data["comment"] = data.comment - process_merge_request(rtype, update_data) + def update_request() -> None: + if data.mrid: + rtype = "update-request" + update_data: dict[str, Any] = {"action": "approve", "mrid": data.mrid} + else: + rtype = "create-request" + update_data = {"mr_type": 2, "olids": data.olids, "action": "create-merged"} + if data.comment: + update_data["comment"] = data.comment + process_merge_request(rtype, update_data) - try: - RetryStrategy( - [ClientException], - max_retries=5, - delay=2, - )(update_request) - except MaxRetriesExceeded as e: - raise HTTPException( - status_code=400, - detail=str(e.last_exception), - ) + try: + RetryStrategy( + [ClientException], + max_retries=5, + delay=2, + )(update_request) + except MaxRetriesExceeded as e: + raise HTTPException( + status_code=400, + detail=str(e.last_exception), + ) return merge_result From 49740e6205a54e74842e50a1a562da977f5d9ae8 Mon Sep 17 00:00:00 2001 From: RayBB Date: Tue, 16 Jun 2026 18:26:39 -0700 Subject: [PATCH 4/5] factor out orchestration code --- openlibrary/fastapi/merge_authors.py | 21 ++-------- openlibrary/plugins/upstream/edits.py | 41 +++++++++++++++++++ openlibrary/plugins/upstream/merge_authors.py | 24 ++--------- .../tests/fastapi/test_merge_authors.py | 2 +- 4 files changed, 48 insertions(+), 40 deletions(-) diff --git a/openlibrary/fastapi/merge_authors.py b/openlibrary/fastapi/merge_authors.py index a0d3a998ea8..f17dc16a616 100644 --- a/openlibrary/fastapi/merge_authors.py +++ b/openlibrary/fastapi/merge_authors.py @@ -13,13 +13,13 @@ from infogami.infobase.client import ClientException from openlibrary.fastapi.auth import LibrarianDep # noqa: TC001 -from openlibrary.plugins.upstream.edits import process_merge_request +from openlibrary.plugins.upstream.edits import perform_merge_update from openlibrary.plugins.upstream.merge_authors import ( AuthorMergeEngine, AuthorRedirectEngine, ) from openlibrary.utils.request_context import req_context, web_ctx_ip -from openlibrary.utils.retry import MaxRetriesExceeded, RetryStrategy +from openlibrary.utils.retry import MaxRetriesExceeded router = APIRouter(tags=["merge-authors"]) @@ -44,23 +44,8 @@ def merge_authors_json(_: LibrarianDep, data: MergeAuthorsBody) -> Any: detail=json.loads(e.json), ) - def update_request() -> None: - if data.mrid: - rtype = "update-request" - update_data: dict[str, Any] = {"action": "approve", "mrid": data.mrid} - else: - rtype = "create-request" - update_data = {"mr_type": 2, "olids": data.olids, "action": "create-merged"} - if data.comment: - update_data["comment"] = data.comment - process_merge_request(rtype, update_data) - try: - RetryStrategy( - [ClientException], - max_retries=5, - delay=2, - )(update_request) + perform_merge_update(mrid=data.mrid, olids=data.olids, comment=data.comment) except MaxRetriesExceeded as e: raise HTTPException( status_code=400, diff --git a/openlibrary/plugins/upstream/edits.py b/openlibrary/plugins/upstream/edits.py index 30018161160..d8147623051 100644 --- a/openlibrary/plugins/upstream/edits.py +++ b/openlibrary/plugins/upstream/edits.py @@ -5,11 +5,13 @@ import web +from infogami.infobase.client import ClientException from infogami.utils import delegate from infogami.utils.view import render_template from openlibrary import accounts from openlibrary.core.edits import ApiMode, CommunityEditsQueue, get_status_for_view from openlibrary.utils.request_context import site +from openlibrary.utils.retry import RetryStrategy # Usergroups that may use create or update merge requests ALLOWED_USERGROUPS: list[str] = [ @@ -36,6 +38,45 @@ def process_merge_request(rtype, data): return resp +def perform_merge_update(mrid: str | None, olids: str, comment: str | None) -> None: + """Orchestrate the merge request update with retries. + + Builds the appropriate request type and data, then attempts to process + the merge request with retry logic. + + Args: + mrid: Merge request ID to approve, or None to create a new request. + olids: Comma-separated author keys (used when creating a new request). + comment: Optional comment for the merge request. + + Raises: + MaxRetriesExceeded: If all retry attempts to process the request fail. + """ + + def update_request() -> None: + if mrid: + process_merge_request( + "update-request", + { + "action": "approve", + "mrid": mrid, + **({"comment": comment} if comment else {}), + }, + ) + else: + process_merge_request( + "create-request", + { + "mr_type": 2, + "olids": olids, + "action": "create-merged", + **({"comment": comment} if comment else {}), + }, + ) + + RetryStrategy([ClientException], max_retries=5, delay=2)(update_request) + + class community_edits_queue(delegate.page): path = "/merges" diff --git a/openlibrary/plugins/upstream/merge_authors.py b/openlibrary/plugins/upstream/merge_authors.py index a55eb08faca..d1f9ff309a8 100644 --- a/openlibrary/plugins/upstream/merge_authors.py +++ b/openlibrary/plugins/upstream/merge_authors.py @@ -11,11 +11,11 @@ from infogami.utils import delegate from infogami.utils.view import render_template, safeint from openlibrary.accounts import get_current_user -from openlibrary.plugins.upstream.edits import process_merge_request +from openlibrary.plugins.upstream.edits import perform_merge_update, process_merge_request from openlibrary.plugins.worksearch.code import top_books_from_author from openlibrary.utils import dicthash, uniq from openlibrary.utils.request_context import site -from openlibrary.utils.retry import MaxRetriesExceeded, RetryStrategy +from openlibrary.utils.retry import MaxRetriesExceeded class BasicRedirectEngine: @@ -336,29 +336,11 @@ def merge_records() -> Any: except ClientException as e: raise web.badrequest(json.loads(e.json)) - def update_request() -> None: - data = {} - if mrid: - # Update the request - rtype = "update-request" - data = {"action": "approve", "mrid": mrid} - else: - # Create new request - rtype = "create-request" - data = {"mr_type": 2, "olids": olids, "action": "create-merged"} - if comment: - data["comment"] = comment - process_merge_request(rtype, data) - # actually perform merge and save affected records to db merge_result = merge_records() # attempt to update the merge request status with retries try: - RetryStrategy( - [ClientException], - max_retries=5, - delay=2, - )(update_request) + perform_merge_update(mrid=mrid, olids=olids, comment=comment) except MaxRetriesExceeded as e: raise web.badrequest(str(e.last_exception)) return delegate.RawText(json.dumps(merge_result), content_type="application/json") diff --git a/openlibrary/tests/fastapi/test_merge_authors.py b/openlibrary/tests/fastapi/test_merge_authors.py index be0a17ca5d0..2b65f93e254 100644 --- a/openlibrary/tests/fastapi/test_merge_authors.py +++ b/openlibrary/tests/fastapi/test_merge_authors.py @@ -67,7 +67,7 @@ def mock_merge_engine(): def mock_process_merge_request(): """Mock process_merge_request to avoid actual merge request creation.""" with patch( - "openlibrary.fastapi.merge_authors.process_merge_request", + "openlibrary.plugins.upstream.edits.process_merge_request", autospec=True, ) as mock: yield mock From 6be06311ff822dce637ec7a084a5fd7d075e79ee Mon Sep 17 00:00:00 2001 From: RayBB Date: Tue, 16 Jun 2026 18:31:47 -0700 Subject: [PATCH 5/5] add missing content type --- openlibrary/plugins/openlibrary/js/merge.js | 1 + 1 file changed, 1 insertion(+) diff --git a/openlibrary/plugins/openlibrary/js/merge.js b/openlibrary/plugins/openlibrary/js/merge.js index 87586e34e1a..3895c36940e 100644 --- a/openlibrary/plugins/openlibrary/js/merge.js +++ b/openlibrary/plugins/openlibrary/js/merge.js @@ -91,6 +91,7 @@ export function initAuthorView() { $.ajax({ url: '/authors/merge.json', type: 'POST', + contentType: 'application/json', data: JSON.stringify(data), error: function() { $('#preMerge').fadeOut();