From ccb4df5bd7dbd2a83a1eba347eaa49545971ba93 Mon Sep 17 00:00:00 2001
From: eikek <eike.kettner@posteo.de>
Date: Mon, 10 Jan 2022 00:36:57 +0100
Subject: [PATCH] Prevent duplicate bookmark names
---
.../backend/ops/OQueryBookmarks.scala | 13 ++---
.../migration/h2/V1.31.0__query_bookmark.sql | 3 +-
.../mariadb/V1.31.0__query_bookmark.sql | 3 +-
.../postgresql/V1.31.0__query_bookmark.sql | 3 +-
.../store/records/RQueryBookmark.scala | 49 +++++++++++++++++--
.../src/main/elm/Comp/BookmarkQueryForm.elm | 24 +++------
.../elm/Messages/Comp/BookmarkQueryForm.elm | 4 +-
7 files changed, 65 insertions(+), 34 deletions(-)
diff --git a/modules/backend/src/main/scala/docspell/backend/ops/OQueryBookmarks.scala b/modules/backend/src/main/scala/docspell/backend/ops/OQueryBookmarks.scala
index 72ec9de8..8b8976ce 100644
--- a/modules/backend/src/main/scala/docspell/backend/ops/OQueryBookmarks.scala
+++ b/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] =
- store
- .transact(for {
- r <- RQueryBookmark.createNew(account, b.name, b.label, b.query, b.personal)
- n <- RQueryBookmark.insert(r)
- } yield n)
- .attempt
- .map(AddResult.fromUpdate)
+ def create(account: AccountId, b: NewBookmark): F[AddResult] = {
+ val record =
+ RQueryBookmark.createNew(account, b.name, b.label, b.query, b.personal)
+ store.transact(RQueryBookmark.insertIfNotExists(account, record))
+ }
def update(account: AccountId, id: Ident, b: NewBookmark): F[UpdateResult] =
UpdateResult.fromUpdate(
diff --git a/modules/store/src/main/resources/db/migration/h2/V1.31.0__query_bookmark.sql b/modules/store/src/main/resources/db/migration/h2/V1.31.0__query_bookmark.sql
index f90c8e67..c98efc01 100644
--- a/modules/store/src/main/resources/db/migration/h2/V1.31.0__query_bookmark.sql
+++ b/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")
)
diff --git a/modules/store/src/main/resources/db/migration/mariadb/V1.31.0__query_bookmark.sql b/modules/store/src/main/resources/db/migration/mariadb/V1.31.0__query_bookmark.sql
index de37bcab..73b83810 100644
--- a/modules/store/src/main/resources/db/migration/mariadb/V1.31.0__query_bookmark.sql
+++ b/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`)
)
diff --git a/modules/store/src/main/resources/db/migration/postgresql/V1.31.0__query_bookmark.sql b/modules/store/src/main/resources/db/migration/postgresql/V1.31.0__query_bookmark.sql
index f90c8e67..c98efc01 100644
--- a/modules/store/src/main/resources/db/migration/postgresql/V1.31.0__query_bookmark.sql
+++ b/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")
)
diff --git a/modules/store/src/main/scala/docspell/store/records/RQueryBookmark.scala b/modules/store/src/main/scala/docspell/store/records/RQueryBookmark.scala
index 5dbf34a5..a89cd49b 100644
--- a/modules/store/src/main/scala/docspell/store/records/RQueryBookmark.scala
+++ b/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,
- sql"${r.id},${r.name},${r.label},${r.userId},${r.cid},${r.query},${r.created}"
+ T.all.append(T.internUserId),
+ 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")
diff --git a/modules/webapp/src/main/elm/Comp/BookmarkQueryForm.elm b/modules/webapp/src/main/elm/Comp/BookmarkQueryForm.elm
index e0251cc5..1cf6d8ea 100644
--- a/modules/webapp/src/main/elm/Comp/BookmarkQueryForm.elm
+++ b/modules/webapp/src/main/elm/Comp/BookmarkQueryForm.elm
@@ -76,9 +76,11 @@ initWith bm =
isValid : Model -> Bool
isValid model =
- Comp.PowerSearchInput.isValid model.queryModel
- && model.name
- /= Nothing
+ List.all identity
+ [ Comp.PowerSearchInput.isValid model.queryModel
+ , 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
- ( newThrottle, cmd ) =
- Throttle.try nameCheck2 model.nameExistsThrottle
- in
- ( { model | isPersonal = flag, nameExistsThrottle = newThrottle }, cmd, throttleSub )
+ ( { model | isPersonal = flag }, Cmd.none, Sub.none )
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 ) ]
]
diff --git a/modules/webapp/src/main/elm/Messages/Comp/BookmarkQueryForm.elm b/modules/webapp/src/main/elm/Messages/Comp/BookmarkQueryForm.elm
index 34639a5d..e5d3605f 100644
--- a/modules/webapp/src/main/elm/Messages/Comp/BookmarkQueryForm.elm
+++ b/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!"
}