From 8a4471343c8b901c05341a3b917a77a1fad8cabf Mon Sep 17 00:00:00 2001
From: eikek <eike.kettner@posteo.de>
Date: Sun, 16 Jan 2022 00:31:59 +0100
Subject: [PATCH] Fix personal/non-personal when updating bookmarks

---
 .../backend/ops/OQueryBookmarks.scala         | 21 +++++++++++++++----
 .../store/records/RQueryBookmark.scala        | 12 +++++++----
 2 files changed, 25 insertions(+), 8 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 242b0d17..f1ef2396 100644
--- a/modules/backend/src/main/scala/docspell/backend/ops/OQueryBookmarks.scala
+++ b/modules/backend/src/main/scala/docspell/backend/ops/OQueryBookmarks.scala
@@ -6,6 +6,7 @@
 
 package docspell.backend.ops
 
+import cats.data.OptionT
 import cats.effect._
 import cats.implicits._
 
@@ -14,7 +15,7 @@ import docspell.query.ItemQuery
 import docspell.store.AddResult
 import docspell.store.Store
 import docspell.store.UpdateResult
-import docspell.store.records.RQueryBookmark
+import docspell.store.records._
 
 trait OQueryBookmarks[F[_]] {
 
@@ -73,7 +74,14 @@ object OQueryBookmarks {
 
       def update(account: AccountId, id: Ident, b: NewBookmark): F[UpdateResult] =
         UpdateResult.fromUpdate(
-          store.transact(RQueryBookmark.update(convert.toRecord(account, id, b)))
+          store.transact {
+            (for {
+              userId <- OptionT(RUser.findIdByAccount(account))
+              n <- OptionT.liftF(
+                RQueryBookmark.update(convert.toRecord(account, id, userId, b))
+              )
+            } yield n).getOrElse(0)
+          }
         )
 
       def delete(account: AccountId, bookmark: Ident): F[Unit] =
@@ -85,12 +93,17 @@ object OQueryBookmarks {
     def toModel(r: RQueryBookmark): Bookmark =
       Bookmark(r.id, r.name, r.label, r.query, r.isPersonal, r.created)
 
-    def toRecord(account: AccountId, id: Ident, b: NewBookmark): RQueryBookmark =
+    def toRecord(
+        account: AccountId,
+        id: Ident,
+        userId: Ident,
+        b: NewBookmark
+    ): RQueryBookmark =
       RQueryBookmark(
         id,
         b.name,
         b.label,
-        None, // userId and some other values are not used
+        if (b.personal) userId.some else None,
         account.collective,
         b.query,
         Timestamp.Epoch
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 b16d5dd1..08d21b71 100644
--- a/modules/store/src/main/scala/docspell/store/records/RQueryBookmark.scala
+++ b/modules/store/src/main/scala/docspell/store/records/RQueryBookmark.scala
@@ -95,7 +95,8 @@ object RQueryBookmark {
       DML.set(
         T.name.setTo(r.name),
         T.label.setTo(r.label),
-        T.query.setTo(r.query)
+        T.query.setTo(r.query),
+        T.userId.setTo(r.userId)
       )
     )
 
@@ -119,9 +120,12 @@ object RQueryBookmark {
     ).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
+  // 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, which is not covered by db indexes. This is now
+  // checked before (and therefore subject to race conditions, but is
+  // neglected here)
   def insertIfNotExists(
       account: AccountId,
       r: ConnectionIO[RQueryBookmark]