diff --git a/backend/src/app/rpc/commands/comments.clj b/backend/src/app/rpc/commands/comments.clj index 6c83247998..dcb34410e8 100644 --- a/backend/src/app/rpc/commands/comments.clj +++ b/backend/src/app/rpc/commands/comments.clj @@ -845,17 +845,69 @@ {::db/return-keys false}) nil)) +;; --- COMMAND: Mark all comment threads as read + +;; The dashboard sends the thread ids in batches of this same size +;; (frontend/src/app/main/data/comments.cljs), so the batch has to fit in +;; this limit. +(def ^:private max-mark-all-threads 500) + (def ^:private schema:mark-all-threads-as-read [:map {:title "mark-all-threads-as-read"} - [:threads [:vector ::sm/uuid]]]) + [:threads [:vector {:max max-mark-all-threads} ::sm/uuid]]]) + +(def ^:private sql:comment-threads-files + "SELECT ct.id, ct.file_id + FROM comment_thread ct + WHERE ct.id = ANY(?::uuid[])") + +(def ^:private sql:upsert-comment-thread-status-batch + "insert into comment_thread_status (thread_id, profile_id, modified_at) + select t.id, ?::uuid, ?::timestamptz + from unnest(?::uuid[]) as t(id) + on conflict (thread_id, profile_id) + do update set modified_at = excluded.modified_at;") + +(defn- get-comment-threads-files + [conn thread-ids] + (db/exec! conn [sql:comment-threads-files + (db/create-array conn "uuid" thread-ids)])) + +(defn- check-threads-access + "Raises a :not-found error when any of the threads does not exist or the + caller has no access to the file that owns it." + [cfg profile-id thread-ids] + (let [rows (get-comment-threads-files (::db/conn cfg) thread-ids) + found (into #{} (map :id) rows)] + (when-not (= (count thread-ids) (count found)) + (ex/raise :type :not-found + :code :object-not-found + :hint "not found")) + (doseq [file-id (distinct (map :file-id rows))] + (files/check-comment-permissions! cfg profile-id file-id nil)))) + +(defn- upsert-comment-thread-status-batch + [conn profile-id thread-ids] + (db/exec! conn [sql:upsert-comment-thread-status-batch + profile-id + (ct/in-future "1s") + (db/create-array conn "uuid" thread-ids)])) (sv/defmethod ::mark-all-threads-as-read + "Marks the given comment threads as read for the current profile. The + caller needs to have access to the file that owns every thread." {::doc/added "1.15" + ::doc/changes [["2.19" "The caller needs access to the file that owns every thread and the number of threads is limited to 500"]] ::sm/params schema:mark-all-threads-as-read} - [cfg {:keys [::rpc/profile-id threads] :as params}] - (db/tx-run! - cfg - (fn [{:keys [::db/conn]}] - (doseq [thread-id threads] - (upsert-comment-thread-status! conn profile-id thread-id))))) + [cfg {:keys [::rpc/profile-id threads]}] + (let [thread-ids (distinct threads)] + ;; Nothing to check and nothing to write: an empty list must not open a + ;; transaction nor run the queries with an empty array. + (when (seq thread-ids) + (db/tx-run! + cfg + (fn [{:keys [::db/conn] :as cfg}] + (check-threads-access cfg profile-id thread-ids) + (upsert-comment-thread-status-batch conn profile-id thread-ids) + nil))))) diff --git a/backend/test/backend_tests/rpc_comment_test.clj b/backend/test/backend_tests/rpc_comment_test.clj index 0d8f6b77e5..3447e5aad8 100644 --- a/backend/test/backend_tests/rpc_comment_test.clj +++ b/backend/test/backend_tests/rpc_comment_test.clj @@ -848,3 +848,213 @@ (get-in (meta (:result thread)) [:app.loggers.audit/props :team-id]))) (t/is (= (:id team) (get-in (meta (:result comment)) [:app.loggers.audit/props :team-id])))))) + +(t/deftest mark-all-threads-as-read-requires-access-to-the-threads + (let [owner (th/create-profile* 1 {:is-active true}) + stranger (th/create-profile* 2 {:is-active true}) + + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:team-id (:id team) + :profile-id (:id owner)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + page-id (get-in file [:data :pages 0]) + + thread (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id owner) + :file-id (:id file) + :page-id page-id + :position (gpt/point 0) + :content "private thread" + :frame-id uuid/zero}) + thread-id (get-in thread [:result :id])] + + (t/testing "the sibling rpc already rejects a profile without access" + (let [out (th/command! {::th/type :update-comment-thread-status + ::rpc/profile-id (:id stranger) + :id thread-id})] + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))))) + + (t/testing "mark-all-threads-as-read rejects a profile without access" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id stranger) + :threads [thread-id]})] + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out))))) + + (t/is (nil? (th/db-get :comment-thread-status + {:thread-id thread-id + :profile-id (:id stranger)})))))) + +(t/deftest mark-all-threads-as-read-behaviour + (let [owner (th/create-profile* 1 {:is-active true}) + member (th/create-profile* 2 {:is-active true}) + viewer (th/create-profile* 3 {:is-active true}) + outsider (th/create-profile* 4 {:is-active true}) + + team (th/create-team* 1 {:profile-id (:id owner)}) + _ (th/create-team-role* {:team-id (:id team) + :profile-id (:id member) + :role :editor}) + _ (th/create-team-role* {:team-id (:id team) + :profile-id (:id viewer) + :role :viewer}) + project (th/create-project* 1 {:team-id (:id team) + :profile-id (:id owner)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + page-id (get-in file [:data :pages 0]) + + thread (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id owner) + :file-id (:id file) + :page-id page-id + :position (gpt/point 0) + :content "hello" + :frame-id uuid/zero}) + thread-id (get-in thread [:result :id])] + + (t/testing "the thread is unread for a team member" + (let [out (th/command! {::th/type :get-unread-comment-threads + ::rpc/profile-id (:id member) + :team-id (:id team)})] + (t/is (th/success? out)) + (t/is (= 1 (count (:result out)))))) + + (t/testing "a team member can mark the threads as read" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id member) + :threads [thread-id]})] + (t/is (th/success? out))) + + (let [out (th/command! {::th/type :get-unread-comment-threads + ::rpc/profile-id (:id member) + :team-id (:id team)})] + (t/is (th/success? out)) + (t/is (= 0 (count (:result out)))))) + + (t/testing "a viewer of the team can mark the threads as read" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id viewer) + :threads [thread-id]})] + (t/is (th/success? out)))) + + (t/testing "an unknown thread is rejected" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id member) + :threads [(uuid/random)]})] + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))))) + + (t/testing "more threads than the allowed maximum are rejected" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id member) + :threads (vec (repeatedly 501 uuid/random))})] + (t/is (not (th/success? out))) + (t/is (= :validation (th/ex-type (:error out)))))) + + (t/testing "a list with an inaccessible thread writes nothing" + (let [other-team (th/create-team* 2 {:profile-id (:id owner)}) + other-project (th/create-project* 2 {:team-id (:id other-team) + :profile-id (:id owner)}) + other-file (th/create-file* 2 {:profile-id (:id owner) + :project-id (:id other-project)}) + other-thread (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id owner) + :file-id (:id other-file) + :page-id (get-in other-file [:data :pages 0]) + :position (gpt/point 0) + :content "other team" + :frame-id uuid/zero}) + other-thread-id (get-in other-thread [:result :id]) + + accessible-thread (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id owner) + :file-id (:id file) + :page-id page-id + :position (gpt/point 0) + :content "accessible" + :frame-id uuid/zero}) + accessible-thread-id (get-in accessible-thread [:result :id]) + + out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id member) + :threads [accessible-thread-id other-thread-id]})] + + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))) + (t/is (nil? (th/db-get :comment-thread-status + {:thread-id accessible-thread-id + :profile-id (:id member)}))) + (t/is (nil? (th/db-get :comment-thread-status + {:thread-id other-thread-id + :profile-id (:id member)}))))) + + (t/testing "a profile without any relation with the team is rejected" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id outsider) + :threads [thread-id]})] + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))))))) + +(t/deftest mark-all-threads-as-read-edge-cases + (let [owner (th/create-profile* 1 {:is-active true}) + member (th/create-profile* 2 {:is-active true}) + viewer (th/create-profile* 3 {:is-active true}) + + team (th/create-team* 1 {:profile-id (:id owner)}) + _ (th/create-team-role* {:team-id (:id team) + :profile-id (:id member) + :role :editor}) + _ (th/create-team-role* {:team-id (:id team) + :profile-id (:id viewer) + :role :viewer}) + project (th/create-project* 1 {:team-id (:id team) + :profile-id (:id owner)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + page-id (get-in file [:data :pages 0]) + + thread (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id owner) + :file-id (:id file) + :page-id page-id + :position (gpt/point 0) + :content "hello" + :frame-id uuid/zero}) + thread-id (get-in thread [:result :id])] + + (t/testing "repeated thread ids are accepted and written once" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id member) + :threads [thread-id thread-id]})] + (t/is (th/success? out)) + (t/is (= 1 (count (th/db-query :comment-thread-status + {:thread-id thread-id + :profile-id (:id member)})))))) + + (t/testing "an empty list of threads is accepted and writes nothing" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id viewer) + :threads []})] + (t/is (th/success? out)) + (t/is (= 0 (count (th/db-query :comment-thread-status + {:thread-id thread-id + :profile-id (:id viewer)})))))) + + (t/testing "marking a thread as read twice keeps a single status row" + (let [out (th/command! {::th/type :mark-all-threads-as-read + ::rpc/profile-id (:id member) + :threads [thread-id]})] + (t/is (th/success? out))) + + (t/is (= 1 (count (th/db-query :comment-thread-status + {:thread-id thread-id + :profile-id (:id member)})))) + + (let [out (th/command! {::th/type :get-unread-comment-threads + ::rpc/profile-id (:id member) + :team-id (:id team)})] + (t/is (th/success? out)) + (t/is (= 0 (count (:result out)))))))) diff --git a/frontend/src/app/main/data/comments.cljs b/frontend/src/app/main/data/comments.cljs index 3371dcd592..ff993be7d9 100644 --- a/frontend/src/app/main/data/comments.cljs +++ b/frontend/src/app/main/data/comments.cljs @@ -462,6 +462,18 @@ (rx/map #(partial fetched-comments %)) (rx/catch #(rx/throw {:type :comment-error}))))))) +;; Has to fit in the limit of the backend rpc +;; (backend/src/app/rpc/commands/comments.clj). +(def ^:private mark-all-threads-batch-size 500) + +(defn mark-all-threads-batches + "Splits the thread ids into the batches that the backend accepts on a + single call." + [thread-ids] + (->> thread-ids + (partition-all mark-all-threads-batch-size) + (mapv vec))) + (defn mark-all-threads-as-read "Mark all threads as read" [team-id] @@ -470,11 +482,23 @@ (-data [_] {}) ptk/WatchEvent (watch [_ state _] - (let [threads (-> state :comment-threads vals)] + (let [threads (-> state :comment-threads vals) + batches (mark-all-threads-batches (mapv :id threads)) + refresh-event (retrieve-unread-comment-threads team-id)] (rx/concat - (->> (rp/cmd! :mark-all-threads-as-read {:threads (mapv :id threads)}) - (rx/map #(retrieve-unread-comment-threads team-id)) - (rx/catch #(rx/throw {:type :comment-error}))) + (->> (rx/from batches) + (rx/mapcat (fn [batch] + (rp/cmd! :mark-all-threads-as-read {:threads batch}))) + (rx/reduce (fn [_ _] nil) nil) + (rx/map (fn [_] refresh-event)) + ;; Each batch commits on the server on its own, so a failure in + ;; a later batch leaves the previous ones already marked as + ;; read. The unread list is re-read anyway and the error is + ;; propagated afterwards, so the success toast below stays + ;; limited to a full success. + (rx/catch (fn [_] + (rx/concat (rx/of refresh-event) + (rx/throw {:type :comment-error}))))) (rx/of (ntf/show {:level :info :type :toast :content (tr "dashboard.mark-all-as-read.success") diff --git a/frontend/test/frontend_tests/data/comments_batches_test.cljs b/frontend/test/frontend_tests/data/comments_batches_test.cljs new file mode 100644 index 0000000000..7bab3fa964 --- /dev/null +++ b/frontend/test/frontend_tests/data/comments_batches_test.cljs @@ -0,0 +1,95 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. + +(ns frontend-tests.data.comments-batches-test + (:require + [app.common.uuid :as uuid] + [app.main.data.comments :as dcmt] + [app.main.data.notifications :as ntf] + [app.main.repo :as rp] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] + [frontend-tests.helpers.mock :as mock] + [potok.v2.core :as ptk])) + +(defn- comment-threads-state + [n] + {:comment-threads (into {} (map (fn [_] [(uuid/next) {:id (uuid/next)}]) (range n)))}) + +(t/deftest mark-all-threads-batches-splits-the-ids + (t/is (= [] (dcmt/mark-all-threads-batches []))) + (t/is (= [[1 2 3]] (dcmt/mark-all-threads-batches [1 2 3]))) + (t/is (= [500 1] (mapv count (dcmt/mark-all-threads-batches (range 501)))))) + +(t/deftest ^:async mark-all-threads-as-read-sends-the-ids-in-batches + (let [calls (atom []) + state (comment-threads-state 1200) + event (dcmt/mark-all-threads-as-read (uuid/next))] + + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [cmd params] + (swap! calls conj [cmd params]) + (rx/of nil)))} + (await (async/observe (ptk/watch event state nil))))) + + (t/is (= 3 (count @calls))) + (t/is (every? #(= :mark-all-threads-as-read (first %)) @calls)) + (t/is (= [500 500 200] (mapv (comp count :threads second) @calls))))) + +(t/deftest ^:async mark-all-threads-as-read-sends-a-single-batch + (let [calls (atom []) + state (comment-threads-state 10) + event (dcmt/mark-all-threads-as-read (uuid/next))] + + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [cmd params] + (swap! calls conj [cmd params]) + (rx/of nil)))} + (await (async/observe (ptk/watch event state nil))))) + + (t/is (= 1 (count @calls))) + (t/is (= 10 (count (:threads (second (first @calls)))))))) + +(t/deftest ^:async mark-all-threads-as-read-refreshes-the-unread-list-after-a-batch-fails + (let [calls (atom []) + state (comment-threads-state 1200) + event (dcmt/mark-all-threads-as-read (uuid/next)) + seen (atom []) + errors (atom [])] + + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [cmd params] + (swap! calls conj [cmd params]) + (if (= 2 (count @calls)) + (rx/throw (ex-info "boom" {})) + (rx/of nil))))} + (await (async/observe (ptk/watch event state nil) + :on-next #(swap! seen conj (ptk/type %)) + :on-error #(swap! errors conj %))))) + + (t/is (= 2 (count @calls)) + "the pipeline stops at the batch that fails") + (t/is (= [::dcmt/retrieve-unread-comment-threads] @seen) + "the unread list is re-read and no success toast is emitted") + (t/is (= [{:type :comment-error}] @errors)))) + +(t/deftest ^:async mark-all-threads-as-read-refreshes-and-shows-the-success-toast + (let [state (comment-threads-state 600) + event (dcmt/mark-all-threads-as-read (uuid/next)) + seen (atom [])] + + (await + (mock/with-mocks* + {rp/cmd! (mock/stub (fn [_ _] (rx/of nil)))} + (await (async/observe (ptk/watch event state nil) + :on-next #(swap! seen conj (ptk/type %)))))) + + (t/is (= [::dcmt/retrieve-unread-comment-threads ::ntf/show] @seen)))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 3f25052df3..33f4b6f4f8 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -10,6 +10,7 @@ [frontend-tests.composable-tests.comp.sync-test] [frontend-tests.copy-as-svg-test] [frontend-tests.data.auth-test] + [frontend-tests.data.comments-batches-test] [frontend-tests.data.comments-filters-test] [frontend-tests.data.dashboard-test] [frontend-tests.data.event-test] @@ -165,6 +166,7 @@ 'frontend-tests.composable-tests.comp.sync-test 'frontend-tests.copy-as-svg-test 'frontend-tests.data.auth-test + 'frontend-tests.data.comments-batches-test 'frontend-tests.data.comments-filters-test 'frontend-tests.data.dashboard-test 'frontend-tests.data.event-test