Filter on library when checking lane name unique (PP-1967) (#2190)

This PR does a small bit of refactoring to DRY the check we use for lane name uniqueness and fixes a bug where the check for lane name uniqueness

Previously one of the checks was missing a filter on the lanes library. Making it so that you couldn't create a lane with the same name as any other lane in the CM, rather then any other lane with the same library.

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)

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."
     ),
 )
 

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,30 +189,34 @@ 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),
                     ("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"
-        ) 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: