mirror of
https://github.com/penpot/penpot.git
synced 2026-07-31 18:36:18 +00:00
🐛 Fix auto-link libraries: permission check, default selection, and tests
Backend: - Add edit-permission check in auto-link-libraries before creating file_library_rel rows, matching the manual link RPC behavior - Fix 3 broken tests by removing source library before import to simulate cross-environment scenario - Add test for permission check (viewer vs owner auto-link) - Add test for used-by filtering (only linked files get rels) Frontend: - Fix library resolution default selection: pre-select first candidate so confirming without interaction actually links libraries - Remove unused file-ids prop from library-resolution* component - Remove unused translation keys (library-link-error, library-placeholder)
This commit is contained in:
parent
24fa844558
commit
5628efdfad
@ -931,24 +931,29 @@
|
|||||||
"Auto-link imported files to libraries that have exactly one candidate
|
"Auto-link imported files to libraries that have exactly one candidate
|
||||||
match. Returns a vector of {:id old-lib-id :name string :new-id uuid}
|
match. Returns a vector of {:id old-lib-id :name string :new-id uuid}
|
||||||
for each auto-linked library."
|
for each auto-linked library."
|
||||||
[{:keys [::db/conn ::manifest ::bfc/timestamp] :as cfg} resolution file-ids]
|
[{:keys [::db/conn ::manifest ::bfc/timestamp ::bfc/profile-id] :as cfg} resolution file-ids]
|
||||||
(let [external-libs (:external-libraries manifest)]
|
(let [external-libs (:external-libraries manifest)]
|
||||||
(reduce (fn [linked {:keys [id name used-by] :as ext-lib}]
|
(reduce (fn [linked {:keys [id name used-by] :as ext-lib}]
|
||||||
(let [candidates (get resolution id)]
|
(let [candidates (get resolution id)]
|
||||||
(if (= 1 (count candidates))
|
(if (= 1 (count candidates))
|
||||||
(let [new-lib-id (:id (first candidates))
|
(let [new-lib-id (:id (first candidates))
|
||||||
;; Link only files that actually used this library
|
perms (bfc/get-file-permissions conn profile-id new-lib-id)]
|
||||||
relevant-file-ids (if (seq used-by)
|
;; Only auto-link when the importer has edit permission
|
||||||
(let [used-set (set (map bfc/lookup-index used-by))]
|
;; on the matched library, matching the manual link RPC.
|
||||||
(filterv used-set file-ids))
|
(if (:can-edit perms)
|
||||||
file-ids)]
|
(let [;; Link only files that actually used this library
|
||||||
(doseq [fid relevant-file-ids]
|
relevant-file-ids (if (seq used-by)
|
||||||
(let [rel-params {:file-id fid
|
(let [used-set (set (map bfc/lookup-index used-by))]
|
||||||
:library-file-id new-lib-id}]
|
(filterv used-set file-ids))
|
||||||
(db/insert! conn :file-library-rel rel-params
|
file-ids)]
|
||||||
::db/on-conflict-do-nothing? true)
|
(doseq [fid relevant-file-ids]
|
||||||
(bfc/upsert-file-library-sync! conn (assoc rel-params :synced-at timestamp))))
|
(let [rel-params {:file-id fid
|
||||||
(conj linked {:id id :name name :new-id new-lib-id}))
|
:library-file-id new-lib-id}]
|
||||||
|
(db/insert! conn :file-library-rel rel-params
|
||||||
|
::db/on-conflict-do-nothing? true)
|
||||||
|
(bfc/upsert-file-library-sync! conn (assoc rel-params :synced-at timestamp))))
|
||||||
|
(conj linked {:id id :name name :new-id new-lib-id}))
|
||||||
|
linked))
|
||||||
linked)))
|
linked)))
|
||||||
[]
|
[]
|
||||||
external-libs)))
|
external-libs)))
|
||||||
|
|||||||
@ -12,6 +12,7 @@
|
|||||||
[app.common.features :as cfeat]
|
[app.common.features :as cfeat]
|
||||||
[app.common.pprint :as pp]
|
[app.common.pprint :as pp]
|
||||||
[app.common.thumbnails :as thc]
|
[app.common.thumbnails :as thc]
|
||||||
|
[app.common.time :as ct]
|
||||||
[app.common.types.shape :as cts]
|
[app.common.types.shape :as cts]
|
||||||
[app.common.uuid :as uuid]
|
[app.common.uuid :as uuid]
|
||||||
[app.db :as db]
|
[app.db :as db]
|
||||||
@ -178,6 +179,12 @@
|
|||||||
(assoc ::bfc/include-libraries false))
|
(assoc ::bfc/include-libraries false))
|
||||||
(io/output-stream output))
|
(io/output-stream output))
|
||||||
|
|
||||||
|
;; Remove the source library to simulate a cross-environment import
|
||||||
|
;; where the original library does not exist in the target team.
|
||||||
|
(db/update! th/*system* :file
|
||||||
|
{:deleted-at (ct/now)}
|
||||||
|
{:id (:id library)})
|
||||||
|
|
||||||
;; Now create a new shared library with the same name in the same team
|
;; Now create a new shared library with the same name in the same team
|
||||||
;; (simulating the library existing in the target environment)
|
;; (simulating the library existing in the target environment)
|
||||||
(let [library2 (th/create-file* 3 {:profile-id (:id profile)
|
(let [library2 (th/create-file* 3 {:profile-id (:id profile)
|
||||||
@ -229,6 +236,12 @@
|
|||||||
(assoc ::bfc/include-libraries false))
|
(assoc ::bfc/include-libraries false))
|
||||||
(io/output-stream output))
|
(io/output-stream output))
|
||||||
|
|
||||||
|
;; Remove the source library to simulate a cross-environment import
|
||||||
|
;; where no matching library exists in the target team.
|
||||||
|
(db/update! th/*system* :file
|
||||||
|
{:deleted-at (ct/now)}
|
||||||
|
{:id (:id library)})
|
||||||
|
|
||||||
;; Import without any matching library in the team
|
;; Import without any matching library in the team
|
||||||
(let [result (-> th/*system*
|
(let [result (-> th/*system*
|
||||||
(assoc ::bfc/project-id (:default-project-id profile))
|
(assoc ::bfc/project-id (:default-project-id profile))
|
||||||
@ -267,6 +280,12 @@
|
|||||||
(assoc ::bfc/include-libraries false))
|
(assoc ::bfc/include-libraries false))
|
||||||
(io/output-stream output))
|
(io/output-stream output))
|
||||||
|
|
||||||
|
;; Remove the source library to simulate a cross-environment import
|
||||||
|
;; where the original library does not exist in the target team.
|
||||||
|
(db/update! th/*system* :file
|
||||||
|
{:deleted-at (ct/now)}
|
||||||
|
{:id (:id library)})
|
||||||
|
|
||||||
;; Create TWO shared libraries with the same name
|
;; Create TWO shared libraries with the same name
|
||||||
(let [library2 (th/create-file* 3 {:profile-id (:id profile)
|
(let [library2 (th/create-file* 3 {:profile-id (:id profile)
|
||||||
:project-id (:default-project-id profile)
|
:project-id (:default-project-id profile)
|
||||||
@ -299,3 +318,131 @@
|
|||||||
(let [rels (db/query th/*system* :file-library-rel
|
(let [rels (db/query th/*system* :file-library-rel
|
||||||
{:library-file-id (:id library3)})]
|
{:library-file-id (:id library3)})]
|
||||||
(t/is (= 0 (count rels))))))))
|
(t/is (= 0 (count rels))))))))
|
||||||
|
|
||||||
|
(t/deftest import-auto-link-respects-library-permissions
|
||||||
|
(let [owner (th/create-profile* 1)
|
||||||
|
team (th/create-team* 1 {:profile-id (:id owner)})
|
||||||
|
viewer (th/create-profile* 2)
|
||||||
|
_ (th/create-team-role* {:team-id (:id team)
|
||||||
|
:profile-id (:id viewer)
|
||||||
|
:role :viewer})
|
||||||
|
|
||||||
|
library (th/create-file* 1 {:profile-id (:id owner)
|
||||||
|
:project-id (:default-project-id owner)
|
||||||
|
:is-shared true
|
||||||
|
:name "Icons Library"})
|
||||||
|
file (th/create-file* 2 {:profile-id (:id owner)
|
||||||
|
:project-id (:default-project-id owner)
|
||||||
|
:is-shared false})]
|
||||||
|
|
||||||
|
;; Link file to library
|
||||||
|
(db/insert! th/*system* :file-library-rel
|
||||||
|
{:file-id (:id file)
|
||||||
|
:library-file-id (:id library)})
|
||||||
|
|
||||||
|
;; Export without including libraries
|
||||||
|
(let [output (tmp/tempfile :suffix ".zip")]
|
||||||
|
(v3/export-files!
|
||||||
|
(-> th/*system*
|
||||||
|
(assoc ::bfc/ids #{(:id file)})
|
||||||
|
(assoc ::bfc/embed-assets false)
|
||||||
|
(assoc ::bfc/include-libraries false))
|
||||||
|
(io/output-stream output))
|
||||||
|
|
||||||
|
;; Remove the source library and recreate a matching one owned by owner
|
||||||
|
(db/update! th/*system* :file
|
||||||
|
{:deleted-at (ct/now)}
|
||||||
|
{:id (:id library)})
|
||||||
|
|
||||||
|
;; Create a project in the team for the matched library and import.
|
||||||
|
(let [project (th/create-project* 1 {:profile-id (:id owner)
|
||||||
|
:team-id (:id team)})
|
||||||
|
|
||||||
|
library2 (th/create-file* 3 {:profile-id (:id owner)
|
||||||
|
:project-id (:id project)
|
||||||
|
:is-shared true
|
||||||
|
:name "Icons Library"})
|
||||||
|
|
||||||
|
result (-> th/*system*
|
||||||
|
(assoc ::bfc/project-id (:id project))
|
||||||
|
(assoc ::bfc/profile-id (:id viewer))
|
||||||
|
(assoc ::bfc/team-id (:id team))
|
||||||
|
(assoc ::bfc/input output)
|
||||||
|
(v3/import-files!))]
|
||||||
|
|
||||||
|
;; Auto-link must be skipped because viewer cannot edit the library
|
||||||
|
(t/is (= [] (:auto-linked result)))
|
||||||
|
|
||||||
|
;; No file-library-rel should have been created
|
||||||
|
(let [rels (db/query th/*system* :file-library-rel
|
||||||
|
{:library-file-id (:id library2)})]
|
||||||
|
(t/is (= 0 (count rels))))
|
||||||
|
|
||||||
|
;; Control: the same import performed by the owner (who has edit
|
||||||
|
;; permission on the library) should auto-link.
|
||||||
|
(let [result (-> th/*system*
|
||||||
|
(assoc ::bfc/project-id (:id project))
|
||||||
|
(assoc ::bfc/profile-id (:id owner))
|
||||||
|
(assoc ::bfc/team-id (:id team))
|
||||||
|
(assoc ::bfc/input output)
|
||||||
|
(v3/import-files!))]
|
||||||
|
|
||||||
|
(t/is (= 1 (count (:auto-linked result))))
|
||||||
|
(t/is (= (:id library2) (:new-id (first (:auto-linked result)))))
|
||||||
|
(let [rels (db/query th/*system* :file-library-rel
|
||||||
|
{:library-file-id (:id library2)})]
|
||||||
|
(t/is (= 1 (count rels)))))))))
|
||||||
|
|
||||||
|
(t/deftest import-auto-link-only-files-that-used-library
|
||||||
|
(let [profile (th/create-profile* 1)
|
||||||
|
library (th/create-file* 1 {:profile-id (:id profile)
|
||||||
|
:project-id (:default-project-id profile)
|
||||||
|
:is-shared true
|
||||||
|
:name "Icons Library"})
|
||||||
|
file1 (th/create-file* 2 {:profile-id (:id profile)
|
||||||
|
:project-id (:default-project-id profile)
|
||||||
|
:is-shared false})
|
||||||
|
file2 (th/create-file* 3 {:profile-id (:id profile)
|
||||||
|
:project-id (:default-project-id profile)
|
||||||
|
:is-shared false})]
|
||||||
|
|
||||||
|
;; Only file1 uses the library
|
||||||
|
(db/insert! th/*system* :file-library-rel
|
||||||
|
{:file-id (:id file1)
|
||||||
|
:library-file-id (:id library)})
|
||||||
|
|
||||||
|
;; Export both files without including libraries
|
||||||
|
(let [output (tmp/tempfile :suffix ".zip")]
|
||||||
|
(v3/export-files!
|
||||||
|
(-> th/*system*
|
||||||
|
(assoc ::bfc/ids #{(:id file1) (:id file2)})
|
||||||
|
(assoc ::bfc/embed-assets false)
|
||||||
|
(assoc ::bfc/include-libraries false))
|
||||||
|
(io/output-stream output))
|
||||||
|
|
||||||
|
;; Remove the source library and recreate a matching one
|
||||||
|
(db/update! th/*system* :file
|
||||||
|
{:deleted-at (ct/now)}
|
||||||
|
{:id (:id library)})
|
||||||
|
|
||||||
|
(let [library2 (th/create-file* 4 {:profile-id (:id profile)
|
||||||
|
:project-id (:default-project-id profile)
|
||||||
|
:is-shared true
|
||||||
|
:name "Icons Library"})
|
||||||
|
|
||||||
|
result (-> th/*system*
|
||||||
|
(assoc ::bfc/project-id (:default-project-id profile))
|
||||||
|
(assoc ::bfc/profile-id (:id profile))
|
||||||
|
(assoc ::bfc/team-id (:default-team-id profile))
|
||||||
|
(assoc ::bfc/input output)
|
||||||
|
(v3/import-files!))]
|
||||||
|
|
||||||
|
;; The library should be auto-linked
|
||||||
|
(t/is (= 1 (count (:auto-linked result))))
|
||||||
|
(t/is (= (:id library2) (:new-id (first (:auto-linked result)))))
|
||||||
|
(t/is (= {} (:library-candidates result)))
|
||||||
|
|
||||||
|
;; But only one file-library-rel should exist (for file1)
|
||||||
|
(let [rels (db/query th/*system* :file-library-rel
|
||||||
|
{:library-file-id (:id library2)})]
|
||||||
|
(t/is (= 1 (count rels))))))))
|
||||||
|
|||||||
@ -346,33 +346,41 @@
|
|||||||
[{:keys [resolution selections*]}]
|
[{:keys [resolution selections*]}]
|
||||||
(let [external-libs (:external-libs resolution)
|
(let [external-libs (:external-libs resolution)
|
||||||
library-candidates (:library-candidates resolution)
|
library-candidates (:library-candidates resolution)
|
||||||
selections (deref selections*)
|
selections (deref selections*)]
|
||||||
|
|
||||||
on-select
|
;; Pre-select the first candidate for each unresolved library so that
|
||||||
(mf/use-fn
|
;; confirming without interaction still links the default choice.
|
||||||
(mf/deps selections*)
|
(mf/with-effect []
|
||||||
(fn [old-lib-id candidate-id]
|
(doseq [[old-lib-id candidates] library-candidates]
|
||||||
(swap! selections* assoc old-lib-id candidate-id)))]
|
(when-not (contains? @selections* old-lib-id)
|
||||||
|
(when-let [first-candidate (first candidates)]
|
||||||
|
(swap! selections* assoc old-lib-id (:id first-candidate))))))
|
||||||
|
|
||||||
(when (seq library-candidates)
|
(let [on-select
|
||||||
[:div {:class (stl/css :library-resolution)}
|
(mf/use-fn
|
||||||
[:p {:class (stl/css :library-resolution-message)}
|
(mf/deps selections*)
|
||||||
(tr "dashboard.import.resolve-libraries")]
|
(fn [old-lib-id candidate-id]
|
||||||
|
(swap! selections* assoc old-lib-id candidate-id)))]
|
||||||
|
|
||||||
(for [{:keys [id name]} external-libs]
|
(when (seq library-candidates)
|
||||||
(let [candidates (get library-candidates id)
|
[:div {:class (stl/css :library-resolution)}
|
||||||
options (mapv (fn [c]
|
[:p {:class (stl/css :library-resolution-message)}
|
||||||
{:id (str (:id c))
|
(tr "dashboard.import.resolve-libraries")]
|
||||||
:label (:name c)})
|
|
||||||
candidates)
|
(for [{:keys [id name]} external-libs]
|
||||||
selected (get selections id)]
|
(let [candidates (get library-candidates id)
|
||||||
[:div {:class (stl/css :library-resolution-item)
|
options (mapv (fn [c]
|
||||||
:key (dm/str id)}
|
{:id (str (:id c))
|
||||||
[:div {:class (stl/css :library-resolution-item-name)}
|
:label (:name c)})
|
||||||
name]
|
candidates)
|
||||||
[:> select* {:options options
|
selected (get selections id)]
|
||||||
:default-selected (or (some-> selected str) "")
|
[:div {:class (stl/css :library-resolution-item)
|
||||||
:on-change (partial on-select id)}]]))])))
|
:key (dm/str id)}
|
||||||
|
[:div {:class (stl/css :library-resolution-item-name)}
|
||||||
|
name]
|
||||||
|
[:> select* {:options options
|
||||||
|
:default-selected (or (some-> selected str) "")
|
||||||
|
:on-change (partial on-select id)}]]))]))))
|
||||||
|
|
||||||
(mf/defc import-dialog
|
(mf/defc import-dialog
|
||||||
{::mf/register modal/components
|
{::mf/register modal/components
|
||||||
@ -586,8 +594,7 @@
|
|||||||
{:level :success
|
{:level :success
|
||||||
:content (tr "dashboard.import.auto-linked-libraries" (i18n/c auto-linked-count))}])
|
:content (tr "dashboard.import.auto-linked-libraries" (i18n/c auto-linked-count))}])
|
||||||
[:> library-resolution* {:resolution library-resolution
|
[:> library-resolution* {:resolution library-resolution
|
||||||
:selections* library-selections*
|
:selections* library-selections*}]])
|
||||||
:file-ids (:file-ids library-resolution)}]])
|
|
||||||
|
|
||||||
(if (or (= :import-error status) (and (= :analyze status) errors?))
|
(if (or (= :import-error status) (and (= :analyze status) errors?))
|
||||||
[:div {:class (stl/css :import-error-disclaimer)}
|
[:div {:class (stl/css :import-error-disclaimer)}
|
||||||
|
|||||||
File diff suppressed because it is too large
Load Diff
@ -756,12 +756,6 @@ msgstr[1] "%s bibliotecas fueron vinculadas automáticamente por nombre."
|
|||||||
msgid "dashboard.import.resolve-libraries"
|
msgid "dashboard.import.resolve-libraries"
|
||||||
msgstr "Algunas bibliotecas no pudieron vincularse automáticamente. Selecciona la biblioteca correcta para cada una:"
|
msgstr "Algunas bibliotecas no pudieron vincularse automáticamente. Selecciona la biblioteca correcta para cada una:"
|
||||||
|
|
||||||
msgid "dashboard.import.library-placeholder"
|
|
||||||
msgstr "Selecciona una biblioteca…"
|
|
||||||
|
|
||||||
msgid "dashboard.import.library-link-error"
|
|
||||||
msgstr "No se pudo vincular la biblioteca"
|
|
||||||
|
|
||||||
msgid "dashboard.import.confirm-library-links"
|
msgid "dashboard.import.confirm-library-links"
|
||||||
msgstr "Confirmar vínculos de biblioteca"
|
msgstr "Confirmar vínculos de biblioteca"
|
||||||
|
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user