diff --git a/src/palace/manager/api/admin/controller/lanes.py b/src/palace/manager/api/admin/controller/lanes.py index eafe7a906..e2fb9b30a 100644 --- a/src/palace/manager/api/admin/controller/lanes.py +++ b/src/palace/manager/api/admin/controller/lanes.py @@ -25,6 +25,7 @@ from palace.manager.sqlalchemy.model.lane import Lane from palace.manager.sqlalchemy.model.library import Library from palace.manager.sqlalchemy.util import create, get_one +from palace.manager.util.problem_detail import ProblemDetailException class LanesController(CirculationManagerController, AdminPermissionsControllerMixin): @@ -92,12 +93,8 @@ def lanes_for_parent(parent): return NO_CUSTOM_LISTS_FOR_LANE if display_name != lane.display_name: - old_lane = get_one( - self._db, Lane, display_name=display_name, parent=lane.parent - ) - if old_lane: - return LANE_WITH_PARENT_AND_DISPLAY_NAME_ALREADY_EXISTS - lane.display_name = display_name + self._check_lane_name_unique(display_name, lane.parent, library) + lane.display_name = display_name else: if not custom_list_ids or len(custom_list_ids) == 0: return NO_CUSTOM_LISTS_FOR_LANE @@ -111,15 +108,7 @@ def lanes_for_parent(parent): "The specified parent lane does not exist, or is associated with a different library." ) ) - old_lane = get_one( - self._db, - Lane, - display_name=display_name, - parent=parent, - library=library, - ) - if old_lane: - return LANE_WITH_PARENT_AND_DISPLAY_NAME_ALREADY_EXISTS + self._check_lane_name_unique(display_name, parent, library) lane, is_new = create( self._db, @@ -192,6 +181,17 @@ def delete_lane_and_sublanes(lane): delete_lane_and_sublanes(lane) return Response(str(_("Deleted")), 200) + def _check_lane_name_unique( + self, display_name: str, parent: Lane | None, library: Library + ) -> None: + lane_with_same_name = get_one( + self._db, Lane, display_name=display_name, parent=parent, library=library + ) + if lane_with_same_name: + raise ProblemDetailException( + LANE_WITH_PARENT_AND_DISPLAY_NAME_ALREADY_EXISTS + ) + def show_lane(self, lane_identifier): library = get_request_library() self.require_library_manager(library) diff --git a/src/palace/manager/api/admin/problem_details.py b/src/palace/manager/api/admin/problem_details.py index 15cd0d143..e209e69ae 100644 --- a/src/palace/manager/api/admin/problem_details.py +++ b/src/palace/manager/api/admin/problem_details.py @@ -462,9 +462,9 @@ LANE_WITH_PARENT_AND_DISPLAY_NAME_ALREADY_EXISTS = pd( "http://librarysimplified.org/terms/problem/lane-with-parent-and-display-name-already-exists", status_code=400, - title=_("Lane with parent and display name already exists"), + title=_("Lane with parent and display name already exists for this library"), detail=_( - "You cannot create a lane with the same parent and display name as an existing lane." + "You cannot create a lane with the same parent and display name as an existing lane within the same library." ), ) diff --git a/tests/manager/api/admin/controller/test_lanes.py b/tests/manager/api/admin/controller/test_lanes.py index 140b3b8c3..095006cba 100644 --- a/tests/manager/api/admin/controller/test_lanes.py +++ b/tests/manager/api/admin/controller/test_lanes.py @@ -21,6 +21,7 @@ from palace.manager.sqlalchemy.util import get_one from tests.fixtures.api_admin import AdminControllerFixture from tests.fixtures.api_controller import ControllerFixture +from tests.fixtures.problem_detail import raises_problem_detail class AdminLibraryManagerFixture(AdminControllerFixture): @@ -188,9 +189,12 @@ def test_lanes_post_errors(self, alm_fixture: AdminLibraryManagerFixture): lane2 = alm_fixture.ctrl.db.lane("lane2") lane1.customlists += [list] - with alm_fixture.request_context_with_library_and_admin( - "/", method="POST" - ) as ctx: + with ( + alm_fixture.request_context_with_library_and_admin( + "/", method="POST" + ) as ctx, + raises_problem_detail(pd=LANE_WITH_PARENT_AND_DISPLAY_NAME_ALREADY_EXISTS), + ): ctx.request.form = ImmutableMultiDict( [ ("id", lane1.id), @@ -198,20 +202,21 @@ def test_lanes_post_errors(self, alm_fixture: AdminLibraryManagerFixture): ("custom_list_ids", json.dumps([list.id])), ] ) - response = alm_fixture.manager.admin_lanes_controller.lanes() - assert LANE_WITH_PARENT_AND_DISPLAY_NAME_ALREADY_EXISTS == response + alm_fixture.manager.admin_lanes_controller.lanes() - with alm_fixture.request_context_with_library_and_admin( - "/", method="POST" - ) as ctx: + with ( + alm_fixture.request_context_with_library_and_admin( + "/", method="POST" + ) as ctx, + raises_problem_detail(pd=LANE_WITH_PARENT_AND_DISPLAY_NAME_ALREADY_EXISTS), + ): ctx.request.form = ImmutableMultiDict( [ ("display_name", "lane2"), ("custom_list_ids", json.dumps([list.id])), ] ) - response = alm_fixture.manager.admin_lanes_controller.lanes() - assert LANE_WITH_PARENT_AND_DISPLAY_NAME_ALREADY_EXISTS == response + alm_fixture.manager.admin_lanes_controller.lanes() with alm_fixture.request_context_with_library_and_admin( "/", method="POST" @@ -345,6 +350,10 @@ def test_lanes_edit(self, alm_fixture: AdminLibraryManagerFixture): # are two works in the lane. assert 0 == lane.size + # We create a lane with the same name in a different library, this should not cause + # an error, as we only check for duplicate names within the same library. + alm_fixture.ctrl.db.lane("new name", library=alm_fixture.ctrl.db.library()) + with alm_fixture.request_context_with_library_and_admin( "/", method="POST" ) as ctx: