From 315b389a66d888bcd5aaf1acbb5536adf8a873fc Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 3 Feb 2025 12:49:56 +0100 Subject: [PATCH] :bug: Fix name generation and handling on creating objects (files, tokens, ...) (#5745) * :sparkles: Update function implementation * :sparkles: Add tests for a new function implementation * :sparkles: Update function usages according to new signature * :sparkles: Update docstrings * :sparkles: Use simpler assertion * :lipstick: Replace concat with cons on name-seq * :bug: Fix incorrect error handling on legacy workspace redirect --------- Co-authored-by: Andrey Fedorov --- backend/src/app/features/components_v2.clj | 2 +- common/src/app/common/files/helpers.cljc | 59 ++++++++++++------- common/src/app/common/logic/libraries.cljc | 2 +- .../test/common_tests/files/helpers_test.cljc | 38 ++++++++++++ frontend/src/app/main/data/dashboard.cljs | 54 ++++++++++------- frontend/src/app/main/data/tokens.cljs | 20 +++++-- frontend/src/app/main/data/workspace.cljs | 12 ++-- .../app/main/data/workspace/interactions.cljs | 6 +- frontend/src/app/main/ui.cljs | 5 +- frontend/translations/en.po | 4 ++ 10 files changed, 144 insertions(+), 58 deletions(-) create mode 100644 common/test/common_tests/files/helpers_test.cljc diff --git a/backend/src/app/features/components_v2.clj b/backend/src/app/features/components_v2.clj index bb0acd876..025916bb6 100644 --- a/backend/src/app/features/components_v2.clj +++ b/backend/src/app/features/components_v2.clj @@ -1071,7 +1071,7 @@ groups (d/group-by #(first (cfh/split-path (:path %))) assets) ;; If there is a group called as the generic-name we have to preserve it unames (into #{} (keep str) (keys groups)) - groups (rename-keys groups {generic-name (cfh/generate-unique-name unames generic-name)}) + groups (rename-keys groups {generic-name (cfh/generate-unique-name generic-name unames)}) ;; Split large groups in chunks of max-group-size elements groups (loop [groups (seq groups) diff --git a/common/src/app/common/files/helpers.cljc b/common/src/app/common/files/helpers.cljc index c3f56695d..f167601b6 100644 --- a/common/src/app/common/files/helpers.cljc +++ b/common/src/app/common/files/helpers.cljc @@ -9,7 +9,6 @@ [app.common.data :as d] [app.common.data.macros :as dm] [app.common.geom.shapes.common :as gco] - [app.common.schema :as sm] [app.common.uuid :as uuid] [clojure.set :as set] [clojure.walk :as walk] @@ -400,31 +399,51 @@ elements)] (into #{} (keep :name) elements))) -(defn- extract-numeric-suffix - [basename] - (if-let [[_ p1 p2] (re-find #"(.*) ([0-9]+)$" basename)] - [p1 (+ 1 (d/parse-integer p2))] - [basename 1])) +(defn- name-seq + "Creates a lazy, infinite sequence of names starting with `base-name`, + followed by variants with suffixes applied. The sequence follows this pattern: + - `base-name` + - `(str base-name (suffix-fn 1))` + - `(str base-name (suffix-fn 2))` + - `(str base-name (suffix-fn 3))`, etc." + [base-name suffix-fn] + (cons base-name + (map #(str/concat base-name (suffix-fn %)) + (iterate inc 1)))) + +(defn ^:private get-suffix + "Default suffix impelemtation" + [copy-count] + (str/concat " " copy-count)) (defn generate-unique-name - "A unique name generator" - [used basename] + "Generates a unique name by selecting the first available name from a generated sequence. + The sequence consists of `base-name` and its variants, avoiding conflicts with `existing-names`. + + Parameters: + - `base-name` - string used as the base for name generation. + - `existing-names` - a collection of existing names to check for uniqueness. + - Options: + - `:suffix-fn` - a function that generates suffixes, given an integer (default: `get-suffix`). + - `:immediate-suffix?` - if `true`, the base name is considered taken, and suffixing starts immediately. + + Returns: + - A unique name not present in `existing-names`." + [base-name existing-names & {:keys [suffix-fn immediate-suffix?] + :or {suffix-fn get-suffix}}] (dm/assert! "expected a set of strings" - (sm/check-set-of-strings! used)) + (coll? existing-names)) (dm/assert! "expected a string for `basename`." - (string? basename)) - - (if-not (contains? used basename) - basename - (let [[prefix initial] (extract-numeric-suffix basename)] - (loop [counter initial] - (let [candidate (str prefix " " counter)] - (if (contains? used candidate) - (recur (inc counter)) - candidate)))))) + (string? base-name)) + (let [existing-name-set (cond-> (set existing-names) + immediate-suffix? (conj base-name)) + names (name-seq base-name suffix-fn)] + (->> names + (remove #(contains? existing-name-set %)) + first))) (defn walk-pages "Go through all pages of a file and apply a function to each one" @@ -724,7 +743,7 @@ (defn split-by-last-period "Splits a string into two parts: - the text before and including the last period, + the text before and including the last period, and the text after the last period." [s] (if-let [last-period (str/last-index-of s ".")] diff --git a/common/src/app/common/logic/libraries.cljc b/common/src/app/common/logic/libraries.cljc index adfea7774..a090fe690 100644 --- a/common/src/app/common/logic/libraries.cljc +++ b/common/src/app/common/logic/libraries.cljc @@ -2011,7 +2011,7 @@ has-flow? (partial ctp/get-frame-flow flows)] (reduce (fn [changes frame-id] - (let [name (cfh/generate-unique-name @unames "Flow 1") + (let [name (cfh/generate-unique-name "Flow" @unames :immediate-suffix? true) frame-id (get ids-map frame-id) flow-id (uuid/next) new-flow {:id flow-id diff --git a/common/test/common_tests/files/helpers_test.cljc b/common/test/common_tests/files/helpers_test.cljc new file mode 100644 index 000000000..00d30dbf3 --- /dev/null +++ b/common/test/common_tests/files/helpers_test.cljc @@ -0,0 +1,38 @@ +;; 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 + +(ns common-tests.files.helpers-test + (:require + [app.common.files.helpers :as cfh] + [clojure.test :as t])) + +(t/deftest test-generate-unique-name + (t/testing "Test unique name generation" + (let [suffix-fn #(str "-copy-" %)] + (t/is (cfh/generate-unique-name "base-name" + #{"base-name" "base-name-copy-1"} + :suffix-fn suffix-fn) + "base-name-copy-2") + (t/is (cfh/generate-unique-name "base-name" + #{"base-name-copy-2"} + :suffix-fn suffix-fn) + "base-name-copy-1") + (t/is (cfh/generate-unique-name "base-name" + #{"base-namec-copy"} + :suffix-fn suffix-fn) + "base-name-copy-1") + (t/is (cfh/generate-unique-name "base-name" + #{"base-name"} + :suffix-fn suffix-fn) + "base-name-copy-1"))) + + (t/testing "Test unique name generation with immidate suffix and default suffix-fn" + (t/is (cfh/generate-unique-name "base-name" #{} :immediate-suffix? true) + "base-name 1") + (t/is (cfh/generate-unique-name "base-name" + #{"base-name 1" "base-name 2"} + :immediate-suffix? true) + "base-name 3"))) diff --git a/frontend/src/app/main/data/dashboard.cljs b/frontend/src/app/main/data/dashboard.cljs index 3f0d9bbd3..5e637cc0f 100644 --- a/frontend/src/app/main/data/dashboard.cljs +++ b/frontend/src/app/main/data/dashboard.cljs @@ -25,6 +25,7 @@ [app.util.time :as dt] [beicon.v2.core :as rx] [clojure.set :as set] + [cuerdas.core :as str] [potok.v2.core :as ptk])) (log/set-level! :warn) @@ -247,9 +248,9 @@ (ptk/reify ::create-project ptk/WatchEvent (watch [_ state _] - (let [projects (get state :projects) - unames (cfh/get-used-names projects) - name (cfh/generate-unique-name unames (str (tr "dashboard.new-project-prefix") " 1")) + (let [unames (cfh/get-used-names (get state :projects)) + base-name (tr "dashboard.new-project-prefix") + name (cfh/generate-unique-name base-name unames :immediate-suffix? true) team-id (:current-team-id state) params {:name name :team-id team-id} @@ -280,13 +281,18 @@ :name name}) ptk/WatchEvent - (watch [_ _ _] + (watch [_ state _] (let [{:keys [on-success on-error] :or {on-success identity on-error rx/throw}} (meta params) - - new-name (str name " " (tr "dashboard.copy-suffix"))] - + projects (get state :projects) + unames (cfh/get-used-names projects) + suffix-fn (fn [copy-count] + (str/concat " " + (tr "dashboard.copy-suffix") + (when (> copy-count 1) + (str " " copy-count)))) + new-name (cfh/generate-unique-name name unames :suffix-fn suffix-fn)] (->> (rp/cmd! :duplicate-project {:project-id id :name new-name}) (rx/tap on-success) (rx/map project-duplicated) @@ -477,15 +483,15 @@ (let [{:keys [on-success on-error] :or {on-success identity on-error rx/throw}} (meta params) - - files (get state :files) - unames (cfh/get-used-names files) - name (or name (cfh/generate-unique-name unames (str (tr "dashboard.new-file-prefix") " 1"))) - features (-> (features/get-team-enabled-features state) - (set/difference cfeat/frontend-only-features)) - params (-> params - (assoc :name name) - (assoc :features features))] + unames (cfh/get-used-names (get state :files)) + base-name (tr "dashboard.new-file-prefix") + name (or name + (cfh/generate-unique-name base-name unames :immediate-suffix? true)) + features (-> (features/get-team-enabled-features state) + (set/difference cfeat/frontend-only-features)) + params (-> params + (assoc :name name) + (assoc :features features))] (->> (rp/cmd! :create-file params) (rx/tap on-success) @@ -500,13 +506,17 @@ (dm/assert! (string? name)) (ptk/reify ::duplicate-file ptk/WatchEvent - (watch [_ _ _] + (watch [_ state _] (let [{:keys [on-success on-error] :or {on-success identity on-error rx/throw}} (meta params) - - new-name (str name " " (tr "dashboard.copy-suffix"))] - + unames (cfh/get-used-names (get state :files)) + suffix-fn (fn [copy-count] + (str/concat " " + (tr "dashboard.copy-suffix") + (when (> copy-count 1) + (str " " copy-count)))) + new-name (cfh/generate-unique-name name unames :suffix-fn suffix-fn)] (->> (rp/cmd! :duplicate-file {:file-id id :name new-name}) (rx/tap on-success) (rx/map file-created) @@ -589,10 +599,10 @@ name (if in-project? (let [files (get state :files) unames (cfh/get-used-names files)] - (cfh/generate-unique-name unames (str (tr "dashboard.new-file-prefix") " 1"))) + (cfh/generate-unique-name (tr "dashboard.new-file-prefix") unames :immediate-suffix? true)) (let [projects (get state :projects) unames (cfh/get-used-names projects)] - (cfh/generate-unique-name unames (str (tr "dashboard.new-project-prefix") " 1")))) + (cfh/generate-unique-name (tr "dashboard.new-project-prefix") unames :immediate-suffix? true))) params (if in-project? {:project-id (:project-id pparams) :name name} diff --git a/frontend/src/app/main/data/tokens.cljs b/frontend/src/app/main/data/tokens.cljs index 3373d35a4..92db299b0 100644 --- a/frontend/src/app/main/data/tokens.cljs +++ b/frontend/src/app/main/data/tokens.cljs @@ -8,6 +8,7 @@ (:require [app.common.data.macros :as dm] [app.common.files.changes-builder :as pcb] + [app.common.files.helpers :as cfh] [app.common.geom.point :as gpt] [app.common.logic.tokens :as clt] [app.common.types.shape :as cts] @@ -286,10 +287,21 @@ (ptk/reify ::duplicate-token ptk/WatchEvent (watch [_ state _] - (when-let [token (some-> (dwts/get-selected-token-set-token state token-name) - (update :name #(str/concat % "-copy")))] - (rx/of - (update-create-token {:token token})))))) + (let [token-set (dwts/get-selected-token-set state) + token (some-> token-set (ctob/get-token token-name)) + tokens (some-> token-set (ctob/get-tokens)) + suffix-fn (fn [copy-count] + (let [suffix (tr "workspace.token.duplicate-suffix")] + (str/concat "-" + suffix + (when (> copy-count 1) + (str "-" copy-count))))) + unames (map :name tokens) + copy-name (cfh/generate-unique-name token-name unames :suffix-fn suffix-fn)] + (when token + (rx/of + (update-create-token + {:token (assoc token :name copy-name)}))))))) (defn set-token-type-section-open [token-type open?] diff --git a/frontend/src/app/main/data/workspace.cljs b/frontend/src/app/main/data/workspace.cljs index 0b44bceb2..c181104c4 100644 --- a/frontend/src/app/main/data/workspace.cljs +++ b/frontend/src/app/main/data/workspace.cljs @@ -507,10 +507,8 @@ (watch [it state _] (let [pages (-> (dsh/lookup-file-data state) (get :pages-index)) - unames (cfh/get-used-names pages) - name (cfh/generate-unique-name unames "Page 1") - + name (cfh/generate-unique-name "Page" unames :immediate-suffix? true) changes (-> (pcb/empty-changes it) (pcb/add-empty-page id name))] @@ -527,7 +525,13 @@ page (get pages page-id) unames (cfh/get-used-names pages) - name (cfh/generate-unique-name unames (:name page)) + suffix-fn (fn [copy-count] + (str/concat " " + (tr "dashboard.copy-suffix") + (when (> copy-count 1) + (str " " copy-count)))) + base-name (:name page) + name (cfh/generate-unique-name base-name unames :suffix-fn suffix-fn) objects (update-vals (:objects page) #(dissoc % :use-for-thumbnail)) main-instances-ids (set (keep #(when (ctk/main-instance? (val %)) (key %)) objects)) diff --git a/frontend/src/app/main/data/workspace/interactions.cljs b/frontend/src/app/main/data/workspace/interactions.cljs index f9f64f3a2..99ceee947 100644 --- a/frontend/src/app/main/data/workspace/interactions.cljs +++ b/frontend/src/app/main/data/workspace/interactions.cljs @@ -44,9 +44,9 @@ (dsh/lookup-page state page-id) (dsh/lookup-page state)) - flows (get page :flows) - unames (cfh/get-used-names (vals flows)) - name (or name (cfh/generate-unique-name unames "Flow 1")) + unames (cfh/get-used-names (vals (get page :flows))) + name (or name + (cfh/generate-unique-name "Flow" unames :immediate-suffix? true)) flow-id (or flow-id (uuid/next)) diff --git a/frontend/src/app/main/ui.cljs b/frontend/src/app/main/ui.cljs index a7cbde8f0..bf6b1ebaa 100644 --- a/frontend/src/app/main/ui.cljs +++ b/frontend/src/app/main/ui.cljs @@ -10,6 +10,7 @@ [app.config :as cf] [app.main.data.common :as dcm] [app.main.data.team :as dtm] + [app.main.errors :as errors] [app.main.refs :as refs] [app.main.repo :as rp] [app.main.router :as rt] @@ -29,7 +30,6 @@ [app.util.dom :as dom] [app.util.i18n :refer [tr]] [beicon.v2.core :as rx] - [potok.v2.core :as ptk] [rumext.v2 :as mf])) (def auth-page @@ -61,8 +61,7 @@ :file-id file-id :page-id page-id :layout layout))) - ptk/handle-error))) - + errors/on-error))) [:> loader* {:title (tr "labels.loading") :overlay true}]) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 3387628ca..b9c07d7ab 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -1143,6 +1143,10 @@ msgstr "Cannot drop a parent set to an own child path." msgid "errors.drag-drop.set-exists" msgstr "Cannot complete drop, a set with same name already exists at path %s." +#: src/app/main/data/tokens.cljs:294 +msgid "workspace.token.duplicate-suffix" +msgstr "copy" + #: src/app/main/ui/auth/verify_token.cljs:84, src/app/main/ui/settings/change_email.cljs:29 msgid "errors.email-already-exists" msgstr "Email already used"