diff --git a/backend/src/app/rpc/commands/files_update.clj b/backend/src/app/rpc/commands/files_update.clj index b7478a124d..06c630dfb3 100644 --- a/backend/src/app/rpc/commands/files_update.clj +++ b/backend/src/app/rpc/commands/files_update.clj @@ -429,6 +429,25 @@ :hint "branch metadata not found for branch file" :file-id (:id file))) + ;; Only an open branch takes saves. A branch kept after its merge has + ;; its work in main, and no command integrates ops appended after the + ;; merge. An archived branch is read-only until it is restored or + ;; turned into an ordinary file. Refuse instead of stranding the ops. + (case (:status branch) + "merged" + (ex/raise :type :validation + :code :branch-merged + :hint "a merged branch is read-only" + :file-id (:id file)) + + "archived" + (ex/raise :type :validation + :code :branch-archived + :hint "an archived branch is read-only" + :file-id (:id file)) + + nil) + (db/update! conn :project {:modified-at modified-at} {:id (:project-id file)} 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 d7996a0086..ae4b6d3a3a 100644 --- a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj @@ -211,6 +211,80 @@ (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 + :color {:id (uuid/random) :name "C" :color "#112233" :opacity 1}})) + +(defn- archived-branch* + "A profile and an archived branch of one of its files." + [name] + (let [profile (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + create (create-branch* profile (:id file) name)] + (th/command! {::th/type :archive-file-branch + ::rpc/profile-id (:id profile) + :id (:id create)}) + [profile create])) + +(t/deftest an-archived-branch-refuses-saves + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [[profile create] (archived-branch* "oplog-archived") + branch-file-id (:branch-file-id create) + out (add-color* profile branch-file-id)] + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :branch-archived)) + (t/is (empty? (oplog-rows branch-file-id)))))) + +(t/deftest a-restored-branch-takes-saves-again + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [[profile create] (archived-branch* "oplog-restored") + branch-file-id (:branch-file-id create)] + (th/command! {::th/type :archive-file-branch + ::rpc/profile-id (:id profile) + :id (:id create) + :archived false}) + (let [out (add-color* profile branch-file-id)] + (t/is (nil? (:error out))) + (t/is (= 1 (count (oplog-rows branch-file-id)))))))) + +(t/deftest an-archived-branch-turned-into-a-file-takes-saves + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [[profile create] (archived-branch* "oplog-materialized") + branch-file-id (:branch-file-id create) + out (th/command! {::th/type :materialize-file-branch + ::rpc/profile-id (:id profile) + :branch-file-id branch-file-id})] + (t/is (nil? (:error out))) + (t/is (nil? (:error (add-color* profile branch-file-id))))))) + (t/deftest update-from-main-compares-in-main-id-space ;; A branch stores no data: its document is the base snapshot plus the op ;; log, so every reference it inherited names MAIN. A local reference is a diff --git a/frontend/playwright/data/pull-requests/archive-file-branch-restore.json b/frontend/playwright/data/pull-requests/archive-file-branch-restore.json new file mode 100644 index 0000000000..0f173b4150 --- /dev/null +++ b/frontend/playwright/data/pull-requests/archive-file-branch-restore.json @@ -0,0 +1,4 @@ +{ + "~:id": "~uc7ce0794-0992-8105-8004-38f280443998", + "~:status": "open" +} diff --git a/frontend/playwright/data/pull-requests/get-file-branch-info-archived.json b/frontend/playwright/data/pull-requests/get-file-branch-info-archived.json new file mode 100644 index 0000000000..2a059deeec --- /dev/null +++ b/frontend/playwright/data/pull-requests/get-file-branch-info-archived.json @@ -0,0 +1,15 @@ +{ + "~:id": "~uc7ce0794-0992-8105-8004-38f280443998", + "~:branch-file-id": "~uc7ce0794-0992-8105-8004-38f280443849", + "~:source-file-id": "~uc7ce0794-0992-8105-8004-38f280443997", + "~:name": "parked", + "~:description": null, + "~:status": "archived", + "~:created-by": "~uc7ce0794-0992-8105-8004-38e630f29a9b", + "~:created-at": "~m1713536343369", + "~:updated-at": "~m1713873823633", + "~:source-name": "New File 1", + "~:ahead": 0, + "~:behind": 0, + "~:conflicts": 0 +} diff --git a/frontend/playwright/data/pull-requests/get-file-branch-info-merged.json b/frontend/playwright/data/pull-requests/get-file-branch-info-merged.json new file mode 100644 index 0000000000..0e135479ed --- /dev/null +++ b/frontend/playwright/data/pull-requests/get-file-branch-info-merged.json @@ -0,0 +1,16 @@ +{ + "~:id": "~uc7ce0794-0992-8105-8004-38f280443998", + "~:branch-file-id": "~uc7ce0794-0992-8105-8004-38f280443849", + "~:source-file-id": "~uc7ce0794-0992-8105-8004-38f280443997", + "~:name": "shipped", + "~:description": null, + "~:status": "merged", + "~:created-by": "~uc7ce0794-0992-8105-8004-38e630f29a9b", + "~:created-at": "~m1713536343369", + "~:updated-at": "~m1713873823633", + "~:source-name": "New File 1", + "~:ahead": 0, + "~:behind": 0, + "~:conflicts": 0, + "~:merged-at": "~m1713873823633" +} diff --git a/frontend/playwright/data/pull-requests/get-file-branches-closed.json b/frontend/playwright/data/pull-requests/get-file-branches-closed.json new file mode 100644 index 0000000000..66edafc77b --- /dev/null +++ b/frontend/playwright/data/pull-requests/get-file-branches-closed.json @@ -0,0 +1,32 @@ +[ + { + "~:id": "~uc7ce0794-0992-8105-8004-38f280443991", + "~:branch-file-id": "~uc7ce0794-0992-8105-8004-38f280443851", + "~:source-file-id": "~uc7ce0794-0992-8105-8004-38f280443997", + "~:name": "shipped", + "~:description": null, + "~:status": "merged", + "~:created-by": "~uc7ce0794-0992-8105-8004-38e630f29a9b", + "~:created-at": "~m1713536343369", + "~:updated-at": "~m1713873823633", + "~:source-name": "New File 1", + "~:ahead": 0, + "~:behind": 0, + "~:conflicts": 0 + }, + { + "~:id": "~uc7ce0794-0992-8105-8004-38f280443992", + "~:branch-file-id": "~uc7ce0794-0992-8105-8004-38f280443852", + "~:source-file-id": "~uc7ce0794-0992-8105-8004-38f280443997", + "~:name": "parked", + "~:description": null, + "~:status": "archived", + "~:created-by": "~uc7ce0794-0992-8105-8004-38e630f29a9b", + "~:created-at": "~m1713536343369", + "~:updated-at": "~m1713873823633", + "~:source-name": "New File 1", + "~:ahead": 0, + "~:behind": 0, + "~:conflicts": 0 + } +] diff --git a/frontend/playwright/ui/specs/merged-and-archived-branches.spec.js b/frontend/playwright/ui/specs/merged-and-archived-branches.spec.js new file mode 100644 index 0000000000..3255f85e35 --- /dev/null +++ b/frontend/playwright/ui/specs/merged-and-archived-branches.spec.js @@ -0,0 +1,152 @@ +import { test, expect } from "@playwright/test"; +import { WorkspacePage } from "../pages/WorkspacePage"; + +test.beforeEach(async ({ page }) => { + await WorkspacePage.init(page); + await WorkspacePage.mockConfigFlags(page, ["enable-branching"]); +}); + +const setupBranchFile = async (workspacePage, branchInfo) => { + await workspacePage.setupEmptyFile(); + await workspacePage.mockRPCs({ + "get-file-branch-info?file-id=*": `pull-requests/${branchInfo}`, + "get-file-branches?file-id=*": + "pull-requests/get-file-branches-closed.json", + "get-file-pull-requests?file-id=*": + "pull-requests/get-file-branches-empty.json", + "get-branch-diff?branch-id=*": "pull-requests/get-branch-diff-empty.json", + }); +}; + +test("a kept merged branch opens read-only", async ({ page }) => { + const workspacePage = new WorkspacePage(page); + await setupBranchFile(workspacePage, "get-file-branch-info-merged.json"); + await workspacePage.goToWorkspace(); + + const banner = page.locator("#branch-banner"); + await expect(banner.getByText("Merged branch")).toBeVisible(); + await expect( + banner.getByRole("button", { name: "View in main" }), + ).toBeVisible(); + await expect(banner.getByRole("button", { name: "Compare" })).toBeHidden(); + await expect(banner.getByRole("button", { name: "Restore" })).toBeHidden(); + + await expect(page.getByText("Merged into main, read only")).toBeVisible(); + await expect( + page.getByRole("toolbar", { name: "Drawing tools" }), + ).toBeHidden(); +}); + +test("an archived branch opens read-only and offers its ways out", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupBranchFile(workspacePage, "get-file-branch-info-archived.json"); + await workspacePage.mockRPC( + "archive-file-branch", + "pull-requests/archive-file-branch-restore.json", + ); + await workspacePage.goToWorkspace(); + + const banner = page.locator("#branch-banner"); + await expect(banner.getByText("Archived branch")).toBeVisible(); + await expect(banner.getByText("Merged branch")).toBeHidden(); + await expect(banner.getByRole("button", { name: "Compare" })).toBeVisible(); + await expect( + banner.getByRole("button", { name: "Turn into an ordinary file" }), + ).toBeVisible(); + await expect(page.getByText("Archived, read only")).toBeVisible(); + await expect( + page.getByRole("toolbar", { name: "Drawing tools" }), + ).toBeHidden(); + + // restoring reopens the branch, and the banner follows its new status + await workspacePage.mockRPC( + "get-file-branch-info?file-id=*", + "pull-requests/get-file-branch-info-branch.json", + ); + const restore = page.waitForRequest( + (req) => + req.url().includes("archive-file-branch") && + /"~:archived":\s*false/.test(req.postData() ?? ""), + ); + await banner.getByRole("button", { name: "Restore" }).click(); + await restore; + await expect(banner.getByText("1 change not in main")).toBeVisible(); + await expect( + page.getByRole("toolbar", { name: "Drawing tools" }), + ).toBeVisible(); +}); + +test("an archived branch asks before turning into an ordinary file", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupBranchFile(workspacePage, "get-file-branch-info-archived.json"); + await workspacePage.goToWorkspace(); + + const banner = page.locator("#branch-banner"); + await banner + .getByRole("button", { name: "Turn into an ordinary file" }) + .click(); + await expect( + page.getByText("Turn this branch into an ordinary file"), + ).toBeVisible(); +}); + +test("the switcher groups merged and archived branches apart", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupBranchFile(workspacePage, "get-file-branch-info-none.json"); + await workspacePage.goToWorkspace(); + + await page.getByTitle("Switch branch").click(); + const popover = page.getByRole("dialog", { name: "Branches" }); + + await popover.getByRole("button", { name: /^Merged/ }).click(); + await expect(popover.getByText("shipped")).toBeVisible(); + await expect(popover.getByText("parked")).toBeHidden(); + + await popover.getByRole("button", { name: /^Archived/ }).click(); + await expect(popover.getByText("parked")).toBeVisible(); + await expect(popover.getByText("Archived, read only")).toBeVisible(); +}); + +test("the branch list offers Restore for archived branches only", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupBranchFile(workspacePage, "get-file-branch-info-none.json"); + await workspacePage.goToWorkspace(); + + await page.getByTitle("Switch branch").click(); + await page.getByText("All branches").click(); + + await page.getByRole("tab", { name: "Merged (1)" }).click(); + await page.getByRole("button", { name: "Options", exact: true }).click(); + await expect( + page.getByRole("menuitem", { name: "Branch info" }), + ).toBeVisible(); + await expect(page.getByRole("menuitem", { name: "Restore" })).toBeHidden(); + await expect(page.getByRole("menuitem", { name: "Compare" })).toBeHidden(); + await expect( + page.getByRole("menuitem", { name: "Turn into an ordinary file" }), + ).toBeHidden(); + await page.keyboard.press("Escape"); + + await page.getByRole("tab", { name: "Archived (1)" }).click(); + await page.getByRole("button", { name: "Options", exact: true }).click(); + await expect(page.getByRole("menuitem", { name: "Restore" })).toBeVisible(); + await expect( + page.getByRole("menuitem", { name: "Turn into an ordinary file" }), + ).toBeVisible(); + await page.getByRole("menuitem", { name: "Branch info" }).click(); + + const info = page.getByRole("dialog").filter({ hasText: "Branch info" }); + await expect(info.getByRole("button", { name: "Restore" })).toBeVisible(); + await expect(info.getByRole("button", { name: "Compare" })).toBeVisible(); + await expect( + info.getByRole("button", { name: "Turn into an ordinary file" }), + ).toBeVisible(); +}); diff --git a/frontend/src/app/main/data/workspace/branches.cljs b/frontend/src/app/main/data/workspace/branches.cljs index b501100e3f..be5396dc6f 100644 --- a/frontend/src/app/main/data/workspace/branches.cljs +++ b/frontend/src/app/main/data/workspace/branches.cljs @@ -241,6 +241,8 @@ (rx/catch (fn [cause] (rx/of (refusal-notification cause (tr "workspace.branches.lifecycle.error"))))))))) (defn archive-branch + "Archive a branch, or restore it with `archived?` false. The context + refreshes as well, so the banner of the open branch follows its status." ([id] (archive-branch id true)) ([id archived?] (assert (uuid? id) "expected valid uuid for `id`") @@ -248,7 +250,7 @@ ptk/WatchEvent (watch [_ _ _] (->> (rp/cmd! :archive-file-branch {:id id :archived archived?}) - (rx/mapcat (fn [_] (rx/of (fetch-branches)))) + (rx/mapcat (fn [_] (rx/of (fetch-branches) (fetch-branch-context)))) (rx/catch (fn [cause] (rx/of (refusal-notification cause (tr "workspace.branches.lifecycle.error")))))))))) (defn delete-branch diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 274af8cf65..2f578f77df 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -526,11 +526,22 @@ (defn flash-persistence [cause] (let [data (ex-data cause)] - (if (delegated-persistence-failure? data) + (cond + (delegated-persistence-failure? data) ;; The persistence state wraps the failure and records the original ;; type under :cause-type; dispatch on it to reach the cause's handler. (on-error (-> (exception->error-data cause) (assoc :type (or (:cause-type data) (:type data))))) + + ;; A merged or archived branch is read-only and the server refuses its + ;; saves: an expected refusal, so it is explained, not reported. + (= :branch-merged (:code data)) + (st/emit! (ntf/warn (tr "errors.save-refused-merged-branch"))) + + (= :branch-archived (:code data)) + (st/emit! (ntf/warn (tr "errors.save-refused-archived-branch"))) + + :else (flash :cause cause :type :handled :timeout nil diff --git a/frontend/src/app/main/refs.cljs b/frontend/src/app/main/refs.cljs index 4ccd578d35..377f49d116 100644 --- a/frontend/src/app/main/refs.cljs +++ b/frontend/src/app/main/refs.cljs @@ -74,8 +74,22 @@ (get projects project-id))) st/state)) +(defn- open-closed-branch? + "True when the open file is a merged or an archived branch. A merged + branch has its work in main, and an archived one is set aside until it + is restored or turned into an ordinary file: both open read-only." + [state] + (let [ctx (:workspace-branch-context state)] + (and (contains? #{"merged" "archived"} (get-in ctx [:info :status])) + (= (:file-id ctx) (:current-file-id state))))) + (def permissions - (l/derived (l/key :permissions) team)) + "The team permissions, without edition on a merged or archived branch." + (l/derived (fn [state] + (cond-> (:permissions (dsh/lookup-team state)) + (open-closed-branch? state) + (assoc :can-edit false))) + st/state)) (def teams (l/derived (l/key :teams) st/state)) diff --git a/frontend/src/app/main/ui/workspace.cljs b/frontend/src/app/main/ui/workspace.cljs index 5355b3cf48..9c95562485 100644 --- a/frontend/src/app/main/ui/workspace.cljs +++ b/frontend/src/app/main/ui/workspace.cljs @@ -147,13 +147,6 @@ :overlay true :file-loading true}]) -(defn- make-team-ref - [team-id] - (l/derived (fn [state] - (let [teams (get state :teams)] - (get teams team-id))) - st/state)) - (defn- make-file-ref [file-id] (l/derived (fn [state] @@ -213,18 +206,17 @@ (let [layout (mf/deref refs/workspace-layout) wglobal (mf/deref refs/workspace-global) - team-ref (mf/with-memo [team-id] - (make-team-ref team-id)) file-ref (mf/with-memo [file-id] (make-file-ref file-id)) - team (mf/deref team-ref) file (mf/deref file-ref) file-loaded? (get file ::has-data) file-name (:name file) - permissions (:permissions team) + ;; a merged or archived branch takes no edits: `refs/permissions` + ;; drops `:can-edit` for it, and the contexts below pass that on + permissions (mf/deref refs/permissions) read-only? (mf/deref refs/workspace-read-only?) read-only? (or read-only? (not (:can-edit permissions))) @@ -286,28 +278,30 @@ [:> (mf/provider ctx/current-file-id) {:value file-id} [:> (mf/provider ctx/current-page-id) {:value page-id} [:> (mf/provider ctx/design-tokens) {:value design-tokens?} - [:> (mf/provider ctx/workspace-read-only?) {:value read-only?} - [:> modal-container*] - [:> components-debugger*] - [:section {:class (stl/css :workspace) - :style {:background-color background-color - :touch-action "none" - :position "relative"}} - [:> context-menu*] - (when (and file-loaded? page-id) - [:> workspace-inner* - {:page-id page-id - :file-id file-id - :file file - :wglobal wglobal - :layout layout}]) - (when (or (not (and file-loaded? page-id)) - ;; in wasm renderer, extend the pixel loader until the first frame is rendered - ;; but do not apply it when switching pages - (and wasm-renderer-enabled? - (not file-loaded?) - (not @first-frame-rendered?))) - [:> workspace-loader*])]]]]]])) + [:> (mf/provider ctx/permissions) {:value permissions} + [:> (mf/provider ctx/can-edit?) {:value (:can-edit permissions)} + [:> (mf/provider ctx/workspace-read-only?) {:value read-only?} + [:> modal-container*] + [:> components-debugger*] + [:section {:class (stl/css :workspace) + :style {:background-color background-color + :touch-action "none" + :position "relative"}} + [:> context-menu*] + (when (and file-loaded? page-id) + [:> workspace-inner* + {:page-id page-id + :file-id file-id + :file file + :wglobal wglobal + :layout layout}]) + (when (or (not (and file-loaded? page-id)) + ;; in wasm renderer, extend the pixel loader until the first frame is rendered + ;; but do not apply it when switching pages + (and wasm-renderer-enabled? + (not file-loaded?) + (not @first-frame-rendered?))) + [:> workspace-loader*])]]]]]]]])) (mf/defc workspace-page* {::mf/lazy-load true} diff --git a/frontend/src/app/main/ui/workspace/branch_switcher.cljs b/frontend/src/app/main/ui/workspace/branch_switcher.cljs index f03faca948..9cbf08a4cb 100644 --- a/frontend/src/app/main/ui/workspace/branch_switcher.cljs +++ b/frontend/src/app/main/ui/workspace/branch_switcher.cljs @@ -102,9 +102,35 @@ :is-current (boolean current) :data-branch (dm/str (:branch-file-id row)) :on-click on-open} - (if (:is-main row) - (main-subtitle (:is-library row)) - [:> row-status* {:row row :pr pr}])]]) + ;; merged and archived branches carry no change counts + (cond + (:is-main row) (main-subtitle (:is-library row)) + (= "merged" (:status row)) (tr "workspace.branches.header.merged") + (= "archived" (:status row)) (tr "workspace.branches.header.archived") + :else [:> row-status* {:row row :pr pr}])]]) + +(mf/defc closed-group* + "A collapsed section of the popover for branches out of the flow: its + title and count open it on click." + {::mf/private true} + [{:keys [label rows on-open]}] + (let [open* (mf/use-state false) + open? (deref open*) + on-toggle (mf/use-fn #(swap! open* not))] + (when (seq rows) + [:* + [:button {:class (stl/css :action) + :aria-expanded open? + :on-click on-toggle} + [:> i/icon* {:icon-id (if open? i/arrow-down i/arrow-right) :size "s"}] + [:span {:class (stl/css :action-label)} label] + [:span {:class (stl/css :action-aside)} (dm/str (count rows))]] + (when open? + [:ul {:class (stl/css :rows)} + (for [row rows] + [:> switcher-row* {:key (dm/str (:branch-file-id row)) + :row row + :on-open on-open}])])]))) (mf/defc switcher-popover* {::mf/private true} @@ -113,8 +139,6 @@ filter* (mf/use-state "") filter-v (deref filter*) - archived* (mf/use-state false) - show-archived? (deref archived*) matches? (fn [row] @@ -138,17 +162,13 @@ main-row (cons main-row) :always (filter matches?)) - archived - (->> rows - (remove #(= "open" (:status %))) - (filter matches?)) + with-status + (fn [status] + (filter #(and (= status (:status %)) (matches? %)) rows)) on-filter-change (mf/use-fn #(reset! filter* (dom/get-target-val %))) - on-toggle-archived - (mf/use-fn #(swap! archived* not)) - on-open (mf/use-fn (fn [event] @@ -231,20 +251,12 @@ :pr (get prs-by-branch (:id row)) :on-open on-open}])]]) - (when (seq archived) - [:* - [:button {:class (stl/css :action) - :aria-expanded show-archived? - :on-click on-toggle-archived} - [:> i/icon* {:icon-id (if show-archived? i/arrow-down i/arrow-right) :size "s"}] - [:span {:class (stl/css :action-label)} (tr "workspace.branches.archived")] - [:span {:class (stl/css :action-aside)} (dm/str (count archived))]] - (when show-archived? - [:ul {:class (stl/css :rows)} - (for [row archived] - [:> switcher-row* {:key (dm/str (:branch-file-id row)) - :row 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}]]) [:hr {:class (stl/css :divider)}] @@ -285,8 +297,6 @@ :created-by (:id profile) :branch-file-id file-id}) - merged? (contains? #{"merged" "archived"} (:status ctx)) - ;; clicks inside the row (chip and popover) must not close it bar-ref (mf/use-ref nil) @@ -357,8 +367,19 @@ :class (stl/css :status-note)} (main-subtitle (:is-shared file))] - ^boolean merged? - nil + ;; merged and archived branches open read-only and count no + ;; changes: the note says which of the two it is + (= "merged" (:status ctx)) + [:> text* {:as "span" + :typography t/body-small + :class (stl/css :status-note)} + (tr "workspace.branches.header.merged")] + + (= "archived" (:status ctx)) + [:> text* {:as "span" + :typography t/body-small + :class (stl/css :status-note)} + (tr "workspace.branches.header.archived")] (in-sync? ctx pr) [:> text* {:as "span" diff --git a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs index 8e50fcb745..d2d3c0b908 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs @@ -439,7 +439,7 @@ :on-click on-submit} (tr "workspace.branches.merge.confirm-accept")]]])) -;; --- Confirmation for relevant branch actions (merge / update) +;; --- Confirmation for relevant branch actions (merge / update / materialize) (defn- confirm-merge! "Open the merge dialog for `branch` (merge into main is irreversible; the @@ -463,6 +463,17 @@ :on-accept (fn [_] (st/emit! (dwb/update-branch-from-main branch {:excluded excluded})))})))) +(defn- confirm-materialize! + "Ask for confirmation before turning `branch` into an ordinary file: the + branch stops being one, so it can no longer be compared or merged." + [branch] + (st/emit! (modal/show {:type :confirm + :title (tr "workspace.branches.materialize.title") + :message (tr "workspace.branches.materialize.message" (:name branch)) + :accept-label (tr "workspace.branches.materialize.accept") + :accept-style :danger + :on-accept (fn [_] (st/emit! (dwb/materialize-branch (:branch-file-id branch))))}))) + ;; --- Diff counts: known, or honestly unknown ;; ;; A row whose comparison could not be made carries no counts at all (and @@ -488,7 +499,9 @@ author? (= (:created-by branch) (:id profile)) status (:status branch) merged? (= "merged" status) - archived? (contains? #{"archived" "merged"} status) + archived? (= "archived" status) + ;; only an open branch carries change counts + open? (= "open" status) editing? (mf/use-state false) desc* (mf/use-state (or (:description branch) "")) @@ -506,7 +519,13 @@ (st/emit! (dwb/set-branch-description (:id branch) (str/trim desc))) (reset! editing? false))) on-compare (mf/use-fn (mf/deps branch) - #(modal/show! :branch-compare {:branch 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)))) + on-materialize (mf/use-fn (mf/deps branch) + #(confirm-materialize! branch))] [:> modal* {:is-open true :on-open-change (prui/on-dismiss on-close)} [:> prui/dialog-header* {:icon i/git-branch @@ -569,7 +588,7 @@ [:dd {:class (stl/css :info-val)} (some-> (:updated-at branch) ct/inst ct/timeago)]]) - (when-not archived? + (when open? [:div {:class (stl/css :info-row)} [:dt {:class (stl/css :info-key)} (tr "workspace.branches.info.changes")] [:dd {:class (stl/css :info-val :info-counts)} @@ -595,7 +614,16 @@ (-> (:merged-at branch) ct/inst (ct/format-inst :localized-date-time))]])]] [:> modal-footer* {} - (when-not archived? + ;; 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} (tr "workspace.branches.compare")]) [:> button* {:variant "primary" :on-click on-close} (tr "labels.close")]]])) @@ -609,7 +637,11 @@ ahead (:ahead entry) behind (:behind entry) main? (:is-main entry) - archived? (contains? #{"archived" "merged"} (:status entry)) + merged? (= "merged" (:status entry)) + archived? (= "archived" (:status entry)) + ;; merged and archived branches are out of the flow: no counts, no + ;; branch work, and dimmed in the list + closed? (or merged? archived?) ;; `pr` is the branch's open pull request (nil when none): the ;; review is a STATE of the branch, so the entry surfaces it as a @@ -761,15 +793,10 @@ (fn [event] (dom/stop-propagation event) (on-set-menu false) - (st/emit! (modal/show {:type :confirm - :title (tr "workspace.branches.materialize.title") - :message (tr "workspace.branches.materialize.message" (:name entry)) - :accept-label (tr "workspace.branches.materialize.accept") - :accept-style :danger - :on-accept (fn [_] (st/emit! (dwb/materialize-branch (:branch-file-id entry))))}))))] + (confirm-materialize! entry)))] [:li {:class (stl/css-case :branch-entry true - :is-archived archived? + :is-archived closed? :is-current current) :role "button" :on-click (when-not current on-open)} @@ -805,7 +832,7 @@ (dm/str approvals "/" total)])])] [:div {:class (stl/css :branch-entry-aside)} - (when (and (not archived?) (not main?)) + (when (and (not closed?) (not main?)) [:div {:class (stl/css :branch-entry-counts)} (if (counts-error? entry) [:span {:class (stl/css :count-error) @@ -839,17 +866,17 @@ :on-click on-toggle-menu}])} [:> menu-item* {:id "info" :on-action on-info} (tr "workspace.branches.menu.info")] - (when-not archived? + (when-not merged? [:> menu-item* {:id "compare" :on-action on-compare} (tr "workspace.branches.compare")]) - (when (and (not archived?) (pos? (or behind 0))) + (when (and (not closed?) (pos? (or behind 0))) [:> menu-item* {:id "update" :on-action on-update} (tr "workspace.branches.update")]) ;; review lifecycle: a branch without an open pull request offers to ;; request one; with it, the review actions take its place (the ;; review either ends merging the branch or is cancelled, leaving ;; the branch as a normal branch again) - (when (and (not archived?) (dwpr/enabled?) (nil? pr)) + (when (and (not closed?) (dwpr/enabled?) (nil? pr)) [:> menu-item* {:id "request-review" :on-action on-request-review} (tr "workspace.pull-requests.menu.request-review")]) (when (some? pr) @@ -867,16 +894,18 @@ (when (and (some? pr) (or author? admin?)) [:> menu-item* {:id "cancel-review" :on-action on-cancel-review} (tr "workspace.pull-requests.actions.close")]) - (when-not archived? + (when-not closed? [:> menu-item* {:id "rename" :on-action on-start-rename} (tr "labels.rename")]) - [:> menu-item* {:id "archive" :on-action on-archive} - (tr (if archived? - "workspace.branches.menu.restore" - "workspace.branches.menu.archive"))] - ;; only an open branch can leave the branch engine: once merged or - ;; archived there is nothing left to turn into an ordinary file - (when-not archived? + ;; a merged branch cannot go back to work: its changes are in main + (when-not merged? + [:> menu-item* {:id "archive" :on-action on-archive} + (tr (if archived? + "workspace.branches.menu.restore" + "workspace.branches.menu.archive"))]) + ;; a merged branch has its work in main already: only an open or an + ;; archived branch can leave the branch engine as an ordinary file + (when-not merged? [:> menu-item* {:id "materialize" :on-action on-materialize} (tr "workspace.branches.materialize.action")]) [:> menu-item* {:id "delete" :on-action on-delete} @@ -886,11 +915,13 @@ (defn- branches-for-tab "The branch rows a tab of the all-branches dialog lists: the open ones, - the open ones the profile created, or the merged and archived ones." + the open ones the profile created, the merged ones, or the archived + ones." [tab rows profile-id] (case tab "mine" (filter #(and (= "open" (:status %)) (= profile-id (:created-by %))) rows) - "archived" (remove #(= "open" (:status %)) rows) + "merged" (filter #(= "merged" (:status %)) rows) + "archived" (filter #(= "archived" (:status %)) rows) (filter #(= "open" (:status %)) rows))) (mf/defc branches-list-dialog* @@ -937,6 +968,7 @@ (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) @@ -1840,18 +1872,23 @@ (defn- banner-status "What the bottom banner says about the open branch, most pressing - first and in the order of its primary action: merged, conflicts, main - moved, an open review, branch work." + first and in the order of its primary action: merged, archived, + conflicts, main moved, an open review, branch work." [ctx pr] (let [ahead (or (:ahead ctx) 0) behind (or (:behind ctx) 0) conflicts (or (:conflicts ctx) 0)] (cond - (contains? #{"merged" "archived"} (:status ctx)) + (= "merged" (:status ctx)) {:id :merged :icon i/git-merge :label (tr "workspace.branches.banner.merged")} + (= "archived" (:status ctx)) + {:id :archived + :icon i/git-branch + :label (tr "workspace.branches.banner.archived")} + (counts-error? ctx) {:id :counts-error :icon i/triangle-alert @@ -1887,7 +1924,8 @@ (mf/defc branch-context-banner* [{:keys [file-id]}] (let [ctx (mf/deref branch-context) - merged? (contains? #{"merged" "archived"} (:status ctx)) + merged? (= "merged" (:status ctx)) + archived? (= "archived" (:status ctx)) collapsed* (mf/use-state false) collapsed? (deref collapsed*) @@ -1915,6 +1953,8 @@ 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-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})) on-merge @@ -1980,7 +2020,7 @@ [:div {:id "branch-banner" :ref banner-ref :class (stl/css-case :branch-banner true - :branch-banner-merged merged? + :branch-banner-merged (or merged? archived?) :is-collapsed collapsed?)} [:button {:class (stl/css :branch-banner-handle) :title (tr (if collapsed? @@ -2001,12 +2041,32 @@ [:span {:class (stl/css :branch-banner-title)} (:label status)]] [:div {:class (stl/css :branch-banner-separator)}] - (if merged? + (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)} + [:> 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")]] + + :else (let [conflicts (or (:conflicts ctx) 0)] [:div {:class (stl/css :branch-banner-actions)} [:> button* {:variant "secondary" diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs index a964620c15..15a73ec196 100644 --- a/frontend/test/frontend_tests/main_errors_test.cljs +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -786,3 +786,22 @@ (let [state (ptk/update (last @events) {})] (t/is (nil? (get-in state [:notification :timeout]))) (t/is (some? (get-in state [:notification :links 0]))))))))) + +(t/deftest closed-branch-save-refusals-explain-themselves + (t/testing "a save refused on a merged or archived branch warns without a report" + (doseq [code [:branch-merged :branch-archived]] + (let [events (atom []) + scheduled (atom [])] + (with-redefs [st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + tm/schedule (mock/stub (fn [callback] + (swap! scheduled conj callback)))] + (errors/flash-persistence (ex-info "read-only branch" {:type :validation :code code})) + (doseq [callback @scheduled] (callback)) + (t/is (empty? (report-events @events)) "no error report for an expected refusal") + (t/is (= 1 (count @events))) + (let [state (ptk/update (first @events) {})] + (t/is (= :warning (get-in state [:notification :level]))) + (t/is (= (i18n/tr (if (= code :branch-merged) + "errors.save-refused-merged-branch" + "errors.save-refused-archived-branch")) + (get-in state [:notification :content]))))))))) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index a5b4bc6982..841378027e 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -1735,12 +1735,23 @@ msgstr "" msgid "errors.registration-disabled" msgstr "The registration is currently disabled." -#: src/app/main/errors.cljs:538 +#: src/app/main/errors.cljs:549 msgid "errors.save-failed" msgstr "" "Autosave is not working due to an error. Contact support to report the " "error and reload to continue from your last saved version." +#: src/app/main/errors.cljs:542 +msgid "errors.save-refused-archived-branch" +msgstr "" +"This branch is archived and read only. Restore it or turn it into an " +"ordinary file to keep editing." + +#: src/app/main/errors.cljs:539 +msgid "errors.save-refused-merged-branch" +msgstr "" +"This branch is merged into main and read only. Make your changes in main." + #: src/app/main/data/persistence.cljs:307 msgid "errors.save-retrying" msgstr "Connection lost. Retrying to save your changes." @@ -6348,6 +6359,10 @@ msgstr "Ungroup" msgid "workspace.branches.archived" msgstr "Archived" +#: src/app/main/ui/workspace/sidebar/branches.cljs:1840 +msgid "workspace.branches.banner.archived" +msgstr "Archived branch" + #, unused msgid "workspace.branches.banner.collapse" msgstr "Collapse" @@ -6923,6 +6938,14 @@ msgstr "No branches yet" msgid "workspace.branches.header-badge-tooltip" msgstr "On branch %s from main" +#: src/app/main/ui/workspace/branch_switcher.cljs:98, src/app/main/ui/workspace/branch_switcher.cljs:315 +msgid "workspace.branches.header.archived" +msgstr "Archived, read only" + +#: src/app/main/ui/workspace/branch_switcher.cljs:97, src/app/main/ui/workspace/branch_switcher.cljs:309 +msgid "workspace.branches.header.merged" +msgstr "Merged into main, read only" + #: src/app/main/ui/workspace/branch_switcher.cljs:295 msgid "workspace.branches.header-badge-tooltip-library" msgstr "On branch %s from main. Library consumers can adopt changes from main only." @@ -7160,7 +7183,7 @@ msgstr "Default branch (read by library consumers)" msgid "workspace.branches.materialize.accept" msgstr "Turn it into a file" -#: src/app/main/ui/workspace/sidebar/branches.cljs:882 +#: src/app/main/ui/workspace/sidebar/branches.cljs:609, src/app/main/ui/workspace/sidebar/branches.cljs:895, src/app/main/ui/workspace/sidebar/branches.cljs:2026 msgid "workspace.branches.materialize.action" msgstr "Turn into an ordinary file" @@ -7261,6 +7284,10 @@ 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"