Prevent duplicate bookmark names

modules/backend/src/main/scala/docspell/backend/ops/OQueryBookmarks.scala

@@ -57,14 +57,11 @@ object OQueryBookmarks {
             _.map(r => Bookmark(r.id, r.name, r.label, r.query, r.isPersonal, r.created))
           )
 
-      def create(account: AccountId, b: NewBookmark): F[AddResult] =
+      def create(account: AccountId, b: NewBookmark): F[AddResult] = {
-        store
+        val record =
-          .transact(for {
+          RQueryBookmark.createNew(account, b.name, b.label, b.query, b.personal)
-            r <- RQueryBookmark.createNew(account, b.name, b.label, b.query, b.personal)
+        store.transact(RQueryBookmark.insertIfNotExists(account, record))
-            n <- RQueryBookmark.insert(r)
+      }
-          } yield n)
-          .attempt
-          .map(AddResult.fromUpdate)
 
       def update(account: AccountId, id: Ident, b: NewBookmark): F[UpdateResult] =
         UpdateResult.fromUpdate(

modules/store/src/main/resources/db/migration/h2/V1.31.0__query_bookmark.sql

@@ -6,7 +6,8 @@ CREATE TABLE "query_bookmark" (
   "cid" varchar(254) not null,
   "query" varchar(2000) not null,
   "created" timestamp,
+  "__user_id" varchar(254) not null,
   foreign key ("user_id") references "user_"("uid") on delete cascade,
   foreign key ("cid") references "collective"("cid") on delete cascade,
-  unique("cid", "user_id", "name")
+  unique("cid", "__user_id", "name")
 )

modules/store/src/main/resources/db/migration/mariadb/V1.31.0__query_bookmark.sql

@@ -6,7 +6,8 @@ CREATE TABLE `query_bookmark` (
   `cid` varchar(254) not null,
   `query` varchar(2000) not null,
   `created` timestamp,
+  `__user_id` varchar(254) not null,
   foreign key (`user_id`) references `user_`(`uid`) on delete cascade,
   foreign key (`cid`) references `collective`(`cid`) on delete cascade,
-  unique(`cid`, `user_id`, `name`)
+  unique(`cid`, `__user_id`, `name`)
 )

modules/store/src/main/resources/db/migration/postgresql/V1.31.0__query_bookmark.sql

@@ -6,7 +6,8 @@ CREATE TABLE "query_bookmark" (
   "cid" varchar(254) not null,
   "query" varchar(2000) not null,
   "created" timestamp,
+  "__user_id" varchar(254) not null,
   foreign key ("user_id") references "user_"("uid") on delete cascade,
   foreign key ("cid") references "collective"("cid") on delete cascade,
-  unique("cid", "user_id", "name")
+  unique("cid", "__user_id", "name")
 )

modules/store/src/main/scala/docspell/store/records/RQueryBookmark.scala

@@ -7,7 +7,7 @@
 package docspell.store.records
 
 import cats.data.NonEmptyList
-import cats.syntax.option._
+import cats.syntax.all._
 
 import docspell.common._
 import docspell.query.ItemQuery
@@ -16,6 +16,7 @@ import docspell.store.qb._
 
 import doobie._
 import doobie.implicits._
+import docspell.store.AddResult
 
 final case class RQueryBookmark(
     id: Ident,
@@ -48,6 +49,8 @@ object RQueryBookmark {
     val query = Column[ItemQuery]("query", this)
     val created = Column[Timestamp]("created", this)
 
+    val internUserId = Column[String]("__user_id", this)
+
     val all: NonEmptyList[Column[_]] =
       NonEmptyList.of(id, name, label, userId, cid, query, created)
   }
@@ -76,12 +79,14 @@ object RQueryBookmark {
       curTime
     )
 
-  def insert(r: RQueryBookmark): ConnectionIO[Int] =
+  def insert(r: RQueryBookmark): ConnectionIO[Int] = {
+    val userIdDummy = r.userId.getOrElse(Ident.unsafe("-"))
     DML.insert(
       T,
-      T.all,
+      T.all.append(T.internUserId),
-      sql"${r.id},${r.name},${r.label},${r.userId},${r.cid},${r.query},${r.created}"
+      sql"${r.id},${r.name},${r.label},${r.userId},${r.cid},${r.query},${r.created},$userIdDummy"
     )
+  }
 
   def update(r: RQueryBookmark): ConnectionIO[Int] =
     DML.update(
@@ -97,6 +102,42 @@ object RQueryBookmark {
   def deleteById(cid: Ident, id: Ident): ConnectionIO[Int] =
     DML.delete(T, T.id === id && T.cid === cid)
 
+  def nameExists(account: AccountId, name: String): ConnectionIO[Boolean] = {
+    val user = RUser.as("u")
+    val bm = RQueryBookmark.as("bm")
+
+    val users = Select(
+      user.uid.s,
+      from(user),
+      user.cid === account.collective && user.login === account.user
+    )
+    Select(
+      select(count(bm.id)),
+      from(bm),
+      bm.name === name && bm.cid === account.collective && (bm.userId.isNull || bm.userId
+        .in(users))
+    ).build.query[Int].unique.map(_ > 0)
+  }
+
+  // impl note: store.add doesn't work, because it checks for duplicate
+  // after trying to insert the check is necessary because a name
+  // should be unique across personal *and* collective bookmarks
+  def insertIfNotExists(
+      account: AccountId,
+      r: ConnectionIO[RQueryBookmark]
+  ): ConnectionIO[AddResult] =
+    for {
+      bm <- r
+      res <-
+        nameExists(account, bm.name).flatMap {
+          case true =>
+            AddResult
+              .entityExists(s"A bookmark '${bm.name}' already exists.")
+              .pure[ConnectionIO]
+          case false => insert(bm).attempt.map(AddResult.fromUpdate)
+        }
+    } yield res
+
   def allForUser(account: AccountId): ConnectionIO[Vector[RQueryBookmark]] = {
     val user = RUser.as("u")
     val bm = RQueryBookmark.as("bm")

modules/webapp/src/main/elm/Comp/BookmarkQueryForm.elm

@@ -76,9 +76,11 @@ initWith bm =
 
 isValid : Model -> Bool
 isValid model =
-    Comp.PowerSearchInput.isValid model.queryModel
+    List.all identity
-        && model.name
+        [ Comp.PowerSearchInput.isValid model.queryModel
-        /= Nothing
+        , model.name /= Nothing
+        , not model.nameExists
+        ]
 
 
 get : Model -> Maybe BookmarkedQuery
@@ -116,14 +118,6 @@ update flags msg model =
         nameCheck1 name =
             Api.bookmarkNameExists flags name NameExistsResp
 
-        nameCheck2 =
-            case model.name of
-                Just n ->
-                    Api.bookmarkNameExists flags n NameExistsResp
-
-                Nothing ->
-                    Cmd.none
-
         throttleSub =
             Throttle.ifNeeded
                 (Time.every 150 (\_ -> UpdateThrottle))
@@ -141,11 +135,7 @@ update flags msg model =
             )
 
         SetPersonal flag ->
-            let
+            ( { model | isPersonal = flag }, Cmd.none, Sub.none )
-                ( newThrottle, cmd ) =
-                    Throttle.try nameCheck2 model.nameExistsThrottle
-            in
-            ( { model | isPersonal = flag, nameExistsThrottle = newThrottle }, cmd, throttleSub )
 
         QueryMsg lm ->
             let
@@ -218,7 +208,7 @@ view texts model =
                 ]
                 []
             , span
-                [ class S.infoMessagePlain
+                [ class S.warnMessagePlain
                 , class "font-medium text-sm"
                 , classList [ ( "invisible", not model.nameExists ) ]
                 ]

modules/webapp/src/main/elm/Messages/Comp/BookmarkQueryForm.elm

@@ -33,7 +33,7 @@ gb =
     , userLocationText = "The bookmarked query is just for you"
     , collectiveLocation = "Collective scope"
     , collectiveLocationText = "The bookmarked query can be used and edited by all users"
-    , nameExistsWarning = "A bookmark with this name exists, it is overwritten on save!"
+    , nameExistsWarning = "A bookmark with this name exists!"
     }
 
 
@@ -45,5 +45,5 @@ de =
     , userLocationText = "Der Bookmark ist nur für dich"
     , collectiveLocation = "Kollektiv-Bookmark"
     , collectiveLocationText = "Der Bookmark kann von allen Benutzer verwendet werden"
-    , nameExistsWarning = "Der Bookmark existiert bereits. Er wird beim Speichern überschrieben."
+    , nameExistsWarning = "Der Bookmark existiert bereits!"
     }