From c1dec5d4f0233afedef1f01fed4e0248f5638db1 Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Fri, 23 Oct 2026 10:32:21 +0200 Subject: [PATCH 1/2] :sparkles: Add merge reviews and tidy up the branching workspace --- .../app/email/review-request/en.html | 2 +- .../resources/app/email/review-request/en.txt | 2 +- backend/src/app/migrations.clj | 5 +- .../sql/0806-add-file-branch-merge-table.sql | 33 ++ backend/src/app/rpc/commands/files_branch.clj | 120 ++++- .../rpc_file_branch_oplog_test.clj | 81 +-- .../backend_tests/rpc_file_branch_test.clj | 120 +++-- .../rpc_file_pull_request_test.clj | 84 +-- .../images/icons/git-branch-minus.svg | 6 + .../images/icons/git-compare-arrows.svg | 8 + frontend/resources/images/icons/git-graph.svg | 8 + .../images/icons/git-merge-conflict.svg | 7 + .../icons/git-pull-request-create-arrow.svg | 8 + .../src/app/main/data/workspace/branches.cljs | 97 ++-- .../main/ui/ds/foundations/assets/icon.cljs | 5 + .../app/main/ui/ds/product/branch_row.cljs | 42 +- .../src/app/main/ui/ds/product/branch_row.mdx | 4 + .../main/ui/workspace/branch_switcher.cljs | 56 +- .../main/ui/workspace/branch_switcher.scss | 15 +- .../src/app/main/ui/workspace/main_menu.cljs | 4 +- .../main/ui/workspace/sidebar/branches.cljs | 486 ++++++++++++------ .../main/ui/workspace/sidebar/branches.scss | 10 +- .../ui/workspace/sidebar/pull_requests.cljs | 6 +- .../main/ui/workspace/sidebar/reviews.cljs | 405 +++++++++++++++ .../main/ui/workspace/sidebar/reviews.scss | 240 +++++++++ .../data/workspace_branches_test.cljs | 71 ++- frontend/translations/en.po | 131 +++-- 27 files changed, 1555 insertions(+), 501 deletions(-) create mode 100644 backend/src/app/migrations/sql/0806-add-file-branch-merge-table.sql create mode 100644 frontend/resources/images/icons/git-branch-minus.svg create mode 100644 frontend/resources/images/icons/git-compare-arrows.svg create mode 100644 frontend/resources/images/icons/git-graph.svg create mode 100644 frontend/resources/images/icons/git-merge-conflict.svg create mode 100644 frontend/resources/images/icons/git-pull-request-create-arrow.svg create mode 100644 frontend/src/app/main/ui/workspace/sidebar/reviews.cljs create mode 100644 frontend/src/app/main/ui/workspace/sidebar/reviews.scss diff --git a/backend/resources/app/email/review-request/en.html b/backend/resources/app/email/review-request/en.html index a9f1651d5a..1301724caa 100644 --- a/backend/resources/app/email/review-request/en.html +++ b/backend/resources/app/email/review-request/en.html @@ -186,7 +186,7 @@
- {{ source-user }} has requested your review on the pull request "{{ pull-request-title }}" ({{ pull-request-route }}).
+ {{ source-user }} has requested your review on the design review "{{ pull-request-title }}" ({{ pull-request-route }}). diff --git a/backend/resources/app/email/review-request/en.txt b/backend/resources/app/email/review-request/en.txt index 84e12bb126..fb29439fc3 100644 --- a/backend/resources/app/email/review-request/en.txt +++ b/backend/resources/app/email/review-request/en.txt @@ -1,6 +1,6 @@ Hello {{name|abbreviate:25}}! -{{ source-user }} has requested your review on the pull request "{{ pull-request-title }}" ({{ pull-request-route }}). +{{ source-user }} has requested your review on the design review "{{ pull-request-title }}" ({{ pull-request-route }}). -- diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index 4ffc31dadf..ddb2502c23 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -542,7 +542,10 @@ :fn (mg/resource "app/migrations/sql/0804-add-file-branch-change-changes-count.sql")} {:name "0805-stamp-file-branch-change-changes-count" - :fn mg0805/migrate}]) + :fn mg0805/migrate} + + {:name "0806-add-file-branch-merge-table" + :fn (mg/resource "app/migrations/sql/0806-add-file-branch-merge-table.sql")}]) (defn apply-migrations! [pool name migrations] diff --git a/backend/src/app/migrations/sql/0806-add-file-branch-merge-table.sql b/backend/src/app/migrations/sql/0806-add-file-branch-merge-table.sql new file mode 100644 index 0000000000..63879fcdf4 --- /dev/null +++ b/backend/src/app/migrations/sql/0806-add-file-branch-merge-table.sql @@ -0,0 +1,33 @@ +-- The record of every merge of a branch into its source file (main): who +-- merged it, when, the comment they left and, when the merge accepted a +-- design review, that review. A merge deletes the branch, and the objects +-- GC later deletes its file, which takes the `file_branch` and +-- `file_pull_request` rows with it through their cascades. So this table +-- keeps its own copy of the names it shows and holds no foreign key to +-- either table: the record outlives both. It goes away with main. + +CREATE TABLE file_branch_merge ( + id uuid PRIMARY KEY, + + created_at timestamptz NOT NULL DEFAULT clock_timestamp(), + + -- main: the file the branch was merged into + file_id uuid NOT NULL REFERENCES file(id) ON DELETE CASCADE DEFERRABLE, + + -- the merged branch, by id and by the name it had when it was merged + file_branch_id uuid NOT NULL, + branch_name text NOT NULL, + + merged_by uuid NULL REFERENCES profile(id) ON DELETE SET NULL DEFERRABLE, + comment text NULL, + + -- the design review the merge accepted, if any + pull_request_id uuid NULL, + pull_request_title text NULL +); + +CREATE INDEX file_branch_merge__file_id__created_at__idx + ON file_branch_merge(file_id, created_at DESC); + +CREATE INDEX file_branch_merge__merged_by__idx + ON file_branch_merge(merged_by); diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index 5adcfa7e4b..8dfd51f4c2 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -403,6 +403,36 @@ :profile-id profile-id :session-id session-id}})) +;; --- Helpers: merge record + +(def ^:private sql:get-open-branch-pull-request + "SELECT id, title + FROM file_pull_request + WHERE file_branch_id = ? + AND status = 'open' + AND deleted_at IS NULL") + +(defn- record-merge! + "Insert the record of a merge of `branch` into its source file (see the + `file_branch_merge` migration). The open pull request of the branch is + the design review the merge accepts, so it is read here, before the + merge closes it. A blank comment is stored as no comment. Must run + inside the merge transaction." + [conn {:keys [id source-file-id name]} {:keys [profile-id comment created-at]}] + (let [pr (db/exec-one! conn [sql:get-open-branch-pull-request id]) + comment (when-not (str/blank? comment) (str/trim comment))] + (db/insert! conn :file-branch-merge + {:id (uuid/next) + :created-at created-at + :file-id source-file-id + :file-branch-id id + :branch-name name + :merged-by profile-id + :comment comment + :pull-request-id (:id pr) + :pull-request-title (:title pr)} + {::db/return-keys false}))) + ;; --- COMMAND: create-file-branch (defn- check-branch-name! @@ -593,6 +623,7 @@ JOIN file AS sf ON (sf.id = fb.source_file_id) WHERE fb.source_file_id = ? AND fb.deleted_at IS NULL + AND fb.status != 'merged' AND (?::boolean OR fb.status = 'open') ORDER BY fb.created_at DESC") @@ -958,11 +989,9 @@ ;; deselected, which the merge resolves to the target side [:excluded {:optional true} [::sm/set :string]] [:expected-main-revn {:optional true} ::sm/int] - ;; when false (the default) the branch and its file are logically - ;; deleted right after a successful merge, in the SAME transaction (so - ;; merged copies do not pile up); when true the branch is kept as - ;; "merged", staying readable with status merged until it is deleted. - [:keep-branch {:optional true} ::sm/boolean]]) + ;; what the user says about the merge; kept in its record + ;; (`file_branch_merge`), shown in the list of merges of main + [:comment {:optional true} [:string {:max 4000}]]]) (sv/defmethod ::merge-file-branch "Merge a branch into its source file (main). @@ -974,15 +1003,16 @@ kinds the engine cannot translate (`bm/unsupported-kinds`) make it return `{:status :unsupported}` so no change is silently dropped. Otherwise it takes a safety snapshot of main, applies the merge to main through the - production change pipeline, marks the branch merged and notifies open - clients via msgbus." + production change pipeline, records the merge with its `comment` + (`file_branch_merge`), deletes the branch and its file and notifies + open clients via msgbus." {::doc/added "2.16" ::webhooks/event? true ::sm/params schema:merge-file-branch ::climit/id [[:merge-file-branch/by-profile ::rpc/profile-id] [:merge-file-branch/global]]} [{:keys [::mbus/msgbus] :as cfg} - {:keys [::rpc/profile-id ::rpc/session-id branch-id resolutions expected-main-revn keep-branch excluded]}] + {:keys [::rpc/profile-id ::rpc/session-id branch-id resolutions expected-main-revn comment excluded]}] (bp/with-policies (check-branching-enabled!) (let [branch (db/get* cfg :file-branch {:id branch-id})] @@ -1011,7 +1041,7 @@ ;; advisory lock the normal update-file path takes; stable order ;; to avoid deadlocks): without the branch lock, edits saved to ;; the branch while the merge runs would be silently lost when - ;; the branch is marked merged (and possibly deleted) below. + ;; the branch is marked merged and deleted below. (run! (partial db/xact-lock! conn) (sort [main-id branch-file-id])) ;; the row read before the lock is stale by now: a competing @@ -1073,15 +1103,16 @@ finish-branch! (fn [ts] - ;; mark merged. The pinned base snapshot stays for a - ;; KEPT branch (its state is derived from that - ;; snapshot and the op log, so releasing the pin - ;; would leave it unreadable once the snapshot GC - ;; runs); delete-branch! releases it on the delete - ;; path. Unless the user chose to keep it the branch - ;; and its file are deleted in this same transaction + ;; record the merge, mark the branch merged and + ;; delete it and its file in this same transaction ;; (no client-driven second call, no window where a - ;; crash leaves a stale copy). + ;; crash leaves a stale copy); delete-branch! + ;; releases the pinned base snapshot. The record is + ;; written first: it reads the open pull request the + ;; lines below close. + (record-merge! conn branch {:profile-id profile-id + :comment comment + :created-at ts}) (db/update! conn :file-branch {:status "merged" :merged-at ts @@ -1097,10 +1128,9 @@ {:profile-id profile-id :status "merged" :deleted-at (ct/in-future delay)}) - (when-not keep-branch - (vswap! messages conj - (delete-branch! cfg branch {:profile-id profile-id - :session-id session-id})))))] + (vswap! messages conj + (delete-branch! cfg branch {:profile-id profile-id + :session-id session-id}))))] (cond (seq unresolved) (audited {:status :conflicts :conflicts conflicts :main-revn (:revn main-file)} @@ -1784,6 +1814,7 @@ "SELECT count(*) AS count FROM file_branch AS fb WHERE fb.deleted_at IS NULL + AND fb.status != 'merged' AND fb.source_file_id = coalesce((SELECT b.source_file_id FROM file_branch AS b WHERE b.branch_file_id = ? @@ -1791,8 +1822,8 @@ ?)") (sv/defmethod ::get-file-branch-count - "How many branches the file has, archived and merged ones included, as - the listing with `:include-archived` returns them. When the file is + "How many branches the file has, archived ones included, as the + listing with `:include-archived` returns them. When the file is itself a branch, the branches of its source (main) are counted. The workspace header asks this on every file open to decide whether to @@ -1810,6 +1841,49 @@ (select-keys [:count])))) {:count 0})) +;; --- COMMAND QUERY: get-file-branch-merges + +(def ^:private schema:get-file-branch-merges + [:map {:title "get-file-branch-merges"} + [:file-id ::sm/uuid]]) + +(def ^:private sql:resolve-source-file + "SELECT coalesce((SELECT b.source_file_id + FROM file_branch AS b + WHERE b.branch_file_id = ? + AND b.deleted_at IS NULL), + ?) AS id") + +(def ^:private sql:get-file-branch-merges + "SELECT fbm.id, + fbm.created_at, + fbm.file_id, + fbm.file_branch_id, + fbm.branch_name, + fbm.merged_by, + fbm.comment, + fbm.pull_request_id, + fbm.pull_request_title + FROM file_branch_merge AS fbm + WHERE fbm.file_id = ? + ORDER BY fbm.created_at DESC") + +(sv/defmethod ::get-file-branch-merges + "List the merges into a file, newest first: each one names the merged + branch, who merged it and when, the comment left with it and the design + review it accepted, if any. When the file is itself a branch, the + merges into its source (main) are listed. The records outlive the + branches they name (see the `file_branch_merge` migration)." + {::doc/added "2.16" + ::sm/params schema:get-file-branch-merges} + [cfg {:keys [::rpc/profile-id file-id]}] + (check-branching-enabled!) + (db/run! cfg + (fn [{:keys [::db/conn] :as cfg}] + (let [main-id (:id (db/exec-one! conn [sql:resolve-source-file file-id file-id]))] + (files/check-read-permissions! cfg profile-id main-id) + (db/exec! conn [sql:get-file-branch-merges main-id]))))) + ;; --- COMMAND: update-file-branch (rename / description) (def ^:private schema:update-file-branch diff --git a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj index ae4b6d3a3a..b469c63862 100644 --- a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj @@ -15,7 +15,6 @@ [app.common.data :as d] [app.common.features :as cfeat] [app.common.files.migrations :as fmg] - [app.common.time :as ct] [app.common.types.shape :as cts] [app.common.uuid :as uuid] [app.config :as cf] @@ -172,70 +171,6 @@ mf (th/db-get :file {:id (:id file)})] (t/is (= (:revn mf) (:base-revn row)))))))) -(t/deftest merge-with-keep-branch-leaves-the-branch-readable - (with-redefs [cf/flags (conj cf/flags :branching)] - (let [profile (th/create-profile* 1 {:is-active true}) - proj-id (:default-project-id profile) - file (th/create-file* 1 {:profile-id (:id profile) - :project-id proj-id - :is-shared false}) - create (create-branch* profile (:id file) "oplog-keep") - branch-file-id (:branch-file-id create) - branch-id (:id create) - color-id (uuid/random)] - - (apply-change* profile branch-file-id - {:type :add-color - :color {:id color-id :name "C" :color "#112233" :opacity 1}}) - - (let [out (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] - (t/is (nil? (:error out))) - (t/is (= :merged (-> out :result :status)))) - - (t/testing "the kept branch still derives its state" - (let [out (th/command! {::th/type :get-file - ::rpc/profile-id (:id profile) - :id branch-file-id})] - (t/is (nil? (:error out))) - (t/is (contains? (-> out :result :data :colors) color-id)))) - - (t/testing "the base pin survives while the branch is kept" - (let [rows (->> (th/db-query :file-change {:file-id (:id file)}) - (filter #(some-> (:label %) (str/starts-with? "branch-base/"))))] - (t/is (seq rows)) - (t/is (every? (fn [{:keys [deleted-at]}] - (or (nil? deleted-at) - (ct/is-after? deleted-at (ct/in-future {:days 400})))) - rows))))))) - -(t/deftest a-kept-merged-branch-refuses-saves - (with-redefs [cf/flags (conj cf/flags :branching)] - (let [profile (th/create-profile* 1 {:is-active true}) - proj-id (:default-project-id profile) - file (th/create-file* 1 {:profile-id (:id profile) - :project-id proj-id - :is-shared false}) - create (create-branch* profile (:id file) "oplog-merged") - branch-file-id (:branch-file-id create) - branch-id (:id create)] - - (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true}) - - (let [out (apply-change* profile branch-file-id - {:type :add-color - :color {:id (uuid/random) :name "C" :color "#112233" :opacity 1}})] - (t/is (th/ex-of-type? (:error out) :validation)) - (t/is (th/ex-of-code? (:error out) :branch-merged))) - - (t/testing "nothing reaches the op log" - (t/is (empty? (oplog-rows branch-file-id))))))) - (defn- add-color* [profile file-id] (apply-change* profile file-id {:type :add-color @@ -1408,8 +1343,7 @@ ;; a media row MAIN owns (let [merge (-> (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true}) + :branch-id branch-id}) :result)] (t/is (= :merged (:status merge))) (let [ref (-> (derived-obj profile main-id page-id shape-id) @@ -1462,14 +1396,13 @@ ;; drops the row (it filters by branch ownership) and main ends up ;; referencing media it does not own. (with-redefs [cf/flags (conj cf/flags :branching)] - (let [{:keys [profile main-id branch-id branch-file-id page-id shape-id]} + (let [{:keys [profile main-id branch-id page-id shape-id]} (foreign-media-setup) out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (nil? (:error out))) (t/is (= :merged (-> out :result :status))) @@ -1477,10 +1410,4 @@ (let [ref (-> (derived-obj profile main-id page-id shape-id) media-ref) row (th/db-get :file-media-object {:id ref})] (t/is (some? row)) - (t/is (= main-id (:file-id row)))) - - ;; the branch keeps deriving its own state from the kept op log - (let [ref (-> (derived-obj profile branch-file-id page-id shape-id) media-ref) - row (th/db-get :file-media-object {:id ref})] - (t/is (some? row)) - (t/is (= branch-file-id (:file-id row))))))) + (t/is (= main-id (:file-id row))))))) diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index bc01053a30..fed5b53d70 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -797,31 +797,69 @@ (ct/is-before? deleted-at (ct/in-future {:days 400})))) rows)))))) -(t/deftest merge-with-keep-branch-archives-it +(t/deftest merge-records-itself-and-deletes-the-branch (with-redefs [cf/flags (conj cf/flags :branching)] (let [profile (th/create-profile* 1 {:is-active true}) proj-id (:default-project-id profile) file (th/create-file* 1 {:profile-id (:id profile) :project-id proj-id}) - create (:result (th/command! {::th/type :create-file-branch + create! (fn [name] + (:result (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name name}))) + merge! (fn [branch-id params] + (th/command! (merge {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :file-id (:id file) - :name "kept"})) - branch-id (:id create) - branch-file-id (:branch-file-id create)] + :branch-id branch-id} + params))) + first-b (create! "first") + second-b (create! "second")] - (let [out (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + (let [out (merge! (:id first-b) {:comment " Ships the new header "})] (t/is (nil? (:error out))) (t/is (= :merged (-> out :result :status)))) - (let [[row] (th/db-query :file-branch {:id branch-id}) - bf (th/db-get :file {:id branch-file-id})] - (t/is (= "merged" (:status row))) - (t/is (nil? (:deleted-at row))) - (t/is (nil? (:deleted-at bf))))))) + (t/testing "the merged branch and its file are deleted" + (let [[row] (th/db-query :file-branch {:id (:id first-b)}) + bf (th/db-get :file {:id (:branch-file-id first-b)} {::db/remove-deleted false})] + (t/is (= "merged" (:status row))) + (t/is (some? (:deleted-at row))) + (t/is (some? (:deleted-at bf))))) + + (let [out (merge! (:id second-b) {:comment " "})] + (t/is (nil? (:error out)))) + + (t/testing "the merges are listed newest first, from main or a branch" + (let [out (th/command! {::th/type :get-file-branch-merges + ::rpc/profile-id (:id profile) + :file-id (:id file)}) + merges (:result out)] + (t/is (nil? (:error out))) + (t/is (= ["second" "first"] (mapv :branch-name merges))) + (t/is (every? #(= (:id profile) (:merged-by %)) merges)) + (t/is (every? #(= (:id file) (:file-id %)) merges)) + (t/is (= [nil "Ships the new header"] (mapv :comment merges)) + "the comment is trimmed, and a blank one is no comment") + (t/is (every? (comp nil? :pull-request-id) merges)))) + + (t/testing "a merged branch leaves the branch listing" + (let [out (th/command! {::th/type :get-file-branches + ::rpc/profile-id (:id profile) + :file-id (:id file) + :include-archived true})] + (t/is (empty? (:result out)))))))) + +(t/deftest the-merges-of-a-file-need-read-permissions + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [profile (th/create-profile* 1 {:is-active true}) + other (th/create-profile* 2 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + out (th/command! {::th/type :get-file-branch-merges + ::rpc/profile-id (:id other) + :file-id (:id file)})] + (t/is (th/ex-of-type? (:error out) :not-found))))) (t/deftest merge-requires-main-edition-permissions (with-redefs [cf/flags (conj cf/flags :branching)] @@ -1185,8 +1223,7 @@ (t/testing "merging brings the new media with a MAIN-owned row" (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (nil? (:error out))) (t/is (= :merged (-> out :result :status)))) @@ -1243,8 +1280,7 @@ (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (nil? (:error out))) (t/is (= :merged (-> out :result :status)))) @@ -1860,9 +1896,9 @@ (t/is (= "merged" (:status row))) (t/is (some? (:deleted-at row)))))))))) -(t/deftest delete-keeps-the-status-the-branch-carries - ;; deleting a kept merged branch must not rewrite its status: the - ;; record is the only place left that says the branch was merged +(t/deftest a-merged-branch-is-gone-for-delete + ;; a merge deletes its branch: deleting it again finds nothing and does + ;; not rewrite the status that says the branch was merged (with-redefs [cf/flags (conj cf/flags :branching)] (let [profile (th/create-profile* 1 {:is-active true}) proj-id (:default-project-id profile) @@ -1871,7 +1907,7 @@ create (:result (th/command! {::th/type :create-file-branch ::rpc/profile-id (:id profile) :file-id (:id file) - :name "kept"})) + :name "merged"})) branch-id (:id create) branch-file-id (:branch-file-id create) cid (uuid/random)] @@ -1879,14 +1915,12 @@ {:type :add-color :color {:id cid :name "C" :color "#112233" :opacity 1}}) (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (= :merged (-> out :result :status)))) (let [out (th/command! {::th/type :delete-file-branch ::rpc/profile-id (:id profile) :id branch-id})] - (t/is (nil? (:error out))) - (t/is (= :deleted (-> out :result :status)))) + (t/is (= :branch-not-found (-> out :error ex-data :code)))) (let [[row] (th/db-query :file-branch {:id branch-id})] (t/is (= "merged" (:status row))) (t/is (some? (:deleted-at row))))))) @@ -2298,8 +2332,8 @@ (t/deftest merge-refuses-a-branch-merged-under-the-lock ;; two merges of one open branch: the winner commits before the ;; outer merge takes any lock. The outer merge must refuse instead of - ;; trusting its stale "open" row and deleting the branch the winner - ;; kept. + ;; trusting its stale "open" row: merging the branch a second time, + ;; recording a second merge and deleting it again. (with-redefs [cf/flags (conj cf/flags :branching)] (let [profile (th/create-profile* 1 {:is-active true}) proj-id (:default-project-id profile) @@ -2321,26 +2355,26 @@ db/xact-lock! (fn [conn id] ;; the winning merge runs to completion BEFORE - ;; the outer one takes any lock, and keeps the - ;; branch so the outer merge still has its file + ;; the outer one takes any lock (when (compare-and-set! raced? false true) (reset! merged (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true}))) + :branch-id branch-id}))) (orig-lock conn id))] (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) :branch-id branch-id})] - (t/is (= :branch-not-open (-> out :error ex-data :code))))) + (t/is (= :branch-not-found (-> out :error ex-data :code))))) (t/is (= :merged (-> @merged :result :status))) (t/testing "main holds the first merge's revn only" (t/is (= (-> @merged :result :revn) (:revn (first (th/db-query :file {:id (:id file)})))))) - (t/testing "the branch the winning merge kept is still kept" + (t/testing "the winning merge is the only one recorded" + (t/is (= 1 (count (th/db-query :file-branch-merge {:file-branch-id branch-id}))))) + (t/testing "the branch is merged and deleted once" (let [[row] (th/db-query :file-branch {:id branch-id})] (t/is (= "merged" (:status row))) - (t/is (nil? (:deleted-at row))))))))) + (t/is (some? (:deleted-at row))))))))) (t/deftest merge-releases-the-base-the-lock-shows ;; the merge releases the base snapshot pin (on the delete path), and @@ -2514,15 +2548,17 @@ :expected-main-revn 9999})] (t/is (= :file-modified (-> out :error ex-data :code))) (t/is (empty? @seen))) - ;; the kept branch means :file-merged is the only message + ;; the merge deletes the branch: its file's clients hear of the + ;; deletion and main's of the merge (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (= :merged (-> out :result :status)))) - (let [[{:keys [message main-revn]}] @seen] - (t/is (= 1 (count @seen))) - (t/is (= :file-merged (:type message))) + (let [by-type (into {} (map (juxt (comp :type :message) identity)) @seen) + {:keys [message main-revn]} (get by-type :file-merged)] + (t/is (= 2 (count @seen))) + (t/is (= #{:file-deleted :file-merged} (set (keys by-type)))) + (t/is (= branch-file-id (get-in by-type [:file-deleted :message :file-id]))) ;; the merge was already committed when the message left (t/is (= (:revn message) main-revn))))))) diff --git a/backend/test/backend_tests/rpc_file_pull_request_test.clj b/backend/test/backend_tests/rpc_file_pull_request_test.clj index fc082eee7d..496695650f 100644 --- a/backend/test/backend_tests/rpc_file_pull_request_test.clj +++ b/backend/test/backend_tests/rpc_file_pull_request_test.clj @@ -492,7 +492,14 @@ (t/is (= "merged" (:status row))) (t/is (some? (:closed-at row))) ;; the row survives as history even though the branch is gone - (t/is (nil? (:deleted-at row)))))) + (t/is (nil? (:deleted-at row)))) + ;; the record of the merge names the review it accepted, so the + ;; list of merges keeps it once the review is gone + (let [merges (:result (th/command! {::th/type :get-file-branch-merges + ::rpc/profile-id (:id author) + :file-id (:id file)}))] + (t/is (= [(:id pr)] (mapv :pull-request-id merges))) + (t/is (= [(:title pr)] (mapv :pull-request-title merges)))))) (t/testing "deleting the branch closes the pull request" (let [branch (create-branch* author file "delete-me") @@ -585,81 +592,6 @@ (t/is (= :branch-not-open (-> out :error ex-data :code))) (t/is (= "closed" (:status (first (th/db-query :file-pull-request {:id (:id pr)})))))))))))) -(t/deftest create-serializes-against-a-kept-concurrent-merge - ;; same race, but the merge keeps the branch: its row stays readable - ;; with status "merged" and its file is not deleted, so without the - ;; re-read the create sails past the stale status check and INSERTS a - ;; pull request for a branch the merge already closed. - (with-redefs [cf/flags pr-flags] - (let [author (th/create-profile* 1 {:is-active true}) - file (th/create-file* 1 {:profile-id (:id author) - :project-id (:default-project-id author)}) - branch (create-branch* author file "contested") - branch-id (:id branch) - branch-file-id (:branch-file-id branch)] - (add-color* author branch-file-id "Brand") - (let [orig-lock db/xact-lock! - merging? (atom false) - merged (atom nil)] - (with-redefs [db/xact-lock! - (fn [conn id] - ;; the first lock the create takes is the signal: - ;; merge the same branch through the pool in its - ;; own transaction BEFORE the create locks - (when (compare-and-set! merging? false true) - (reset! merged (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id author) - :branch-id branch-id - :keep-branch true}))) - (orig-lock conn id))] - (let [out (th/command! {::th/type :create-pull-request - ::rpc/profile-id (:id author) - :branch-id branch-id - :title "review me"})] - (t/is (= :merged (-> @merged :result :status))) - (t/is (= "merged" (:status (first (th/db-query :file-branch {:id branch-id}))))) - (t/is (= :branch-not-open (-> out :error ex-data :code))) - (t/is (= [] (th/db-query :file-pull-request {:file-branch-id branch-id}))))))))) - -(t/deftest reopen-serializes-against-a-kept-concurrent-merge - ;; same race for the reopen: the kept merge leaves the branch row - ;; readable with status "merged", so without the re-read the reopen - ;; sails past the stale status check and REOPENS the closed pull - ;; request over a branch the merge already closed. - (with-redefs [cf/flags pr-flags] - (let [author (th/create-profile* 1 {:is-active true}) - file (th/create-file* 1 {:profile-id (:id author) - :project-id (:default-project-id author)}) - branch (create-branch* author file "contested") - branch-id (:id branch) - branch-file-id (:branch-file-id branch)] - (add-color* author branch-file-id "Brand") - (let [pr (:result (create-pr* author branch-id {}))] - (t/is (nil? (:error (th/command! {::th/type :close-pull-request - ::rpc/profile-id (:id author) - :id (:id pr)})))) - (let [orig-lock db/xact-lock! - merging? (atom false) - merged (atom nil)] - (with-redefs [db/xact-lock! - (fn [conn id] - ;; the first lock the reopen takes is the signal: - ;; merge the same branch through the pool in its - ;; own transaction BEFORE the reopen locks - (when (compare-and-set! merging? false true) - (reset! merged (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id author) - :branch-id branch-id - :keep-branch true}))) - (orig-lock conn id))] - (let [out (th/command! {::th/type :reopen-pull-request - ::rpc/profile-id (:id author) - :id (:id pr)})] - (t/is (= :merged (-> @merged :result :status))) - (t/is (= "merged" (:status (first (th/db-query :file-branch {:id branch-id}))))) - (t/is (= :branch-not-open (-> out :error ex-data :code))) - (t/is (= "closed" (:status (first (th/db-query :file-pull-request {:id (:id pr)})))))))))))) - (t/deftest closed-pull-requests-listing (with-redefs [cf/flags pr-flags] (let [author (th/create-profile* 1 {:is-active true}) diff --git a/frontend/resources/images/icons/git-branch-minus.svg b/frontend/resources/images/icons/git-branch-minus.svg new file mode 100644 index 0000000000..e1d7392190 --- /dev/null +++ b/frontend/resources/images/icons/git-branch-minus.svg @@ -0,0 +1,6 @@ + + + + + + diff --git a/frontend/resources/images/icons/git-compare-arrows.svg b/frontend/resources/images/icons/git-compare-arrows.svg new file mode 100644 index 0000000000..4bb584c4ec --- /dev/null +++ b/frontend/resources/images/icons/git-compare-arrows.svg @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/frontend/resources/images/icons/git-graph.svg b/frontend/resources/images/icons/git-graph.svg new file mode 100644 index 0000000000..715b79c033 --- /dev/null +++ b/frontend/resources/images/icons/git-graph.svg @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/frontend/resources/images/icons/git-merge-conflict.svg b/frontend/resources/images/icons/git-merge-conflict.svg new file mode 100644 index 0000000000..a0ecbf4632 --- /dev/null +++ b/frontend/resources/images/icons/git-merge-conflict.svg @@ -0,0 +1,7 @@ + + + + + + + diff --git a/frontend/resources/images/icons/git-pull-request-create-arrow.svg b/frontend/resources/images/icons/git-pull-request-create-arrow.svg new file mode 100644 index 0000000000..472b28b162 --- /dev/null +++ b/frontend/resources/images/icons/git-pull-request-create-arrow.svg @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/frontend/src/app/main/data/workspace/branches.cljs b/frontend/src/app/main/data/workspace/branches.cljs index be5396dc6f..ee5c27ad60 100644 --- a/frontend/src/app/main/data/workspace/branches.cljs +++ b/frontend/src/app/main/data/workspace/branches.cljs @@ -18,6 +18,7 @@ [app.util.dom :as dom] [app.util.i18n :refer [tr]] [beicon.v2.core :as rx] + [cuerdas.core :as str] [potok.v2.core :as ptk])) (defn- reload-file-window @@ -137,6 +138,33 @@ (rx/catch (fn [_] (rx/of (update-branches-state {:status :error}))))))))) +(defn- set-branch-merges + [merges-state] + (ptk/reify ::set-branch-merges + ptk/UpdateEvent + (update [_ state] + (assoc state :workspace-branch-merges merges-state)))) + +(defn fetch-branch-merges + "Load the merges into the open file's main, newest first, into + `:workspace-branch-merges` (`{:status :loading|:loaded|:error}` plus + `:data`): who merged each branch, when, and the comment left with it. + The command resolves a branch to its main, so it is asked about the + open file as is. A failure is kept as an error, never as an empty list, + so it cannot read as a file with no merges." + [] + (ptk/reify ::fetch-branch-merges + ptk/UpdateEvent + (update [_ state] + (assoc state :workspace-branch-merges {:status :loading})) + + ptk/WatchEvent + (watch [_ state _] + (when-let [file-id (:current-file-id state)] + (->> (rp/cmd! :get-file-branch-merges {:file-id file-id}) + (rx/map #(set-branch-merges {:status :loaded :data %})) + (rx/catch (fn [_] (rx/of (set-branch-merges {:status :error}))))))))) + (defn fetch-branching-limits "Load the size limits this instance refuses beyond, so the panel can state them before a user meets one. The numbers live in the backend @@ -254,15 +282,24 @@ (rx/catch (fn [cause] (rx/of (refusal-notification cause (tr "workspace.branches.lifecycle.error")))))))))) (defn delete-branch - [id] - (assert (uuid? id) "expected valid uuid for `id`") - (ptk/reify ::delete-branch - ptk/WatchEvent - (watch [_ _ _] - (->> (rp/cmd! :delete-file-branch {:id id}) - (rx/mapcat (fn [_] (rx/of (ntf/success (tr "workspace.branches.lifecycle.deleted")) - (fetch-branches)))) - (rx/catch (fn [cause] (rx/of (refusal-notification cause (tr "workspace.branches.lifecycle.error"))))))))) + "Delete a branch. Deleting the open branch takes its file away, so the + caller passes `open-file-id` (the file it comes from) to land there once + the branch is gone; this session is not sent away by the deletion it + made itself." + ([id] (delete-branch id nil)) + ([id open-file-id] + (assert (uuid? id) "expected valid uuid for `id`") + (ptk/reify ::delete-branch + ptk/WatchEvent + (watch [_ _ _] + (->> (rp/cmd! :delete-file-branch {:id id}) + (rx/mapcat (fn [_] (rx/concat + (rx/of (ntf/success (tr "workspace.branches.lifecycle.deleted")) + (fetch-branches)) + (if (uuid? open-file-id) + (rx/of (open-branch open-file-id)) + (rx/empty))))) + (rx/catch (fn [cause] (rx/of (refusal-notification cause (tr "workspace.branches.lifecycle.error")))))))))) (defn materialize-branch "Turn a branch into an ordinary file: `branch-file-id` is the branch's @@ -361,18 +398,19 @@ (defn show-branches "The entry point of the File menu and the Ctrl/Cmd+Alt+B shortcut. A - file that has branches gets the branch switcher; one with none gets the - create-branch dialog, since the switcher only exists once a branch does. - No-op when the branching feature flag is disabled." + file that has branches gets the branches dialog (the full list, the + same the switcher's \"All branches\" opens, which shows this shortcut); + one with none gets the create-branch dialog. No-op when the branching + feature flag is disabled." [] (ptk/reify ::show-branches ptk/WatchEvent (watch [_ state _] (when (contains? cf/flags :branching) (if (has-branches? state) - (rx/of (toggle-branch-switcher true)) - (let [file (get-in state [:files (:current-file-id state)])] - (rx/of (modal/show :create-branch {:file-name (:name file)})))))))) + (rx/of (toggle-branch-switcher false) + (modal/show :branches-list {})) + (rx/of (modal/show :create-branch {}))))))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; COMPARE (read-only 3-way diff) @@ -524,7 +562,7 @@ `main-revn` is main's revn when the command computed those conflicts. It is stored with them because the modal's own diff fetch can still be in flight when the user applies; see `conflict-main-revn`." - [{:keys [branch mode conflicts main-revn excluded]}] + [{:keys [branch mode conflicts main-revn excluded comment]}] (ptk/reify ::open-conflict-resolutions ptk/UpdateEvent (update [_ state] @@ -536,11 +574,13 @@ ptk/WatchEvent (watch [_ _ _] ;; `excluded` is the per-change selection the merge/update attempt - ;; was made with; the resolution modal applies it on retry + ;; was made with, and `comment` what the user said about the merge; + ;; the resolution modal sends both again on retry (rx/of (modal/show :branch-conflicts {:branch branch :mode mode :conflicts conflicts - :excluded excluded}))))) + :excluded excluded + :comment comment}))))) (defn- conflict-main-revn "Main's revn when the server computed the conflict set stored for @@ -662,16 +702,14 @@ (defn merge-branch "Merge `branch` (a branch row/context map with at least `:id`) into main. - On success the branch is marked merged server-side and open clients - reload main via the `:file-merged` msgbus event. + On success the server records the merge and deletes the branch and its + file, and open clients reload main via the `:file-merged` msgbus event. Options: - `:resolutions` — optional `{entity-id (:main|:branch)}` map used to resolve conflicts before integrating. - - `:keep-branch` — when true the merged branch is kept (it shows up - under \"Archived\"); when false (default) the server deletes the - branch and its file in the same transaction, so merged copies do not - pile up and eat disk space. + - `:comment` — what the user says about the merge, kept with its + record and shown in the reviews dialog. - `:excluded` — the per-change selection (change keys, `app.common.files.branch-merge/change-key`) naming the changes the user deselected, which main keeps its own value for. @@ -683,7 +721,7 @@ before that diff has loaded carry the revn of the conflict answer instead (`conflict-main-revn`). Unresolved conflicts open the resolution modal." ([branch] (merge-branch branch nil)) - ([branch {:keys [resolutions keep-branch excluded]}] + ([branch {:keys [resolutions comment excluded]}] (assert (uuid? (:id branch)) "expected a branch row with a valid `:id`") (ptk/reify ::merge-branch ptk/WatchEvent @@ -700,7 +738,7 @@ (cond-> {:branch-id branch-id} (seq resolutions) (assoc :resolutions resolutions) (seq excluded) (assoc :excluded excluded) - keep-branch (assoc :keep-branch true) + (not (str/blank? comment)) (assoc :comment (str/trim comment)) (some? main-revn) (assoc :expected-main-revn main-revn))) (rx/mapcat (fn [{:keys [status source-file-id conflicts] :as answer}] @@ -708,15 +746,16 @@ ;; the merged result lives in main: take the user there ;; to see it (navigate if elsewhere, hard-reload if ;; already on main). Other clients reload via the - ;; `:file-merged` event; the branch deletion (when not - ;; kept) already happened server-side. + ;; `:file-merged` event; the branch deletion already + ;; happened server-side. :merged (rx/of (ntf/success (tr "workspace.branches.merge.success")) (show-merge-result source-file-id)) :conflicts (rx/of (open-conflict-resolutions {:branch branch :mode :merge :conflicts conflicts :main-revn (:main-revn answer) - :excluded excluded})) + :excluded excluded + :comment comment})) :unsupported (rx/of (ntf/warn (tr "workspace.branches.merge.unsupported"))) (rx/of (ntf/error (tr "workspace.branches.merge.error")))))) (rx/catch diff --git a/frontend/src/app/main/ui/ds/foundations/assets/icon.cljs b/frontend/src/app/main/ui/ds/foundations/assets/icon.cljs index c3f17bce70..d287ff53ac 100644 --- a/frontend/src/app/main/ui/ds/foundations/assets/icon.cljs +++ b/frontend/src/app/main/ui/ds/foundations/assets/icon.cljs @@ -155,11 +155,16 @@ (def ^:icon-id gap-horizontal "gap-horizontal") (def ^:icon-id gap-vertical "gap-vertical") (def ^:icon-id git-branch "git-branch") +(def ^:icon-id git-branch-minus "git-branch-minus") (def ^:icon-id git-branch-plus "git-branch-plus") (def ^:icon-id git-commit "git-commit") (def ^:icon-id git-commit-vertical "git-commit-vertical") +(def ^:icon-id git-compare-arrows "git-compare-arrows") +(def ^:icon-id git-graph "git-graph") (def ^:icon-id git-merge "git-merge") +(def ^:icon-id git-merge-conflict "git-merge-conflict") (def ^:icon-id git-pull-request-arrow "git-pull-request-arrow") +(def ^:icon-id git-pull-request-create-arrow "git-pull-request-create-arrow") (def ^:icon-id graphics "graphics") (def ^:icon-id grid "grid") (def ^:icon-id grid-column "grid-column") diff --git a/frontend/src/app/main/ui/ds/product/branch_row.cljs b/frontend/src/app/main/ui/ds/product/branch_row.cljs index 4e850fb875..a44a6cc3fb 100644 --- a/frontend/src/app/main/ui/ds/product/branch_row.cljs +++ b/frontend/src/app/main/ui/ds/product/branch_row.cljs @@ -23,23 +23,47 @@ (mf/defc branch-row* "A branch in a short list, as a button that opens it. The current one is marked and cannot be opened again. `children` go to the end of the row: - usually a `branch-status*`." + usually a `branch-status*`. A label cut short by the row's width shows + in full on hover." {::mf/schema schema:branch-row} [{:keys [class label is-current children] :rest props}] - (let [is-current (d/nilv is-current false) - props (mf/spread-props props - {:class [class (stl/css-case :branch-row true - :is-current is-current)] - :type "button" - :disabled is-current - :aria-current (when is-current "true")})] + (let [is-current (d/nilv is-current false) + + label-ref (mf/use-ref nil) + truncated* (mf/use-state false) + truncated? (deref truncated*) + + check-truncated + (mf/use-fn + (fn [] + (when-let [node (mf/ref-val label-ref)] + (reset! truncated* (> (.-scrollWidth node) (.-clientWidth node)))))) + + ;; the title goes on the button, not the label: the current row is + ;; disabled and still has to show it + props (mf/spread-props props + {:class [class (stl/css-case :branch-row true + :is-current is-current)] + :type "button" + :title (when truncated? label) + :disabled is-current + :aria-current (when is-current "true")})] + + (mf/with-effect [label] + (check-truncated) + (when-let [node (mf/ref-val label-ref)] + (let [observer (js/ResizeObserver. check-truncated)] + (.observe observer node) + #(.disconnect observer)))) + [:> :button props [:> icon* {:icon-id (if is-current i/tick i/git-branch) :size "s" :class (stl/css :icon)}] [:> text* {:as "span" :typography t/body-small - :class (stl/css :label)} + :class (stl/css :label) + :ref label-ref} label] (when (some? children) [:span {:class (stl/css :aside)} diff --git a/frontend/src/app/main/ui/ds/product/branch_row.mdx b/frontend/src/app/main/ui/ds/product/branch_row.mdx index c4813c0ced..b2657fe781 100644 --- a/frontend/src/app/main/ui/ds/product/branch_row.mdx +++ b/frontend/src/app/main/ui/ds/product/branch_row.mdx @@ -31,6 +31,10 @@ Children go to the end of the row, usually a `BranchStatus`. The label is mandatory. The row is a `button`; any other prop (`on-click`, `data-*`) goes to it. +A label too long for the row ends in an ellipsis, and the row then +carries the full label as its `title`, so it shows on hover (the current +row too, though it is disabled). A label that fits gets no `title`. + ```clj [:> branch-row* {:label (:name branch) :data-branch (dm/str (:branch-file-id branch)) diff --git a/frontend/src/app/main/ui/workspace/branch_switcher.cljs b/frontend/src/app/main/ui/workspace/branch_switcher.cljs index 9cbf08a4cb..d652af2a57 100644 --- a/frontend/src/app/main/ui/workspace/branch_switcher.cljs +++ b/frontend/src/app/main/ui/workspace/branch_switcher.cljs @@ -29,6 +29,8 @@ [app.main.ui.ds.product.branch-status :refer [branch-status*]] [app.main.ui.workspace.menu-shortcuts :refer [shortcuts*]] [app.main.ui.workspace.sidebar.pull-requests :as prui] + ;; registers the reviews dialog the popover opens + [app.main.ui.workspace.sidebar.reviews] [app.util.dom :as dom] [app.util.i18n :refer [tr]] [cuerdas.core :as str] @@ -84,14 +86,6 @@ (zero? (or (:behind row) 0)) (zero? (or (:conflicts row) 0)))) -(defn- main-subtitle - "The note beside main. Files that use a shared library read its main, - so on a library it says so." - [library?] - (if library? - (tr "workspace.branches.main-subtitle-library") - (tr "workspace.branches.main-subtitle"))) - ;; --- Popover (mf/defc switcher-row* @@ -104,10 +98,17 @@ :on-click on-open} ;; merged and archived branches carry no change counts (cond - (:is-main row) (main-subtitle (:is-library row)) + (:is-main row) (tr "workspace.branches.main-subtitle") (= "merged" (:status row)) (tr "workspace.branches.header.merged") (= "archived" (:status row)) (tr "workspace.branches.header.archived") - :else [:> row-status* {:row row :pr pr}])]]) + :else [:> row-status* {:row row :pr pr}])] + ;; files that use a shared library read its main: said under the row, + ;; so the note never takes the room of the branch name + (when (and (:is-main row) (:is-library row)) + [:> text* {:as "div" + :typography t/body-small + :class (stl/css :row-note)} + (tr "workspace.branches.main-library-note")])]) (mf/defc closed-group* "A collapsed section of the popover for branches out of the flow: its @@ -134,7 +135,7 @@ (mf/defc switcher-popover* {::mf/private true} - [{:keys [file current prs-by-branch on-close]}] + [{:keys [current prs-by-branch on-close]}] (let [{:keys [data status]} (mf/deref branches) filter* (mf/use-state "") @@ -177,10 +178,17 @@ on-create (mf/use-fn - (mf/deps file on-close) + (mf/deps on-close) (fn [_] (on-close) - (modal/show! :create-branch {:file-name (:name file)}))) + (modal/show! :create-branch {}))) + + on-show-reviews + (mf/use-fn + (mf/deps on-close) + (fn [_] + (on-close) + (modal/show! :branch-reviews {}))) on-show-all (mf/use-fn @@ -251,9 +259,6 @@ :pr (get prs-by-branch (:id row)) :on-open on-open}])]]) - [:> closed-group* {:label (tr "workspace.branches.merged") - :rows (with-status "merged") - :on-open on-open}] [:> closed-group* {:label (tr "workspace.branches.archived") :rows (with-status "archived") :on-open on-open}]]) @@ -262,12 +267,18 @@ ;; branches only grow from main: on a branch there is nothing to create (when (:is-main current) - [:button {:class (stl/css :action) :on-click on-create} - [:> i/icon* {:icon-id i/git-branch-plus :size "s"}] - [:span {:class (stl/css :action-label)} (tr "workspace.branches.switcher.new")]]) + [:* + [:button {:class (stl/css :action) :on-click on-create} + [:> i/icon* {:icon-id i/git-branch-plus :size "s"}] + [:span {:class (stl/css :action-label)} (tr "workspace.branches.switcher.new")]] + [:hr {:class (stl/css :divider)}]]) + + [:button {:class (stl/css :action) :on-click on-show-reviews} + [:> i/icon* {:icon-id i/git-pull-request-arrow :size "s"}] + [:span {:class (stl/css :action-label)} (tr "workspace.branches.switcher.reviews")]] [:button {:class (stl/css :action) :on-click on-show-all} - [:> i/icon* {:icon-id i/menu :size "s"}] + [:> i/icon* {:icon-id i/git-graph :size "s"}] [:span {:class (stl/css :action-label)} (tr "workspace.branches.switcher.all")] [:> shortcuts* {:id :open-branches}]]])) @@ -365,7 +376,7 @@ [:> text* {:as "span" :typography t/body-small :class (stl/css :status-note)} - (main-subtitle (:is-shared file))] + (tr "workspace.branches.main-subtitle")] ;; merged and archived branches open read-only and count no ;; changes: the note says which of the two it is @@ -391,7 +402,6 @@ [:> row-status* {:row ctx :pr pr}])) [:& dropdown {:show open? :on-close on-close :container bar-ref} - [:> switcher-popover* {:file file - :current current + [:> switcher-popover* {:current current :prs-by-branch prs-by-branch :on-close on-close}]]]))) diff --git a/frontend/src/app/main/ui/workspace/branch_switcher.scss b/frontend/src/app/main/ui/workspace/branch_switcher.scss index 8f7a794cab..8a7466fc13 100644 --- a/frontend/src/app/main/ui/workspace/branch_switcher.scss +++ b/frontend/src/app/main/ui/workspace/branch_switcher.scss @@ -24,8 +24,12 @@ padding: var(--sp-xs) var(--sp-m) var(--sp-s); } +// the note gives way before the branch name in the chip does .status-note { - flex-shrink: 0; + min-inline-size: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; color: var(--color-foreground-secondary); } @@ -71,6 +75,15 @@ list-style: none; } +// lined up with the end of the row above, where its subtitle sits +.row-note { + padding-block-end: var(--sp-xs); + padding-inline: var(--sp-s); + color: var(--color-foreground-secondary); + font-style: italic; + text-align: end; +} + .list-note { padding: var(--sp-s); color: var(--color-foreground-secondary); diff --git a/frontend/src/app/main/ui/workspace/main_menu.cljs b/frontend/src/app/main/ui/workspace/main_menu.cljs index e9a4e2b837..8809a2ebc3 100644 --- a/frontend/src/app/main/ui/workspace/main_menu.cljs +++ b/frontend/src/app/main/ui/workspace/main_menu.cljs @@ -968,8 +968,8 @@ ::ev/origin "workspace:menu"}) (modal/show :plugin-management {})))) - ;; the switcher when the file has branches; the create dialog when - ;; it has none, since the switcher only exists once a branch does + ;; the branches dialog when the file has branches; the create + ;; dialog when it has none show-branches (mf/use-fn (fn [event] diff --git a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs index d2d3c0b908..cb1b533c72 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs @@ -22,7 +22,7 @@ [app.main.ui.ds.controls.checkbox :refer [checkbox*]] [app.main.ui.ds.controls.input :refer [input*]] [app.main.ui.ds.foundations.assets.icon :as i] - [app.main.ui.ds.layout.menu :refer [menu* menu-item*]] + [app.main.ui.ds.layout.menu :refer [menu* menu-item* menu-separator*]] [app.main.ui.ds.layout.modal :refer [modal* modal-header* modal-content* modal-footer*]] [app.main.ui.ds.layout.tab-switcher :refer [tab-switcher*]] [app.main.ui.ds.notifications.context-notification :refer [context-notification*]] @@ -232,7 +232,7 @@ (mf/defc create-branch-dialog* {::mf/register modal/components ::mf/register-as :create-branch} - [{:keys [file-name]}] + [] (let [;; a branch only grows from main, so the open file is main library? (boolean (:is-shared (mf/deref refs/file))) branch-name* (mf/use-state "") @@ -261,8 +261,7 @@ [:> modal* {:is-open true :on-open-change (prui/on-dismiss on-close)} [:> prui/dialog-header* {:icon i/git-branch - :title (tr "workspace.branches.create.title") - :subtitle (tr "workspace.branches.create.subtitle" (or file-name ""))}] + :title (tr "workspace.branches.create.title")}] [:> modal-content* {:class (stl/css :dialog-content)} [:> input* {:label (tr "workspace.branches.create.name-label") @@ -303,6 +302,64 @@ :on-click on-submit} (tr "workspace.branches.create.submit")]]])) +;; --- Rename branch dialog (modal) +;; +;; The branch list renames in place; the bottom bar has no room for that, +;; so it asks for the new name here. + +(mf/defc rename-branch-dialog* + {::mf/register modal/components + ::mf/register-as :rename-branch} + [{:keys [branch]}] + (let [branch-name* (mf/use-state (or (:name branch) "")) + branch-name (deref branch-name*) + value (str/trim branch-name) + valid? (and (not (str/blank? value)) + (not= value (:name branch))) + + on-name-change + (mf/use-fn #(reset! branch-name* (dom/get-target-val %))) + + on-close + (mf/use-fn #(st/emit! (modal/hide))) + + on-submit + (mf/use-fn + (mf/deps branch value valid?) + (fn [_] + (when valid? + (st/emit! (dwb/rename-branch (:id branch) value) + (modal/hide))))) + + on-key-down + (mf/use-fn + (mf/deps on-submit) + (fn [event] + (when (kbd/enter? event) + (on-submit event))))] + + [:> modal* {:is-open true + :on-open-change (prui/on-dismiss on-close)} + [:> prui/dialog-header* {:icon i/git-branch + :title (tr "workspace.branches.rename.title") + :subtitle (:name branch)}] + + [:> modal-content* {:class (stl/css :dialog-content)} + [:> input* {:label (tr "workspace.branches.create.name-label") + :icon i/git-branch + :value branch-name + :auto-focus true + :on-change on-name-change + :on-key-down on-key-down}]] + + [:> modal-footer* {} + [:> button* {:variant "secondary" :on-click on-close} + (tr "labels.cancel")] + [:> button* {:variant "primary" + :disabled (not valid?) + :on-click on-submit} + (tr "labels.rename")]]])) + ;; --- Preview limits dialog (modal) ;; ;; Every refusal the branching commands can emit has a line here, so a user @@ -381,8 +438,8 @@ {::mf/register modal/components ::mf/register-as :merge-branch} [{:keys [branch excluded]}] - (let [archive?* (mf/use-state false) - archive? (deref archive?*) + (let [comment* (mf/use-state "") + comment (deref comment*) ;; The branch name and "main" are highlighted (foreground-primary) ;; within the otherwise secondary-colored prompt. We wrap each of @@ -396,17 +453,17 @@ (str sep (tr "workspace.branches.main") sep))] (str/split msg sep)) - on-toggle-archive - (mf/use-fn #(swap! archive?* not)) + on-comment-change + (mf/use-fn #(reset! comment* (dom/get-target-val %))) on-close (mf/use-fn #(st/emit! (modal/hide))) on-submit (mf/use-fn - (mf/deps branch archive? excluded) + (mf/deps branch comment excluded) (fn [_] - (st/emit! (dwb/merge-branch branch {:keep-branch archive? :excluded excluded}) + (st/emit! (dwb/merge-branch branch {:comment comment :excluded excluded}) (modal/hide))))] [:> modal* {:is-open true @@ -420,16 +477,20 @@ [:span {:key idx :class (stl/css :modal-message-emphasis)} part] [:span {:key idx} part]))] - [:> checkbox* {:id "merge-archive-branch" - :label (tr "workspace.branches.merge.archive-label") - :checked archive? - :on-change on-toggle-archive}] + ;; kept with the record of the merge, in the reviews dialog + [:div {:class (stl/css :field)} + [:label {:class (stl/css :field-label) + :for "merge-branch-comment"} + (tr "workspace.branches.merge.comment-label")] + [:textarea {:id "merge-branch-comment" + :class (stl/css :textarea) + :value comment + :max-length 4000 + :on-change on-comment-change + :rows 3}]] - (if archive? - [:> context-notification* {:level :warning :type :context} - (tr "workspace.branches.merge.archive-warning")] - [:> context-notification* {:level :info :type :context} - (tr "workspace.branches.merge.delete-info")])] + [:> context-notification* {:level :info :type :context} + (tr "workspace.branches.merge.delete-info")]] [:> modal-footer* {} [:> button* {:variant "secondary" :on-click on-close} @@ -442,8 +503,8 @@ ;; --- Confirmation for relevant branch actions (merge / update / materialize) (defn- confirm-merge! - "Open the merge dialog for `branch` (merge into main is irreversible; the - dialog also lets the user choose whether to keep the branch archived). + "Open the merge dialog for `branch` (merge into main is irreversible and + deletes the branch; the dialog asks for a comment for its record). `excluded` is the compare dialog's per-change selection, if any." ([branch] (confirm-merge! branch nil)) ([branch excluded] @@ -474,6 +535,45 @@ :accept-style :danger :on-accept (fn [_] (st/emit! (dwb/materialize-branch (:branch-file-id branch))))}))) +(defn- confirm-archive! + "Ask for confirmation before archiving `branch`, or restoring it with + `archived?` false. The confirmation takes the place of any open dialog." + [branch archived?] + (st/emit! (modal/show {:type :confirm + :title (if archived? + (tr "workspace.branches.archive.title") + (tr "workspace.branches.restore.title")) + :message (if archived? + (tr "workspace.branches.archive.message" (:name branch)) + (tr "workspace.branches.restore.message" (:name branch))) + :accept-label (if archived? + (tr "workspace.branches.menu.archive") + (tr "workspace.branches.menu.restore")) + :accept-style :primary + :on-accept (fn [_] (st/emit! (dwb/archive-branch (:id branch) archived?)))}))) + +(defn- confirm-delete! + "Ask for confirmation before deleting `branch`. `open-file-id`, when + given, is the file to land on afterwards: the one the open branch comes + from, because deleting it takes the open file away." + ([branch] (confirm-delete! branch nil)) + ([branch open-file-id] + (st/emit! (modal/show {:type :confirm + :title (tr "workspace.branches.delete.title") + :message (tr "workspace.branches.delete.message" (:name branch)) + :accept-label (tr "labels.delete") + :accept-style :danger + :on-accept (fn [_] (st/emit! (dwb/delete-branch (:id branch) open-file-id)))})))) + +;; --- Profiles shown as avatars + +(defn- avatar-profile + "The fields of a profile an avatar reads. The logged-in profile carries + more, such as a `:theme` that is nil until the user picks one, which the + avatar's schema refuses (it takes a string or nothing)." + [profile] + (select-keys profile [:id :fullname :email :photo-id :photo-url])) + ;; --- Diff counts: known, or honestly unknown ;; ;; A row whose comparison could not be made carries no counts at all (and @@ -495,7 +595,7 @@ (let [profile (mf/deref refs/profile) profiles (mf/deref refs/profiles) author (or (get profiles (:created-by branch)) - (when (= (:created-by branch) (:id profile)) profile)) + (when (= (:created-by branch) (:id profile)) (avatar-profile profile))) author? (= (:created-by branch) (:id profile)) status (:status branch) merged? (= "merged" status) @@ -521,9 +621,7 @@ on-compare (mf/use-fn (mf/deps branch) #(modal/show! :branch-compare {:branch branch})) on-restore (mf/use-fn (mf/deps branch) - (fn [_] - (st/emit! (modal/hide) - (dwb/archive-branch (:id branch) false)))) + #(confirm-archive! branch false)) on-materialize (mf/use-fn (mf/deps branch) #(confirm-materialize! branch))] [:> modal* {:is-open true @@ -613,20 +711,24 @@ [:dd {:class (stl/css :info-val)} (-> (:merged-at branch) ct/inst (ct/format-inst :localized-date-time))]])]] + ;; the dialog closes from its header; the footer only acts on the + ;; branch. The work of a merged branch is in main: there is nothing + ;; to compare [:> modal-footer* {} - ;; an archived branch is read-only: it comes back to work restored, - ;; or leaves the branch engine as an ordinary file - (when archived? - [:> button* {:variant "secondary" :icon i/history :on-click on-restore} - (tr "workspace.branches.menu.restore")]) - (when archived? - [:> button* {:variant "secondary" :icon i/document :on-click on-materialize} - (tr "workspace.branches.materialize.action")]) - ;; the work of a merged branch is in main: there is nothing to compare (when-not merged? - [:> button* {:variant "secondary" :icon i/switch :on-click on-compare} + [:> button* {:variant (if archived? "secondary" "primary") + :icon i/git-compare-arrows + :on-click on-compare} (tr "workspace.branches.compare")]) - [:> button* {:variant "primary" :on-click on-close} (tr "labels.close")]]])) + ;; an archived branch is read-only: it leaves the branch engine as an + ;; ordinary file, or comes back to work restored, its primary action. + ;; The buttons follow the bottom bar of an open archived branch + (when archived? + [:* + [:> button* {:variant "secondary" :icon i/git-branch-minus :on-click on-materialize} + (tr "workspace.branches.materialize.action")] + [:> button* {:variant "primary" :icon i/history :on-click on-restore} + (tr "workspace.branches.menu.restore")]])]])) ;; --- Branch list entry @@ -769,7 +871,7 @@ (fn [event] (dom/stop-propagation event) (on-set-menu false) - (st/emit! (dwb/archive-branch (:id entry) (not archived?))))) + (confirm-archive! entry (not archived?)))) on-delete (mf/use-fn @@ -777,12 +879,7 @@ (fn [event] (dom/stop-propagation event) (on-set-menu false) - (st/emit! (modal/show {:type :confirm - :title (tr "workspace.branches.delete.title") - :message (tr "workspace.branches.delete.message" (:name entry)) - :accept-label (tr "labels.delete") - :accept-style :danger - :on-accept (fn [_] (st/emit! (dwb/delete-branch (:id entry))))})))) + (confirm-delete! entry))) ;; the exit door: the branch stops being a branch but keeps its ;; content, and the confirmation says so, because the branch row and @@ -908,6 +1005,7 @@ (when-not merged? [:> menu-item* {:id "materialize" :on-action on-materialize} (tr "workspace.branches.materialize.action")]) + [:> menu-separator* {}] [:> menu-item* {:id "delete" :on-action on-delete} (tr "labels.delete")]]])]])) @@ -915,12 +1013,11 @@ (defn- branches-for-tab "The branch rows a tab of the all-branches dialog lists: the open ones, - the open ones the profile created, the merged ones, or the archived - ones." + the open ones the profile created, or the archived ones. A merge deletes + its branch, so no tab lists merged branches." [tab rows profile-id] (case tab "mine" (filter #(and (= "open" (:status %)) (= profile-id (:created-by %))) rows) - "merged" (filter #(= "merged" (:status %)) rows) "archived" (filter #(= "archived" (:status %)) rows) (filter #(= "open" (:status %)) rows))) @@ -937,7 +1034,7 @@ profile-id (:id profile) profiles (cond-> profiles - profile-id (assoc profile-id profile)) + profile-id (assoc profile-id (avatar-profile profile))) prs-by-branch (mf/with-memo [prs] @@ -955,7 +1052,16 @@ ;; only one entry's options dropdown may be open at a time open-menu* (mf/use-state nil) open-menu (deref open-menu*) - set-menu (mf/use-fn (fn [k open?] (reset! open-menu* (when open? k)))) + ;; the DS menu closes every other open menu when one opens, and + ;; each of them answers with `open?` false: closing one entry's + ;; menu must not clear the menu another entry just opened + set-menu (mf/use-fn + (fn [k open?] + (swap! open-menu* (fn [current] + (cond + open? k + (= current k) nil + :else current))))) matching (mf/with-memo [data filter-v] @@ -968,7 +1074,6 @@ (let [label (fn [text t] (dm/str text " (" (count (branches-for-tab t data profile-id)) ")"))] [{:id "mine" :label (label (tr "workspace.branches.list.tab.mine") "mine")} {:id "active" :label (label (tr "workspace.branches.list.tab.active") "active")} - {:id "merged" :label (label (tr "workspace.branches.merged") "merged")} {:id "archived" :label (label (tr "workspace.branches.archived") "archived")}])) rows (branches-for-tab tab matching profile-id) @@ -976,14 +1081,16 @@ on-tab-change (mf/use-fn #(reset! tab* %)) on-filter-change (mf/use-fn #(reset! filter* (dom/get-target-val %))) + ;; the input's wrapper takes the full width it is given: this + ;; box sets that width, so the tabs keep room for their labels search (mf/html - [:> input* {:variant "dense" - :icon i/search - :class (stl/css :branches-list-search) - :placeholder (tr "workspace.branches.search.placeholder") - :value filter-v - :on-change on-filter-change}]) + [:div {:class (stl/css :branches-list-search)} + [:> input* {:variant "dense" + :icon i/search + :placeholder (tr "workspace.branches.search.placeholder") + :value filter-v + :on-change on-filter-change}]]) on-open-change (mf/use-fn @@ -995,10 +1102,7 @@ (mf/use-fn #(modal/show! :branch-limits {})) on-create - (mf/use-fn - (mf/deps file) - (fn [_] - (modal/show! :create-branch {:file-name (:name file)}))) + (mf/use-fn #(modal/show! :create-branch {})) on-retry (mf/use-fn #(st/emit! (dwb/init-branches-state)))] @@ -1186,7 +1290,7 @@ (cond (nil? item) [:div {:class (stl/css :compare-detail-empty)} - [:> empty-state* {:icon i/switch + [:> empty-state* {:icon i/git-compare-arrows :text (tr "workspace.branches.compare.select-hint")}]] :else @@ -1291,7 +1395,7 @@ :on-open-change (prui/on-dismiss on-close) :size "xlarge"} [:> prui/dialog-header* - {:icon i/switch + {:icon i/git-compare-arrows :title (tr "workspace.branches.compare.title") :subtitle (mf/html @@ -1332,7 +1436,7 @@ (cond (= status :loading) [:div {:class (stl/css :compare-empty)} - [:> empty-state* {:icon i/switch + [:> empty-state* {:icon i/git-compare-arrows :text (tr "workspace.branches.compare.loading")}]] (= status :error) @@ -1342,7 +1446,7 @@ (empty? items) [:div {:class (stl/css :compare-empty)} - [:> empty-state* {:icon i/git-branch + [:> empty-state* {:icon i/git-compare-arrows :text (tr "workspace.branches.compare.empty")}]] :else @@ -1428,7 +1532,7 @@ (cond conflicts? [:> button* {:variant "primary" - :icon i/git-merge + :icon i/git-merge-conflict :on-click on-resolve} (tr "workspace.branches.conflicts.resolve")] @@ -1651,16 +1755,11 @@ (mf/defc branch-conflicts-dialog* {::mf/register modal/components ::mf/register-as :branch-conflicts} - [{:keys [branch mode conflicts excluded]}] + [{:keys [branch mode conflicts excluded comment]}] (let [{:keys [diff selected resolutions status]} (mf/deref branch-diff) loading? (= status :loading) - ;; merge mode only: whether to keep the branch archived after merge - keep?* (mf/use-state false) - keep? (deref keep?*) - on-toggle-keep (mf/use-fn #(swap! keep?* not)) - diff-meta (:meta diff) ;; per-side header subtitle: base is pinned at branch creation, main ;; and branch show how recent each one is ("3 hours ago"); the branch @@ -1705,12 +1804,13 @@ #(when sel (st/emit! (dwb/set-conflict-resolution (:id sel) :main)))) on-use-branch (mf/use-fn (mf/deps sel) #(when sel (st/emit! (dwb/set-conflict-resolution (:id sel) :branch)))) - on-apply (mf/use-fn (mf/deps branch resolutions mode keep? excluded) + ;; a merge sends again the comment of the merge dialog + on-apply (mf/use-fn (mf/deps branch resolutions mode excluded comment) #(st/emit! (if (= mode :update) (dwb/update-branch-from-main branch {:resolutions resolutions :excluded excluded}) (dwb/merge-branch branch {:resolutions resolutions - :keep-branch keep? + :comment comment :excluded excluded}))))] ;; the fetched diff backs both the fallback conflict set and the side @@ -1724,7 +1824,7 @@ [:> modal* {:is-open true :on-open-change (prui/on-dismiss on-close) :size "xlarge"} - [:> prui/dialog-header* {:icon i/triangle-alert + [:> prui/dialog-header* {:icon i/git-merge-conflict :tone :warning :title (tr "workspace.branches.conflicts.title") :subtitle (tr "workspace.branches.conflicts.subtitle" (:name branch))} @@ -1747,12 +1847,12 @@ (cond loading? [:div {:class (stl/css :compare-empty)} - [:> empty-state* {:icon i/git-merge + [:> empty-state* {:icon i/git-merge-conflict :text (tr "workspace.branches.compare.loading")}]] (empty? conflicts) [:div {:class (stl/css :compare-empty)} - [:> empty-state* {:icon i/git-merge + [:> empty-state* {:icon i/git-merge-conflict :text (tr "workspace.branches.conflicts.empty")}]] :else @@ -1837,13 +1937,6 @@ :end (mf/html [:* - ;; applying a MERGE deletes the branch unless the user opts to - ;; keep it archived — same choice the plain merge dialog offers - (when (not= mode :update) - [:> checkbox* {:id "conflicts-archive-branch" - :label (tr "workspace.branches.merge.archive-label") - :checked keep? - :on-change on-toggle-keep}]) [:> button* {:variant "secondary" :on-click on-close} (tr "labels.cancel")] [:> button* {:variant "primary" :icon i/git-merge @@ -1945,6 +2038,27 @@ (= (:id ctx) (:file-branch-id %)))) (first))) + profile (mf/deref refs/profile) + team (mf/deref refs/team) + pr-author? (and (some? open-pr) (= (:created-by open-pr) (:id profile))) + admin? (boolean (get-in team [:permissions :is-admin])) + can-merge? (boolean (get-in team [:permissions :can-edit])) + + open? (not (or merged? archived?)) + ahead (or (:ahead ctx) 0) + behind (or (:behind ctx) 0) + + ;; the one action an open branch offers as its primary button, most + ;; pressing first; the options menu leaves it out + primary + (when open? + (cond + (pos? (or (:conflicts ctx) 0)) :resolve + (pos? behind) :update + (and prs-enabled? (some? open-pr)) :open-review + (and prs-enabled? (pos? ahead)) :request-review + (pos? ahead) :merge)) + menu-open* (mf/use-state false) menu-open? (deref menu-open*) on-toggle-menu (mf/use-fn #(swap! menu-open* not)) @@ -1953,7 +2067,7 @@ on-compare (mf/use-fn (mf/deps ctx) #(modal/show! :branch-compare {:branch ctx})) on-update (mf/use-fn (mf/deps ctx) #(confirm-update! ctx)) on-open-main (mf/use-fn (mf/deps ctx) #(st/emit! (dwb/open-branch (:source-file-id ctx)))) - on-restore (mf/use-fn (mf/deps ctx) #(st/emit! (dwb/archive-branch (:id ctx) false))) + on-restore (mf/use-fn (mf/deps ctx) #(confirm-archive! ctx false)) on-materialize (mf/use-fn (mf/deps ctx) #(confirm-materialize! ctx)) on-resolve (mf/use-fn (mf/deps ctx) #(modal/show! :branch-conflicts {:branch ctx :mode :merge})) @@ -1974,6 +2088,27 @@ (mf/deps open-pr) #(st/emit! (dwpr/open-pull-request open-pr))) + ;; the same options as the branch's entry in the branch list; + ;; deleting the open branch lands on the file it comes from + on-menu-action + (mf/use-fn + (mf/deps ctx open-pr) + (fn [action] + (case action + "info" (modal/show! :branch-info {:branch ctx}) + "update" (confirm-update! ctx) + "request-review" (modal/show! :create-pull-request {:branch ctx}) + "open-review" (st/emit! (dwpr/open-pull-request open-pr)) + "review-details" (modal/show! :pull-request-info {:pr open-pr}) + "publish-review" (st/emit! (dwpr/update-pull-request-snapshot (:id open-pr))) + "merge" (confirm-merge! ctx) + "cancel-review" (prui/confirm-close! open-pr) + "rename" (modal/show! :rename-branch {:branch ctx}) + "archive" (confirm-archive! ctx true) + "materialize" (confirm-materialize! ctx) + "delete" (confirm-delete! ctx (:source-file-id ctx)) + nil))) + ;; publishes the room the banner takes from the bottom of the canvas ;; whenever its box changes size, which is what keeps the floating ;; colour/typography palette bar clear of it in both the collapsed and @@ -2041,85 +2176,122 @@ [:span {:class (stl/css :branch-banner-title)} (:label status)]] [:div {:class (stl/css :branch-banner-separator)}] - (cond - merged? - [:> button* {:variant "secondary" - :icon i/arrow-up-right - :on-click on-open-main} - (tr "workspace.branches.banner.view-main")] - - ;; an archived branch was set aside unmerged and is read-only: it - ;; can be compared with main, brought back to work, or taken out - ;; as an ordinary file - archived? - [:div {:class (stl/css :branch-banner-actions)} + [:div {:class (stl/css :branch-banner-actions)} + (cond + merged? [:> button* {:variant "secondary" - :icon i/switch - :on-click on-compare} - (tr "workspace.branches.compare")] - [:> button* {:variant "secondary" - :icon i/document - :on-click on-materialize} - (tr "workspace.branches.materialize.action")] - [:> button* {:variant "primary" - :icon i/history - :on-click on-restore} - (tr "workspace.branches.menu.restore")]] + :icon i/arrow-up-right + :on-click on-open-main} + (tr "workspace.branches.banner.view-main")] - :else - (let [conflicts (or (:conflicts ctx) 0)] - [:div {:class (stl/css :branch-banner-actions)} - [:> button* {:variant "secondary" - :icon i/switch - :on-click on-compare} - (tr "workspace.branches.compare")] + ;; an archived branch was set aside unmerged and is read-only: it + ;; can be compared with main, brought back to work, or taken out + ;; as an ordinary file + archived? + [:* + [:> button* {:variant "secondary" + :icon i/git-compare-arrows + :on-click on-compare} + (tr "workspace.branches.compare")] + [:> button* {:variant "secondary" + :icon i/git-branch-minus + :on-click on-materialize} + (tr "workspace.branches.materialize.action")] + [:> button* {:variant "primary" + :icon i/history + :on-click on-restore} + (tr "workspace.branches.menu.restore")]] - (cond - (pos? conflicts) - [:> button* {:variant "primary" - :icon i/triangle-alert - :on-click on-resolve} - (tr "workspace.branches.conflicts.resolve")] + :else + [:* + [:> button* {:variant "secondary" + :icon i/git-compare-arrows + :on-click on-compare} + (tr "workspace.branches.compare")] - (pos? (or (:behind ctx) 0)) - [:> button* {:variant "primary" - :icon i/status-update - :on-click on-update} - (tr "workspace.branches.update")] + ;; with pull requests enabled the primary flow is asking for a + ;; review (merging moves to the options menu) + (case primary + :resolve + [:> button* {:variant "primary" + :icon i/git-merge-conflict + :on-click on-resolve} + (tr "workspace.branches.conflicts.resolve")] - ;; with pull requests enabled the primary flow is asking - ;; for a review (merging moves behind the kebab menu) - (and prs-enabled? (some? open-pr)) - [:> button* {:variant "primary" - :icon i/git-pull-request-arrow - :on-click on-open-review} - (tr "workspace.pull-requests.actions.open-review")] + :update + [:> button* {:variant "primary" + :icon i/status-update + :on-click on-update} + (tr "workspace.branches.update")] - (and prs-enabled? (pos? (or (:ahead ctx) 0))) - [:> button* {:variant "primary" - :icon i/git-pull-request-arrow - :on-click on-request-review} - (tr "workspace.pull-requests.menu.request-review")] + :open-review + [:> button* {:variant "primary" + :icon i/git-pull-request-arrow + :on-click on-open-review} + (tr "workspace.pull-requests.actions.open-review")] - (pos? (or (:ahead ctx) 0)) - [:> button* {:variant "primary" - :icon i/git-merge - :on-click on-merge} - (tr "workspace.branches.merge.action")]) + :request-review + [:> button* {:variant "primary" + :icon i/git-pull-request-create-arrow + :on-click on-request-review} + (tr "workspace.pull-requests.menu.request-review")] - (when prs-enabled? - ;; the banner sits at the bottom of the canvas: its menu - ;; opens upwards - [:div {:class (stl/css :branch-banner-menu)} - [:> menu* {:is-open menu-open? - :on-open-change on-menu-change - :placement "top end" - :trigger - (mf/html - [:> icon-button* {:variant "ghost" - :icon i/menu - :aria-label (tr "labels.options") - :aria-pressed menu-open? - :on-click on-toggle-menu}])} - [:> menu-item* {:id "merge" :on-action on-merge} - (tr "workspace.pull-requests.actions.merge")]]])]))]]))) + :merge + [:> button* {:variant "primary" + :icon i/git-merge + :on-click on-merge} + (tr "workspace.branches.merge.action")] + + nil)]) + + ;; the branch's options, minus what the banner already offers as + ;; a button; the banner sits at the bottom of the canvas, so the + ;; menu opens upwards + [:div {:class (stl/css :branch-banner-menu)} + [:> menu* {:is-open menu-open? + :on-open-change on-menu-change + :on-action on-menu-action + :placement "top end" + :trigger + (mf/html + [:> icon-button* {:variant "ghost" + :icon i/menu + :aria-label (tr "labels.options") + :aria-pressed menu-open? + :on-click on-toggle-menu}])} + [:> menu-item* {:id "info"} + (tr "workspace.branches.menu.info")] + (when (and open? (pos? behind) (not= primary :update)) + [:> menu-item* {:id "update"} + (tr "workspace.branches.update")]) + (when (and open? prs-enabled? (nil? open-pr) (not= primary :request-review)) + [:> menu-item* {:id "request-review"} + (tr "workspace.pull-requests.menu.request-review")]) + (when (and (some? open-pr) (not= primary :open-review)) + [:> menu-item* {:id "open-review"} + (tr "workspace.pull-requests.actions.open-review")]) + (when (some? open-pr) + [:> menu-item* {:id "review-details"} + (tr "workspace.pull-requests.menu.review-details")]) + (when pr-author? + [:> menu-item* {:id "publish-review"} + (tr "workspace.pull-requests.menu.publish")]) + (when (and open? prs-enabled? (not= primary :merge) + (or (nil? open-pr) can-merge?)) + [:> menu-item* {:id "merge"} + (tr "workspace.pull-requests.actions.merge")]) + (when (and (some? open-pr) (or pr-author? admin?)) + [:> menu-item* {:id "cancel-review"} + (tr "workspace.pull-requests.actions.close")]) + (when open? + [:> menu-item* {:id "rename"} + (tr "labels.rename")]) + (when open? + [:> menu-item* {:id "archive"} + (tr "workspace.branches.menu.archive")]) + (when open? + [:> menu-item* {:id "materialize"} + (tr "workspace.branches.materialize.action")]) + [:> menu-separator* {}] + [:> menu-item* {:id "delete"} + (tr "labels.delete")]]]]]]))) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branches.scss b/frontend/src/app/main/ui/workspace/sidebar/branches.scss index 41a3aa8715..ba94c39431 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branches.scss +++ b/frontend/src/app/main/ui/workspace/sidebar/branches.scss @@ -14,6 +14,7 @@ .branches-empty { display: flex; flex-direction: column; + align-items: center; gap: var(--sp-xxxl); margin: var(--sp-xxxl) auto; inline-size: $sz-200; @@ -154,8 +155,8 @@ } // Merge-dialog confirmation prompt: a regular-weight, secondary-color -// paragraph sitting between the title and the archive controls, with a -// touch more breathing room before the checkbox. +// paragraph sitting between the title and the comment field, with a +// touch more breathing room before it. .modal-message { @include use-typography("body-large"); @@ -1112,7 +1113,7 @@ .branch-banner { position: absolute; - inset-block-end: var(--sp-m); + inset-block-end: var(--sp-xs); inset-inline-start: 50%; transform: translateX(-50%); max-inline-size: 90vw; @@ -1253,8 +1254,7 @@ } .branches-list-search { - inline-size: $sz-224; - margin-inline-start: auto; + inline-size: $sz-192; } .branches-list-entries { diff --git a/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs b/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs index e511624e85..f9c485f97f 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs @@ -210,7 +210,7 @@ [:> modal* {:is-open true :on-open-change (on-dismiss on-close) :size "large"} - [:> dialog-header* {:icon i/git-pull-request-arrow + [:> dialog-header* {:icon i/git-pull-request-create-arrow :title (tr "workspace.pull-requests.create.title") :subtitle (tr "workspace.pull-requests.create.subtitle" (or (:name branch) ""))}] @@ -255,7 +255,7 @@ [:> button* {:variant "secondary" :on-click on-close} (tr "labels.cancel")] [:> button* {:variant "primary" - :icon i/git-pull-request-arrow + :icon i/git-pull-request-create-arrow :disabled (not valid?) :on-click on-submit} (tr "workspace.pull-requests.create.submit")]]])) @@ -608,7 +608,7 @@ [:div {:class (stl/css :pr-banner-actions)} [:> button* {:variant "secondary" - :icon i/switch + :icon i/git-compare-arrows :on-click on-compare} (tr "workspace.branches.compare")] diff --git a/frontend/src/app/main/ui/workspace/sidebar/reviews.cljs b/frontend/src/app/main/ui/workspace/sidebar/reviews.cljs new file mode 100644 index 0000000000..3289ad2c01 --- /dev/null +++ b/frontend/src/app/main/ui/workspace/sidebar/reviews.cljs @@ -0,0 +1,405 @@ +;; 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/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns app.main.ui.workspace.sidebar.reviews + "The reviews dialog: the merges into main, each with who made it, when + and the comment left with it, and the design reviews (pull requests) of + the file. It follows the all-branches dialog: tabs with their counts, + a search box and one list per tab." + (:require-macros [app.main.style :as stl]) + (:require + [app.common.data.macros :as dm] + [app.common.time :as ct] + [app.main.data.modal :as modal] + [app.main.data.workspace.branches :as dwb] + [app.main.data.workspace.pull-requests :as dwpr] + [app.main.refs :as refs] + [app.main.store :as st] + [app.main.ui.ds.buttons.button :refer [button*]] + [app.main.ui.ds.buttons.icon-button :refer [icon-button*]] + [app.main.ui.ds.controls.input :refer [input*]] + [app.main.ui.ds.foundations.assets.icon :as i] + [app.main.ui.ds.layout.menu :refer [menu* menu-item*]] + [app.main.ui.ds.layout.modal :refer [modal* modal-header* modal-content*]] + [app.main.ui.ds.layout.tab-switcher :refer [tab-switcher*]] + [app.main.ui.ds.product.avatar :refer [avatar*]] + [app.main.ui.ds.product.branch-pill :refer [branch-pill*]] + [app.main.ui.ds.product.empty-state :refer [empty-state*]] + [app.main.ui.workspace.sidebar.pull-requests :as prui :refer [pr-state-badge*]] + [app.util.dom :as dom] + [app.util.i18n :refer [tr]] + [app.util.keyboard :as kbd] + [cuerdas.core :as str] + [okulary.core :as l] + [rumext.v2 :as mf])) + +(def ^:private merges + (l/derived :workspace-branch-merges st/state)) + +(defn- matches? + "Whether any of `values` contains the search text, ignoring case. An + empty search matches everything." + [filter-v values] + (or (str/blank? filter-v) + (let [needle (str/lower filter-v)] + (some #(str/includes? (str/lower (or % "")) needle) values)))) + +;; --- Shared pieces + +(mf/defc clamped-text* + "A text of any length shown on two lines at most. When it does not fit, + a \"See more\" link shows the rest and \"See less\" folds it back. The + link is a button of its own, so it never acts on the entry around it." + {::mf/private true} + [{:keys [text]}] + (let [text-ref (mf/use-ref nil) + expanded* (mf/use-state false) + expanded? (deref expanded*) + overflow* (mf/use-state false) + overflow? (deref overflow*) + + on-toggle + (mf/use-fn + (fn [event] + (dom/stop-propagation event) + (swap! expanded* not)))] + + ;; measured only while folded: unfolded, the text always fits + (mf/with-effect [text expanded?] + (when-not expanded? + (when-let [node (mf/ref-val text-ref)] + (let [measure #(reset! overflow* (> (.-scrollHeight node) (.-clientHeight node))) + observer (js/ResizeObserver. measure)] + (measure) + (.observe observer node) + #(.disconnect observer))))) + + [:div {:class (stl/css :clamped)} + [:p {:ref text-ref + :class (stl/css-case :clamped-text true + :is-folded (not expanded?))} + text] + (when (or overflow? expanded?) + [:button {:type "button" + :class (stl/css :see-more) + :aria-expanded expanded? + :on-click on-toggle} + (if expanded? + (tr "workspace.branches.reviews.see-less") + (tr "workspace.branches.reviews.see-more"))])])) + +(mf/defc entry-date* + "When something happened, as time ago, with the full date on hover." + {::mf/private true} + [{:keys [date]}] + (when date + (let [date (ct/inst date)] + [:time {:class (stl/css :entry-date) + :date-time (ct/format-inst date :iso) + :title (ct/format-inst date :localized-date-time)} + (ct/timeago date)]))) + +(mf/defc route* + "The branch the changes come from and main, where they land." + {::mf/private true} + [{:keys [branch-name is-review]}] + [:div {:class (stl/css :entry-route)} + [:> branch-pill* {:label (or branch-name "—") + :is-review is-review}] + [:> i/icon* {:icon-id i/arrow-right + :size "s" + :class (stl/css :entry-route-arrow)}] + [:> branch-pill* {:label (tr "workspace.branches.main") + :is-main true}]]) + +(mf/defc author* + "The profile behind an entry: avatar and name. A profile that left the + team is no longer among the team's profiles and shows as unknown." + {::mf/private true} + [{:keys [profile]}] + (if profile + [:> avatar* {:profile profile + :variant "M" + :class (stl/css :entry-avatar)}] + [:div {:class (stl/css :entry-avatar :entry-avatar-unknown)} + [:> i/icon* {:icon-id i/user :size "s"}]])) + +;; --- Merge reviews + +(mf/defc merge-entry* + {::mf/private true} + [{:keys [merge profiles]}] + (let [author (get profiles (:merged-by merge))] + [:li {:class (stl/css :entry)} + [:> author* {:profile author}] + [:div {:class (stl/css :entry-body)} + [:div {:class (stl/css :entry-head)} + [:span {:class (stl/css :entry-title)} + (or (:fullname author) (tr "workspace.branches.reviews.unknown-author"))] + [:> entry-date* {:date (:created-at merge)}]] + [:div {:class (stl/css :entry-meta)} + [:span {:class (stl/css :entry-meta-text)} + (tr "workspace.branches.reviews.merged")] + [:> route* {:branch-name (:branch-name merge)}]] + ;; the merge accepted a design review: name it, it may be gone by now + (when-let [title (:pull-request-title merge)] + [:div {:class (stl/css :entry-review)} + [:> i/icon* {:icon-id i/git-pull-request-arrow :size "s"}] + [:span {:class (stl/css :entry-review-title)} + (tr "workspace.branches.reviews.from-review" title)]]) + (if (str/blank? (:comment merge)) + [:p {:class (stl/css :entry-no-comment)} + (tr "workspace.branches.reviews.no-comment")] + [:> clamped-text* {:text (:comment merge)}])]])) + +;; --- Design reviews + +(mf/defc design-review-entry* + "A design review: the entry opens it while it is open; a finished one + (merged or closed) can no longer be opened, so the entry shows its + details instead. Its options menu shows the details of every review and + reopens a closed one for its author or a team admin, as the details + dialog does." + {::mf/private true} + [{:keys [pr profiles]}] + (let [author (get profiles (:created-by pr)) + profile (mf/deref refs/profile) + team (mf/deref refs/team) + open? (= "open" (:status pr)) + closed? (= "closed" (:status pr)) + can-reopen? (and closed? + (or (= (:created-by pr) (:id profile)) + (boolean (get-in team [:permissions :is-admin])))) + [approvals total] (prui/current-approvals pr) + + ;; each entry keeps its own menu state: the DS menu closes every + ;; other open menu by itself when one opens + menu-open* (mf/use-state false) + menu-open? (deref menu-open*) + on-toggle-menu (mf/use-fn #(swap! menu-open* not)) + on-menu-change (mf/use-fn #(reset! menu-open* %)) + + on-menu-action + (mf/use-fn + (mf/deps pr) + (fn [action] + (case action + "details" (modal/show! :pull-request-info {:pr pr}) + "reopen" (st/emit! (dwpr/reopen-pull-request (:id pr))) + nil))) + + on-open + (mf/use-fn + (mf/deps pr open?) + (fn [_] + (if open? + (st/emit! (modal/hide) + (dwpr/open-pull-request pr)) + (modal/show! :pull-request-info {:pr pr})))) + + on-key-down + (mf/use-fn + (mf/deps on-open) + (fn [event] + (when (or (kbd/enter? event) (kbd/space? event)) + (dom/prevent-default event) + (on-open event))))] + + [:li {:class (stl/css :entry :is-clickable) + :role "button" + :tab-index 0 + :title (if open? + (tr "workspace.pull-requests.actions.open-review") + (tr "workspace.pull-requests.menu.review-details")) + :on-click on-open + :on-key-down on-key-down} + [:> author* {:profile author}] + [:div {:class (stl/css :entry-body)} + [:div {:class (stl/css :entry-head)} + [:span {:class (stl/css :entry-title)} (:title pr)] + [:> pr-state-badge* {:pr pr}] + [:> entry-date* {:date (:created-at pr)}]] + [:div {:class (stl/css :entry-meta)} + [:span {:class (stl/css :entry-meta-text)} + (or (:fullname author) (tr "workspace.branches.reviews.unknown-author"))] + [:> route* {:branch-name (:branch-name pr) :is-review open?}] + (when (pos? total) + [:span {:class (stl/css :entry-approvals) + :title (tr "workspace.pull-requests.info.approvals")} + [:> i/icon* {:icon-id i/tick :size "s"}] + (dm/str approvals "/" total)])] + (when-not (str/blank? (:description pr)) + [:> clamped-text* {:text (:description pr)}])] + ;; the entry opens the review on click and on Enter: neither the + ;; menu, its trigger nor its items (portaled, but still in this + ;; React tree) may reach it + [:div {:class (stl/css :entry-aside) + :on-click dom/stop-propagation + :on-key-down dom/stop-propagation} + [:> menu* {:is-open menu-open? + :on-open-change on-menu-change + :on-action on-menu-action + :placement "bottom end" + :trigger + (mf/html + [:> icon-button* {:variant "ghost" + :icon i/menu + :aria-label (tr "labels.options") + :aria-pressed menu-open? + :on-click on-toggle-menu}])} + [:> menu-item* {:id "details"} + (tr "workspace.pull-requests.menu.review-details")] + (when can-reopen? + [:> menu-item* {:id "reopen"} + (tr "workspace.pull-requests.actions.reopen")])]]])) + +;; --- Dialog + +(mf/defc list-state* + "What a tab shows while its list is not there: loading, a failure with + a retry, or an empty list." + {::mf/private true} + [{:keys [status icon empty-text on-retry]}] + (case status + :loading + [:div {:class (stl/css :reviews-empty) :role "status"} + [:> empty-state* {:icon i/git-pull-request-arrow + :text (tr "workspace.branches.reviews.loading")}]] + + :error + [:div {:class (stl/css :reviews-empty) :role "alert"} + [:> empty-state* {:icon i/triangle-alert + :text (tr "workspace.branches.reviews.error")}] + [:> button* {:variant "secondary" + :icon i/reload + :on-click on-retry} + (tr "labels.retry")]] + + [:div {:class (stl/css :reviews-empty)} + [:> empty-state* {:icon icon :text empty-text}]])) + +(mf/defc reviews-dialog* + {::mf/register modal/components + ::mf/register-as :branch-reviews} + [] + (let [merges-state (mf/deref merges) + prs-state (mf/deref refs/pull-requests) + profiles (mf/deref refs/profiles) + prs-enabled? (dwpr/enabled?) + + tab* (mf/use-state "merges") + tab (deref tab*) + filter* (mf/use-state "") + filter-v (deref filter*) + + all-merges (:data merges-state) + all-prs (when prs-enabled? (:data prs-state)) + + ;; the search reads what each entry shows: the branch, the + ;; people and the texts + shown-merges + (mf/with-memo [all-merges filter-v profiles] + (filterv #(matches? filter-v [(:branch-name %) + (:comment %) + (:pull-request-title %) + (get-in profiles [(:merged-by %) :fullname])]) + all-merges)) + + shown-prs + (mf/with-memo [all-prs filter-v profiles] + (filterv #(matches? filter-v [(:title %) + (:description %) + (:branch-name %) + (get-in profiles [(:created-by %) :fullname])]) + all-prs)) + + tabs + (mf/with-memo [all-merges all-prs prs-enabled?] + (cond-> [{:id "merges" + :label (dm/str (tr "workspace.branches.reviews.tab.merges") + " (" (count all-merges) ")")}] + prs-enabled? + (conj {:id "design" + :label (dm/str (tr "workspace.branches.reviews.tab.design") + " (" (count all-prs) ")")}))) + + on-tab-change (mf/use-fn #(reset! tab* %)) + on-filter-change (mf/use-fn #(reset! filter* (dom/get-target-val %))) + + ;; the input's wrapper takes the full width it is given: this + ;; box sets that width, so the tabs keep room for their labels + search + (mf/html + [:div {:class (stl/css :reviews-search)} + [:> input* {:variant "dense" + :icon i/search + :placeholder (tr "workspace.branches.reviews.search") + :value filter-v + :on-change on-filter-change}]]) + + on-open-change + (mf/use-fn + (fn [open?] + (when-not open? + (st/emit! (modal/hide))))) + + on-retry-merges + (mf/use-fn #(st/emit! (dwb/fetch-branch-merges)))] + + ;; before paint, so a list from an earlier opening never shows in + ;; place of the loading state + (mf/with-layout-effect [] + (st/emit! (dwb/fetch-branch-merges)) + (when (dwpr/enabled?) + (st/emit! (dwpr/fetch-pull-requests)))) + + [:> modal* {:is-open true + :on-open-change on-open-change + :size "large"} + [:> modal-header* {:title (tr "workspace.branches.reviews.title")}] + [:> modal-content* {:class (stl/css :reviews-content)} + [:> tab-switcher* {:tabs tabs + :selected tab + :on-change on-tab-change + :scrollable-panel true + :action-button-position "end" + :action-button search + :class (stl/css :reviews-tabs)} + (if (= tab "design") + (cond + (= :loading (:status prs-state)) + [:> list-state* {:status :loading}] + + (empty? shown-prs) + [:> list-state* {:icon i/git-pull-request-arrow + :empty-text (if (seq all-prs) + (tr "workspace.branches.reviews.no-results") + (tr "workspace.branches.reviews.design.empty"))}] + + :else + [:ul {:class (stl/css :entries)} + (for [pr shown-prs] + [:> design-review-entry* {:key (dm/str (:id pr)) + :pr pr + :profiles profiles}])]) + + (cond + (contains? #{:loading :error} (:status merges-state)) + [:> list-state* {:status (:status merges-state) + :on-retry on-retry-merges}] + + (empty? shown-merges) + [:> list-state* {:icon i/git-merge + :empty-text (if (seq all-merges) + (tr "workspace.branches.reviews.no-results") + (tr "workspace.branches.reviews.merges.empty"))}] + + :else + [:ul {:class (stl/css :entries)} + (for [merge shown-merges] + [:> merge-entry* {:key (dm/str (:id merge)) + :merge merge + :profiles profiles}])]))]]])) diff --git a/frontend/src/app/main/ui/workspace/sidebar/reviews.scss b/frontend/src/app/main/ui/workspace/sidebar/reviews.scss new file mode 100644 index 0000000000..63de4b3bb7 --- /dev/null +++ b/frontend/src/app/main/ui/workspace/sidebar/reviews.scss @@ -0,0 +1,240 @@ +// 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/. +// +// Copyright (c) KALEIDOS INC Sucursal en España SL + +@use "ds/_borders.scss" as *; +@use "ds/_sizes.scss" as *; +@use "ds/mixins.scss" as *; +@use "ds/typography.scss" as *; + +// --- Dialog: the layout of the all-branches dialog + +.reviews-content { + display: flex; + flex-direction: column; + gap: var(--sp-m); + block-size: $sz-480; + min-block-size: 0; + padding: 0; +} + +.reviews-tabs { + flex: 1; + min-block-size: 0; +} + +.reviews-search { + inline-size: $sz-192; +} + +.reviews-empty { + display: flex; + flex-direction: column; + align-items: center; + gap: var(--sp-xxxl); + margin: var(--sp-xxxl) auto; + inline-size: $sz-200; +} + +// --- Entries + +.entries { + display: flex; + flex-direction: column; + gap: var(--sp-xs); + margin: 0; + padding: var(--sp-m) 0 0; + list-style: none; +} + +.entry { + // the card of the comment or the description: a step lighter than the + // row, also while a clickable row is hovered + --entry-card-bg-color: var(--color-background-tertiary); + + display: grid; + grid-template-columns: auto 1fr auto; + align-items: start; + gap: var(--sp-m); + padding: var(--sp-m) var(--sp-s); + border-radius: $br-8; + + &.is-clickable { + cursor: pointer; + + &:hover { + --entry-card-bg-color: var(--color-background-quaternary); + + background-color: var(--color-background-tertiary); + } + + &:focus-visible { + outline: $b-1 solid var(--color-accent-primary); + } + } +} + +.entry-avatar { + flex-shrink: 0; +} + +// a profile that left the team: a neutral tile the size of the avatar +.entry-avatar-unknown { + display: flex; + align-items: center; + justify-content: center; + inline-size: $sz-32; + block-size: $sz-32; + border-radius: $br-circle; + background-color: var(--color-background-quaternary); + color: var(--color-foreground-secondary); +} + +.entry-body { + display: flex; + flex-direction: column; + gap: var(--sp-xs); + min-inline-size: 0; +} + +.entry-head { + display: flex; + align-items: center; + gap: var(--sp-s); + min-inline-size: 0; +} + +.entry-title { + @include use-typography("body-medium"); + + min-inline-size: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; + color: var(--color-foreground-primary); +} + +.entry-date { + @include use-typography("body-small"); + + flex-shrink: 0; + margin-inline-start: auto; + color: var(--color-foreground-secondary); +} + +.entry-meta { + display: flex; + flex-wrap: wrap; + align-items: center; + gap: var(--sp-s); + min-inline-size: 0; +} + +.entry-meta-text { + @include use-typography("body-small"); + + color: var(--color-foreground-secondary); +} + +.entry-route { + display: flex; + align-items: center; + gap: var(--sp-xs); + min-inline-size: 0; +} + +.entry-route-arrow { + flex-shrink: 0; + color: var(--color-foreground-secondary); +} + +.entry-review { + @include use-typography("body-small"); + + display: flex; + align-items: center; + gap: var(--sp-xs); + min-inline-size: 0; + color: var(--color-foreground-secondary); +} + +.entry-review-title { + min-inline-size: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; +} + +.entry-approvals { + @include use-typography("body-small"); + + display: inline-flex; + align-items: center; + gap: var(--sp-xxs); + color: var(--color-foreground-secondary); +} + +// the empty card of the branch info dialog's description +.entry-no-comment { + @include use-typography("body-medium"); + + margin: 0; + margin-block-start: var(--sp-xs); + padding: var(--sp-s); + border: $b-1 dashed var(--color-background-quaternary); + border-radius: $br-8; + font-style: italic; + color: var(--color-foreground-secondary); +} + +.entry-aside { + display: flex; + align-items: center; +} + +// --- A long text folded to two lines, in the card of the branch info +// dialog's description + +.clamped { + display: flex; + flex-direction: column; + align-items: flex-start; + gap: var(--sp-xs); + margin-block-start: var(--sp-xs); + padding: var(--sp-s); + border-radius: $br-8; + background-color: var(--entry-card-bg-color); +} + +.clamped-text { + @include use-typography("body-medium"); + + margin: 0; + color: var(--color-foreground-primary); + white-space: pre-line; + overflow-wrap: anywhere; +} + +.is-folded { + @include two-line-text-ellipsis; +} + +.see-more { + @include use-typography("body-small"); + + padding: 0; + border: none; + background: none; + color: var(--color-accent-primary); + cursor: pointer; + + &:hover { + text-decoration: underline; + } + + &:focus-visible { + outline: $b-1 solid var(--color-accent-primary); + } +} diff --git a/frontend/test/frontend_tests/data/workspace_branches_test.cljs b/frontend/test/frontend_tests/data/workspace_branches_test.cljs index bedb5b630b..bd5d712932 100644 --- a/frontend/test/frontend_tests/data/workspace_branches_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_branches_test.cljs @@ -223,6 +223,29 @@ (done')) done)))) +(t/deftest the-comment-travels-with-the-merge-and-its-conflicts + (t/async done + (let [calls (atom []) + conflict-id (uuid/next)] + (mock/with-mocks + {rp/cmd! (cmd-stub calls conflict-id)} + (fn [done'] + (let [branch {:id (uuid/next)} + store (make-store)] + (ptk/emit! store (dwb/merge-branch branch {:comment " Ships the header "})) + (t/is (= "Ships the header" (:comment (last-params calls :merge))) + "the comment is trimmed") + (t/testing "the conflict answer hands the comment to the modal" + (t/is (= :branch-conflicts (get-in @store [::modal/modal :type]))) + (t/is (= " Ships the header " + (get-in @store [::modal/modal :props :comment])))) + (t/testing "a blank comment sends no `:comment` at all" + (ptk/emit! store (dwb/merge-branch branch {:comment " "})) + (t/is (not (contains? (last-params calls :merge) :comment)))) + (rx/dispose! store)) + (done')) + done)))) + ;;; --- Branch switcher: the entry point of the menu and the shortcut --- (defn- make-store-with @@ -310,9 +333,10 @@ (t/is (true? (dwb/has-branches? @store)) "a count that failed never hides the switcher") (ptk/emit! store (dwb/show-branches)) - (await (async/wait-for #(true? (:workspace-branch-switcher @store)) - "the shortcut opens the switcher, which retries")) - (t/is (nil? (::modal/modal @store)) "no create dialog on an unknown count") + (await (async/wait-for #(some? (::modal/modal @store)) + "the shortcut opens the branches dialog, which retries")) + (t/is (= :branches-list (get-in @store [::modal/modal :type])) + "no create dialog on an unknown count") (rx/dispose! store))))) (t/deftest ^:async a-listing-after-a-failed-count-settles-it @@ -375,15 +399,15 @@ (t/is (true? (:workspace-branch-switcher @store)) "forcing it open twice keeps it open") (rx/dispose! store))) -(t/deftest ^:async show-branches-opens-the-switcher-when-there-are-branches +(t/deftest ^:async show-branches-opens-the-branches-dialog-when-there-are-branches (await (mock/with-mocks* {cf/flags (conj cf/flags :branching)} - (let [store (make-store-with main-with-branches)] + (let [store (make-store-with (assoc main-with-branches :workspace-branch-switcher true))] (ptk/emit! store (dwb/show-branches)) - (await (async/wait-for #(true? (:workspace-branch-switcher @store)) - "the switcher opens")) - (await (async/settle)) - (t/is (nil? (::modal/modal @store)) "no dialog opens") + (await (async/wait-for #(some? (::modal/modal @store)) + "the branches dialog opens")) + (t/is (= :branches-list (get-in @store [::modal/modal :type]))) + (t/is (false? (:workspace-branch-switcher @store)) "the switcher closes") (rx/dispose! store))))) (t/deftest ^:async show-branches-asks-for-the-first-branch-when-there-are-none @@ -394,7 +418,6 @@ (await (async/wait-for #(some? (::modal/modal @store)) "the create dialog opens")) (t/is (= :create-branch (get-in @store [::modal/modal :type]))) - (t/is (= "Checkout" (get-in @store [::modal/modal :props :file-name]))) (t/is (not (:workspace-branch-switcher @store))) (rx/dispose! store))))) @@ -449,3 +472,31 @@ (t/is (= [{:file-id branch-id}] @navigations) "it opens the branch that was created") (rx/dispose! store)))))) + +;;; --- Reviews dialog: the merges into main --- + +(t/deftest ^:async the-merges-load-for-the-open-file + (let [calls (atom []) + merges [{:id (uuid/next) :branch-name "header" :comment "Ships it"}]] + (await (mock/with-mocks* + {rp/cmd! (mock/stub (fn [cmd params] + (swap! calls conj [cmd params]) + (rx/of merges)))} + (let [store (make-store-with main-without-branches)] + (ptk/emit! store (dwb/fetch-branch-merges)) + (await (async/wait-for #(= :loaded (get-in @store [:workspace-branch-merges :status])) + "the merges land")) + (t/is (= merges (get-in @store [:workspace-branch-merges :data]))) + (t/is (= [[:get-file-branch-merges {:file-id file-id}]] @calls) + "the open file is asked as is: the command resolves a branch to main") + (rx/dispose! store)))))) + +(t/deftest ^:async a-failed-merges-load-is-an-error-not-an-empty-list + (await (mock/with-mocks* + {rp/cmd! (mock/stub (fn [_ _] (rx/throw (ex-info "network" {}))))} + (let [store (make-store-with main-without-branches)] + (ptk/emit! store (dwb/fetch-branch-merges)) + (await (async/wait-for #(= :error (get-in @store [:workspace-branch-merges :status])) + "the failure lands")) + (t/is (nil? (get-in @store [:workspace-branch-merges :data]))) + (rx/dispose! store))))) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 841378027e..1f3a8077bb 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -6355,6 +6355,14 @@ msgstr "Text Transform" msgid "workspace.assets.ungroup" msgstr "Ungroup" +msgid "workspace.branches.archive.message" +msgstr "" +"Archive branch \"%s\"? It becomes read-only and leaves the active list. You " +"can restore it later." + +msgid "workspace.branches.archive.title" +msgstr "Archive branch" + #: src/app/main/ui/workspace/sidebar/branches.cljs:836 msgid "workspace.branches.archived" msgstr "Archived" @@ -6909,10 +6917,6 @@ msgstr "e.g. redesign-checkout" msgid "workspace.branches.create.submit" msgstr "Create branch" -#: src/app/main/ui/workspace/sidebar/branches.cljs:276 -msgid "workspace.branches.create.subtitle" -msgstr "Derived from main · %s" - #: src/app/main/data/workspace/branches.cljs:201 msgid "workspace.branches.create.success" msgstr "Branch \"%s\" created" @@ -7171,14 +7175,13 @@ msgstr "Main" msgid "workspace.branches.main-advanced" msgstr "Main has new changes. You can update this branch" +msgid "workspace.branches.main-library-note" +msgstr "Read by library consumers" + #: src/app/main/ui/workspace/sidebar/branches.cljs:659 msgid "workspace.branches.main-subtitle" msgstr "Default branch" -#: src/app/main/ui/workspace/branch_switcher.cljs:88 -msgid "workspace.branches.main-subtitle-library" -msgstr "Default branch (read by library consumers)" - #: src/app/main/ui/workspace/sidebar/branches.cljs:779 msgid "workspace.branches.materialize.accept" msgstr "Turn it into a file" @@ -7225,15 +7228,8 @@ msgstr "Restore" msgid "workspace.branches.merge.action" msgstr "Merge into main" -#: src/app/main/ui/workspace/sidebar/branches.cljs:370, src/app/main/ui/workspace/sidebar/branches.cljs:1526 -msgid "workspace.branches.merge.archive-label" -msgstr "Archive this branch" - -#: src/app/main/ui/workspace/sidebar/branches.cljs:376 -msgid "workspace.branches.merge.archive-warning" -msgstr "" -"Archived branches keep a full copy of the file so they can be restored " -"later. Keeping many archived branches can take up a lot of storage." +msgid "workspace.branches.merge.comment-label" +msgstr "Merge comment" #: src/app/main/ui/workspace/sidebar/branches.cljs:386 msgid "workspace.branches.merge.confirm-accept" @@ -7256,9 +7252,7 @@ msgstr "This branch has conflicts that must be resolved before merging" #: src/app/main/ui/workspace/sidebar/branches.cljs:378 msgid "workspace.branches.merge.delete-info" -msgstr "" -"After merging, this branch and its file will be deleted to free up storage. " -"Archive it if you might need to restore it later." +msgstr "After merging, this branch and its file will be deleted to free up storage." #: src/app/main/data/workspace/branches.cljs:451, src/app/main/data/workspace/branches.cljs:459 msgid "workspace.branches.merge.error" @@ -7284,14 +7278,66 @@ msgstr "" "This branch changes elements that can't be merged automatically yet " "(components, pages or tokens)" -#: src/app/main/ui/workspace/branch_switcher.cljs:208, src/app/main/ui/workspace/sidebar/branches.cljs:943 -msgid "workspace.branches.merged" -msgstr "Merged" - #, unused msgid "workspace.branches.new" msgstr "New" +msgid "workspace.branches.rename.title" +msgstr "Rename branch" + +msgid "workspace.branches.restore.message" +msgstr "" +"Restore branch \"%s\"? It goes back to the active list and you can edit it " +"again." + +msgid "workspace.branches.restore.title" +msgstr "Restore branch" + +msgid "workspace.branches.reviews.design.empty" +msgstr "No design reviews yet" + +msgid "workspace.branches.reviews.error" +msgstr "Couldn't load the merges" + +msgid "workspace.branches.reviews.from-review" +msgstr "From the design review \"%s\"" + +msgid "workspace.branches.reviews.loading" +msgstr "Loading reviews…" + +msgid "workspace.branches.reviews.merged" +msgstr "Merged" + +msgid "workspace.branches.reviews.merges.empty" +msgstr "No branch has been merged into main yet" + +msgid "workspace.branches.reviews.no-comment" +msgstr "No comment" + +msgid "workspace.branches.reviews.no-results" +msgstr "No reviews match your search" + +msgid "workspace.branches.reviews.search" +msgstr "Search review…" + +msgid "workspace.branches.reviews.see-less" +msgstr "See less" + +msgid "workspace.branches.reviews.see-more" +msgstr "See more" + +msgid "workspace.branches.reviews.tab.design" +msgstr "Design reviews" + +msgid "workspace.branches.reviews.tab.merges" +msgstr "Merge reviews" + +msgid "workspace.branches.reviews.title" +msgstr "Reviews" + +msgid "workspace.branches.reviews.unknown-author" +msgstr "Unknown user" + #: src/app/main/ui/workspace/sidebar/branches.cljs:787 msgid "workspace.branches.search.placeholder" msgstr "Search branch…" @@ -7332,6 +7378,9 @@ msgstr "New branch" msgid "workspace.branches.switcher.new-from-main" msgstr "Branches are created from main" +msgid "workspace.branches.switcher.reviews" +msgstr "Reviews" + #: src/app/main/ui/workspace/branch_switcher.cljs:309 msgid "workspace.branches.switcher.tooltip" msgstr "Switch branch" @@ -9369,7 +9418,7 @@ msgstr "TRY PLUGIN" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:426, src/app/main/ui/workspace/sidebar/pull_requests.cljs:505, src/app/main/ui/workspace/sidebar/pull_requests.cljs:564 msgid "workspace.pull-requests.actions.close" -msgstr "Cancel pull request" +msgstr "Cancel design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:440, src/app/main/ui/workspace/sidebar/pull_requests.cljs:561, src/app/main/ui/workspace/sidebar/pull_requests.cljs:754 msgid "workspace.pull-requests.actions.merge" @@ -9381,7 +9430,7 @@ msgstr "Open review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:430, src/app/main/ui/workspace/sidebar/pull_requests.cljs:567 msgid "workspace.pull-requests.actions.reopen" -msgstr "Reopen pull request" +msgstr "Reopen design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:359, src/app/main/ui/workspace/sidebar/pull_requests.cljs:537, src/app/main/ui/workspace/sidebar/pull_requests.cljs:722 msgid "workspace.pull-requests.badge.conflicts" @@ -9409,7 +9458,7 @@ msgstr "Review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:710 msgid "workspace.pull-requests.banner.reviewing" -msgstr "Reviewing pull request" +msgstr "Reviewing the design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:504 msgid "workspace.pull-requests.close.message" @@ -9419,11 +9468,11 @@ msgstr "" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:503 msgid "workspace.pull-requests.close.title" -msgstr "Cancel pull request" +msgstr "Cancel design review" #: src/app/main/data/workspace/pull_requests.cljs:142, src/app/main/data/workspace/pull_requests.cljs:255 msgid "workspace.pull-requests.create.already-exists" -msgstr "This branch already has an open pull request" +msgstr "This branch already has an open design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:167 msgid "workspace.pull-requests.create.description-label" @@ -9435,7 +9484,7 @@ msgstr "What changed, and what needs review or validation?" #: src/app/main/data/workspace/pull_requests.cljs:143 msgid "workspace.pull-requests.create.error" -msgstr "An error occurred while creating the pull request" +msgstr "An error occurred while creating the design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:193 msgid "workspace.pull-requests.create.info" @@ -9462,7 +9511,7 @@ msgstr "Reviewers" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:202 msgid "workspace.pull-requests.create.submit" -msgstr "Create pull request" +msgstr "Create design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:152 msgid "workspace.pull-requests.create.subtitle" @@ -9470,15 +9519,15 @@ msgstr "Request a review of the changes on \"%s\"" #: src/app/main/data/workspace/pull_requests.cljs:137 msgid "workspace.pull-requests.create.success" -msgstr "Pull request \"%s\" created" +msgstr "Design review \"%s\" created" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:150 msgid "workspace.pull-requests.create.title" -msgstr "Create pull request" +msgstr "Create design review" #: src/app/main/ui/workspace/left_header.cljs:163 msgid "workspace.pull-requests.header-badge-tooltip" -msgstr "Reviewing pull request: %s" +msgstr "Reviewing the design review: %s" #: src/app/main/ui/workspace/left_header.cljs:177 msgid "workspace.pull-requests.header-label" @@ -9514,19 +9563,19 @@ msgstr "Review updated" #: src/app/main/data/workspace/pull_requests.cljs:252 msgid "workspace.pull-requests.lifecycle.branch-gone" -msgstr "The pull request's branch is no longer open" +msgstr "The design review's branch is no longer open" #: src/app/main/data/workspace/pull_requests.cljs:230 msgid "workspace.pull-requests.lifecycle.closed" -msgstr "Pull request closed" +msgstr "Design review closed" #: src/app/main/data/workspace/pull_requests.cljs:160, src/app/main/data/workspace/pull_requests.cljs:187, src/app/main/data/workspace/pull_requests.cljs:213, src/app/main/data/workspace/pull_requests.cljs:237, src/app/main/data/workspace/pull_requests.cljs:257 msgid "workspace.pull-requests.lifecycle.error" -msgstr "An error occurred while updating the pull request" +msgstr "An error occurred while updating the design review" #: src/app/main/data/workspace/pull_requests.cljs:246 msgid "workspace.pull-requests.lifecycle.reopened" -msgstr "Pull request reopened" +msgstr "Design review reopened" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:555, src/app/main/ui/workspace/sidebar/pull_requests.cljs:731 msgid "workspace.pull-requests.menu.info" @@ -9550,7 +9599,7 @@ msgstr "New" #: src/app/main/data/workspace/pull_requests.cljs:396 msgid "workspace.pull-requests.preview.unavailable" -msgstr "The review sandbox is not available (the pull request may be closed)" +msgstr "The review sandbox is not available (the design review may be closed)" #: src/app/main/data/workspace/pull_requests.cljs:182 msgid "workspace.pull-requests.publish.success" @@ -9618,7 +9667,7 @@ msgstr "Submit review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:632 msgid "workspace.pull-requests.section.empty" -msgstr "No pull requests yet. Create one from a branch to request a review." +msgstr "No design reviews yet. Create one from a branch to request a review." #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:639 msgid "workspace.pull-requests.section.history" @@ -9626,7 +9675,7 @@ msgstr "Finished" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:611 msgid "workspace.pull-requests.section.title" -msgstr "Pull requests" +msgstr "Design reviews" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:100 #, unused From 59d5817d1be13805597cdd7fadb399480967d696 Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Fri, 23 Oct 2026 13:12:11 +0200 Subject: [PATCH 2/2] :bug: Give branch access through the team role alone A branch must be open to every member of the team of its source file, by role: owners, admins and editors open and edit it, viewers read it. Two things got in the way. Branch creation granted its creator the owner role on the branch file (a file_profile_rel row), so a team editor became owner and admin of their own branch. create-file-branch grants no file role any more: a branch takes its permissions from the team and project roles alone. materialize-file-branch, which turns a branch into an ordinary file, now grants the owner role to the profile that runs it, as creating a file does. Migration 0807 deletes the file roles of existing branches; materialised files are no longer branches and keep theirs. move-files moved a file but left its branches in the old project. The permissions of a branch come from the project it lives in, so after a move to another team its members got not-found and the workspace asked them to request access. move-files now moves the branches of every moved file with it, deleted branch rows included since their files live on until the GC, and the broken library links cleanup already covers them. It refuses to move a branch file on its own (:cant-move-a-branch), which would part it from its source. Migration 0808 puts every branch back in the project of its source file and drops the links of those branches to libraries of their old team. Assigning a reviewer to a design review grants no access: the email and the dashboard notification only let the reviewer know. Tests cover a branch created with no file role, the creator keeping editor rights, another editor saving to the branch, a viewer reading it, the owner role granted on materialise, a lone branch refused by move-files and an editor of the new team opening a moved branch. --- backend/src/app/migrations.clj | 8 +- .../sql/0807-del-file-branch-profile-rels.sql | 11 +++ .../sql/0808-mod-file-branch-project.sql | 22 +++++ backend/src/app/rpc/commands/files_branch.clj | 26 +++--- backend/src/app/rpc/commands/management.clj | 22 ++++- .../backend_tests/rpc_file_branch_test.clj | 93 +++++++++++++++++++ 6 files changed, 164 insertions(+), 18 deletions(-) create mode 100644 backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql create mode 100644 backend/src/app/migrations/sql/0808-mod-file-branch-project.sql diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index ddb2502c23..2dcdf803b7 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -545,7 +545,13 @@ :fn mg0805/migrate} {:name "0806-add-file-branch-merge-table" - :fn (mg/resource "app/migrations/sql/0806-add-file-branch-merge-table.sql")}]) + :fn (mg/resource "app/migrations/sql/0806-add-file-branch-merge-table.sql")} + + {:name "0807-del-file-branch-profile-rels" + :fn (mg/resource "app/migrations/sql/0807-del-file-branch-profile-rels.sql")} + + {:name "0808-mod-file-branch-project" + :fn (mg/resource "app/migrations/sql/0808-mod-file-branch-project.sql")}]) (defn apply-migrations! [pool name migrations] diff --git a/backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql b/backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql new file mode 100644 index 0000000000..6be81c66e3 --- /dev/null +++ b/backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql @@ -0,0 +1,11 @@ +-- A branch takes its permissions from the team and project roles alone: +-- every editor of main can open and edit it. Branch creation used to +-- grant its creator the owner role on the branch file, which lifted a +-- team editor to owner of that branch. Drop those grants. A branch that +-- was materialised is an ordinary file now (`is_branch` false) and +-- keeps its roles. + +DELETE FROM file_profile_rel AS fpr + USING file AS f + WHERE f.id = fpr.file_id + AND f.is_branch IS TRUE; diff --git a/backend/src/app/migrations/sql/0808-mod-file-branch-project.sql b/backend/src/app/migrations/sql/0808-mod-file-branch-project.sql new file mode 100644 index 0000000000..3ddd5546d8 --- /dev/null +++ b/backend/src/app/migrations/sql/0808-mod-file-branch-project.sql @@ -0,0 +1,22 @@ +-- A branch lives in the project of its source file: its permissions come +-- from that project and its team. Moving a file used to leave its +-- branches behind in the old project, so the members of the new team +-- could not open them. Put every branch back next to its source file. + +UPDATE file AS bf + SET project_id = sf.project_id + FROM file_branch AS fb + JOIN file AS sf ON (sf.id = fb.source_file_id) + WHERE bf.id = fb.branch_file_id + AND bf.project_id <> sf.project_id; + +-- A moved branch may link libraries of its old team: drop those links, +-- as moving a file does. +DELETE FROM file_library_rel AS rel + USING file AS f, project AS p, file AS lf, project AS lp + WHERE rel.file_id = f.id + AND f.is_branch IS TRUE + AND p.id = f.project_id + AND lf.id = rel.library_file_id + AND lp.id = lf.project_id + AND lp.team_id <> p.team_id; diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index 8dfd51f4c2..8bca07333f 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -42,6 +42,7 @@ [app.rpc.climit :as-alias climit] [app.rpc.commands.files :as files] [app.rpc.commands.files-branch-policies :as bp] + [app.rpc.commands.files-create :as fcreate] [app.rpc.commands.files-update :as fupd] [app.rpc.commands.teams :as teams] [app.rpc.doc :as-alias doc] @@ -535,23 +536,16 @@ ;; 3. Mark the new file row as a branch so it is ;; hidden from the project/team file listings. + ;; No file role is granted: a branch takes its + ;; permissions from the team (and project) roles + ;; alone, so every editor of main can open and edit + ;; it, and its creator gets no extra rights. (db/update! conn :file {:is-branch true} {:id branch-id} {::db/return-keys false}) - ;; 4. Grant the creator ownership (same grant - ;; duplicate-file used to make). - (when (uuid? profile-id) - (db/insert! conn :file-profile-rel - {:file-id branch-id - :profile-id profile-id - :is-owner true - :is-admin true - :can-edit true} - {::db/return-keys false})) - - ;; 5. The branch resolves libraries exactly like + ;; 4. The branch resolves libraries exactly like ;; main (component instances from shared libraries ;; must keep resolving on the derived state). (doseq [rel (bfc/get-files-rels cfg #{file-id})] @@ -565,7 +559,7 @@ :library-file-id (:library-file-id rel) :synced-at (or (:synced-at rel) ts)}))) - ;; 6. Persist the branch metadata. `base-revn` + ;; 5. Persist the branch metadata. `base-revn` ;; tracks MAIN's revision counter and ;; `base-branch-revn` the BRANCH file's one. Each ;; starts at its own side's revn (main's current @@ -1711,6 +1705,12 @@ {:id branch-file-id} {::db/return-keys false}) + ;; a branch carries no file roles; as an ordinary file it + ;; gets the owner role its maker would get on creation + (fcreate/create-file-role! conn {:file-id branch-file-id + :profile-id profile-id + :role :owner}) + ;; the payload replaced the log (db/delete! conn :file-branch-change {:branch-id (:id branch)}) diff --git a/backend/src/app/rpc/commands/management.clj b/backend/src/app/rpc/commands/management.clj index 62d5a88330..378bf88f89 100644 --- a/backend/src/app/rpc/commands/management.clj +++ b/backend/src/app/rpc/commands/management.clj @@ -254,10 +254,18 @@ ;; --- COMMAND: Move file (def sql:get-files - "select id, features, project_id from file where id = ANY(?)") + "select id, features, project_id, is_branch from file where id = ANY(?)") +;; A branch lives in the project of its source file: its permissions +;; come from that project and its team, so the branches of a moved file +;; move with it. The deleted branch rows are included, their files still +;; exist until the GC takes them. (def sql:move-files - "update file set project_id = ? where id = ANY(?)") + "update file set project_id = ? + where id = ANY(?) + or id in (select fb.branch_file_id + from file_branch as fb + where fb.source_file_id = ANY(?))") (def sql:delete-broken-relations "with broken as ( @@ -289,6 +297,12 @@ :code :cant-move-to-same-project :hint "Unable to move a file to the same project")) + ;; a branch only moves together with its source file + (when (some :is-branch files) + (ex/raise :type :validation + :code :cant-move-a-branch + :hint "Unable to move a branch file on its own")) + ;; Check if we have permissions on the destination project (proj/check-edition-permissions! conn profile-id project-id) @@ -306,8 +320,8 @@ (doseq [file files] (cfeat/check-file-features! features (:features file))))) - ;; move all files to the project - (db/exec-one! conn [sql:move-files project-id fids]) + ;; move all files, and their branches, to the project + (db/exec-one! conn [sql:move-files project-id fids fids]) ;; delete possible broken relations on moved files (db/exec-one! conn [sql:delete-broken-relations pids]) diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index fed5b53d70..7486638688 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -1362,6 +1362,94 @@ ;; the colour differ from the new base and lands in the op log (t/is (empty? (th/db-query :file-branch-change {:branch-id branch-id}))))))) +(t/deftest branch-permissions-come-from-the-team-role + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [owner (th/create-profile* 1 {:is-active true}) + editor1 (th/create-profile* 2 {:is-active true}) + editor2 (th/create-profile* 3 {:is-active true}) + viewer (th/create-profile* 4 {:is-active true}) + team-id (:default-team-id owner) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:default-project-id owner)}) + _ (doseq [[profile role] [[editor1 :editor] [editor2 :editor] [viewer :viewer]]] + (th/create-team-role* {:team-id team-id + :profile-id (:id profile) + :role role})) + create (:result (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id editor1) + :file-id (:id file) + :name "team-roles"})) + branch-file-id (:branch-file-id create) + perms-of (fn [profile] + (db/run! th/*system* + #(bfc/get-file-permissions % (:id profile) branch-file-id)))] + + (t/testing "creating a branch grants no file role" + (t/is (empty? (th/db-query :file-profile-rel {:file-id branch-file-id})))) + + (t/testing "the creator keeps the rights of a team editor" + (let [perms (perms-of editor1)] + (t/is (true? (:can-edit perms))) + (t/is (false? (:is-admin perms))) + (t/is (false? (:is-owner perms))))) + + (t/testing "another team editor can edit the branch" + (let [bf (th/db-get :file {:id branch-file-id}) + out (th/command! {::th/type :update-file + ::rpc/profile-id (:id editor2) + :id branch-file-id + :session-id (uuid/random) + :revn (:revn bf) + :vern (:vern bf) + :features cfeat/supported-features + :changes [{:type :add-page :id (uuid/random) :name "by-editor2"}]})] + (t/is (nil? (:error out))))) + + (t/testing "a team viewer can read the branch but not edit it" + (let [perms (perms-of viewer)] + (t/is (true? (:can-read perms))) + (t/is (false? (:can-edit perms)))))))) + +(t/deftest branches-move-with-their-source-file + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [owner (th/create-profile* 1 {:is-active true}) + editor (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 (:default-project-id owner)}) + _ (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + create (:result (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id owner) + :file-id (:id file) + :name "travels"})) + branch-file-id (:branch-file-id create)] + + (t/testing "a branch cannot be moved on its own" + (let [out (th/command! {::th/type :move-files + ::rpc/profile-id (:id owner) + :project-id (:id project) + :ids #{branch-file-id}})] + (t/is (th/ex-of-code? (:error out) :cant-move-a-branch)))) + + (t/testing "moving the source file takes its branches along" + (let [out (th/command! {::th/type :move-files + ::rpc/profile-id (:id owner) + :project-id (:id project) + :ids #{(:id file)}})] + (t/is (nil? (:error out)))) + (t/is (= (:id project) (:project-id (th/db-get :file {:id branch-file-id}))))) + + (t/testing "an editor of the new team can open the branch" + (let [out (th/command! {::th/type :get-file + ::rpc/profile-id (:id editor) + :id branch-file-id})] + (t/is (nil? (:error out))) + (t/is (true? (-> out :result :permissions :can-edit)))))))) + (t/deftest materialize-branch-into-an-ordinary-file (with-redefs [cf/flags (conj cf/flags :branching)] (let [profile (th/create-profile* 1 {:is-active true}) @@ -1407,6 +1495,11 @@ (let [row (th/db-get :file {:id branch-file-id})] (t/is (false? (:is-branch row)))) + ;; as an ordinary file it gets the owner role of its maker + (let [rel (th/db-get :file-profile-rel {:file-id branch-file-id + :profile-id (:id profile)})] + (t/is (true? (:is-owner rel)))) + ;; the payload lands where the ordinary write path puts it, which ;; is the `file_data` row of type "main" rather than the legacy ;; `file.data` column