From 8c4ab7e5cf67ac94a683835da520de3279ec2aa5 Mon Sep 17 00:00:00 2001
From: torkus <48790775+torkus@users.noreply.github.com>
Date: Sun, 1 Sep 2024 10:41:09 +0930
Subject: [PATCH] install addon from file, preserve 'ignore' and 'pin' flags
 (#434)

* cli, adds test 'install-addon--ignore-then-update-from-file'
  - it recreates what is being seen in issue #433
* core.clj, download-time tests duplicated at install-time.
  - but with different consequences.
* cli.clj, installing an addon from a file now goes through 'core', not 'addon'.
* core, relaxes restriction on what core/install-addon considers installable.
* addon.clj, special handling for nfo flags when installing addon.
  - if an addon is ignored then the new nfo data will inherit the ignore flag.
  - if an addon is pinned then thew new nfo data will remove the pinned-version flag.
  - it shouldn't be possible to reach this logic right now, core/install-addon should prevent that.
* core.clj, install-addon, replaced 'test-only?' parameter with map of options.
* addon.clj, fixed bug where ignore check fails on single dir addons
  - it assumed all addons had a :group-addons field, but that only exists for addons that have been grouped because there are more than one of them.
  - pretty serious bug.
* core/install-addon, adds option to bypass ignore and pinned flags
* core/remove-many-addons, now refuses to remove ignored addons
* addon/remove-addon, just warns when ignored addon is being removed
* cli/install-addons-from-file, now accepts a map of installation options
* jfx/zip-file-picker, changed default behaviour when installing from file
  - ignored addons are now overwritten
  - pinned addons are now unpinned
* review feedback
* CHANGELOG
---
 CHANGELOG.md                  |  11 +++
 TODO.md                       |  37 ++++++++
 src/strongbox/addon.clj       |  51 ++++++++--
 src/strongbox/cli.clj         |  11 ++-
 src/strongbox/core.clj        |  82 +++++++++++-----
 src/strongbox/jfx.clj         |  10 +-
 src/strongbox/nfo.clj         |  10 ++
 src/strongbox/specs.clj       |   9 ++
 test/strongbox/addon_test.clj |  38 +++++++-
 test/strongbox/cli_test.clj   | 174 +++++++++++++++++++---------------
 test/strongbox/core_test.clj  |  45 ++++-----
 test/strongbox/nfo_test.clj   |  31 +++++-
 12 files changed, 366 insertions(+), 143 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4d3ac91b..97f15112 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,12 +8,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+* error handling for a few cases where uncaught exceptions were being swallowed.
+
 ### Changed
 
+* installing an addon by file now overwrites/updates ignored addons and unpins pinned addons.
+    - this accommodates a use case where Strongbox is used to install a select few addons manually from unsupported sources (like Curseforge) but then the new addons match against older versions in the catalogue from wowi.
+    - please open a feature request if you want this behaviour adjustable.
+
 ### Fixed
 
 * fixed bug in emergency 'short' catalogue generation.
     - it would include addons from BFA instead of stopping at Shadowlands.
+* fixed bug where installing a file manually would bypass ignored and pinned addon checks.
+    - it would tell you it was refusing to install, then install it anyway.
+* fixed bug where ignore check bypassed on ungrouped/single directory addons.
+    - logic assumed all addons were grouped, but grouping only happens when an addon unzip to multiple directories.
+    - pretty big bug!
 
 ### Removed
 
diff --git a/TODO.md b/TODO.md
index 1e0bc7ff..a5f0100e 100644
--- a/TODO.md
+++ b/TODO.md
@@ -8,8 +8,45 @@ see CHANGELOG.md for a more formal list of changes by release
 
 ## todo
 
+* issue 433: preserve ignore and pin flags when addon updated manually via strongbox
+    - usecase: 
+        - old addon exists on wowi
+        - new addon exists on curse
+        - user installs new addon from curse and then 'ignores' it so nothing affects it
+        - user installs updated addon from curse and ignore flag is lost
+    - problems:
+        - if the addon was ignored, it shouldn't have been possible to overwrite it to begin with
+        - it seems reasonable to update addons in this way
+
+bug:
+    - attempting to overwrite ignored addon results in "refusing to delete ignored addon: /path/to/addon/dir" with no addon name
+    - it looks like it installed it anyway
+    
+
+* Desktop Entry Name should not include "(Flatpak)" 
+    - https://github.com/flathub/la.ogri.strongbox/issues/7
+
 ## todo bucket (no particular order)
 
+* gui, when installing an addon from file, allow the following options
+    - test-only?
+    - unpin-pinned?
+    - overwrite-ignored?
+
+bug: clear button isn't clearing search
+
+bug: 
+    select github + wowi
+    search for 'alto'
+        - 3 results, 2 github, 1 wowi
+    deselect 'github'
+        - no results
+    select gitlab (so wowi and gitlab are selected)
+        - wowi result
+
+bug: search, next and previous buttons should take me to top of search results
+
+
 * patch tooltip, support multiple game versions
 
 * flathub, remove '(Flathub)' from .desktop file
diff --git a/src/strongbox/addon.clj b/src/strongbox/addon.clj
index 39a96734..40ca6d77 100644
--- a/src/strongbox/addon.clj
+++ b/src/strongbox/addon.clj
@@ -79,10 +79,13 @@
   (if (:ignore? addon)
     ;; if addon is being ignored, refuse to remove addon.
     ;; note: `group-addons` will add a top level `:ignore?` flag if any addon in a bundle is being ignored.
-    (error "refusing to delete ignored addon:" install-dir)
+    ;; 2024-08-25: behaviour changed. this is not the place to prevent ignored addons from being removed.
+    ;; see `core/install-addon`, `core/remove-many-addons`
+    ;;(error "refusing to delete ignored addon:" (:label addon))
+    (warn "deleting ignored addon:" (:label addon)))
 
-    (doseq [grouped-addon (flatten-addon addon)]
-      (-remove-addon! install-dir (:dirname grouped-addon) (:group-id addon)))))
+  (doseq [grouped-addon (flatten-addon addon)]
+    (-remove-addon! install-dir (:dirname grouped-addon) (:group-id addon))))
 
 ;;
 
@@ -276,8 +279,13 @@
 (defn-spec install-addon (s/or :ok (s/coll-of ::sp/extant-file), :error ::sp/empty-coll)
   "installs an addon given an addon description, a place to install the addon and the addon zip file itself.
   handles suspicious looking bundles, conflicts with other addons, uninstalling previous addon version and updating nfo files.
-  returns a list of nfo files that were written to disk."
-  [addon :addon/nfo-input-minimum, install-dir ::sp/writeable-dir, downloaded-file ::sp/archive-file]
+
+  relies on `core/install-addon` to block installations that would overwrite ignored or pinned addons.
+  if it's gotten this far ignored/pinned addons will be deleted
+  and the new addon will be unzipped over the top.
+
+  returns a list of nfo files that were written to disk, if any."
+  [addon :addon/nfo-input-minimum, install-dir ::sp/writeable-dir, downloaded-file ::sp/archive-file, opts ::sp/install-opts]
   (let [nom (or (:label addon) (:name addon) (fs/base-name downloaded-file))
         version (:version addon)
 
@@ -288,6 +296,15 @@
 
         zipfile-entries (zip/zipfile-normal-entries downloaded-file)
         toplevel-dirs (zip/top-level-directories zipfile-entries)
+
+        toplevel-nfo (->> toplevel-dirs ;; [{:path "EveryAddon/", ...}, ...]
+                          (map :path) ;; ["EveryAddon/", ...]
+                          (map fs/base-name) ;; ["EveryAddon", ...]
+                          (map #(nfo/read-nfo-file install-dir %))) ;; [`(read-nfo-file install-dir "EveryAddon"), ...]
+
+        contains-nfo-with-ignored-flag (utils/any (map :ignore? toplevel-nfo))
+        contains-nfo-with-pinned-version (utils/any (map :pinned-version toplevel-nfo))
+
         primary-dirname (determine-primary-subdir toplevel-dirs)
 
         ;; let the user know if there are bundled addons and they don't share a common prefix
@@ -306,10 +323,26 @@
                         (let [addon-dirname (:path zipentry)
                               primary? (= addon-dirname (:path primary-dirname))
                               new-nfo-data (nfo/derive addon primary?)
+                              ;; if any of the addons this addon is replacing are being ignored,
+                              ;; the new nfo will be ignored too.
+                              new-nfo-data (if contains-nfo-with-ignored-flag
+                                             (nfo/ignore new-nfo-data)
+                                             new-nfo-data)
+
+                              new-nfo-data (if contains-nfo-with-pinned-version
+                                             (nfo/unpin new-nfo-data)
+                                             new-nfo-data)
+
                               new-nfo-data (nfo/add-nfo install-dir addon-dirname new-nfo-data)]
                           (nfo/write-nfo! install-dir addon-dirname new-nfo-data)))
 
         ;; write the nfo files, return a list of all nfo files written
+        ;; todo: if a zip file is being installed then we can't rely on `remove-addon!` having been called,
+        ;; but `remove-completely-overwritten-addons` will have been called and *may* have removed the
+        ;; addon *if* the new addon is a superset of the old one.
+        ;; this leads to the possibility of a new addon that has dropped a subdir or added a new one (like a rename)
+        ;; being skipped and orphaning the original subdir.
+        ;; this means we could hit `unzip-addon` with the original addon still fully intact.
         update-nfo-files (fn []
                            (mapv update-nfo-fn toplevel-dirs))
 
@@ -343,12 +376,16 @@
     (suspicious-bundle-check)
 
     ;; todo: remove support for v1 addons in 2.0.0 ;; todo!
-    ;; when is it not valid? when importing v1 addons. v2 addons need 'padding' as well :(
+    ;; when is it not valid?
+    ;; * when importing v1 addons. v2 addons need 'padding' as well :(
+    ;; * when installing from a file and we have nothing more than a generated ID value
     (when (s/valid? :addon/toc addon)
       (remove-addon! install-dir addon))
 
     (remove-completely-overwritten-addons)
 
+    ;; `addon/install-addon` is all about installing an addon, not checking whether it's safe to do so.
+    ;; use `core/install-addon` for safety checks.
     (unzip-addon)
     (update-nfo-files)))
 
@@ -388,7 +425,7 @@
 (defn-spec ignored-dir-list (s/coll-of ::sp/dirname)
   "returns a list of unique addon directory names (including grouped addons) that are not being ignored"
   [addon-list (s/nilable :addon/installed-list)]
-  (->> addon-list (filter :ignore?) (map :group-addons) flatten (map :dirname) (remove nil?) set))
+  (->> addon-list (filter :ignore?) (map flatten-addon) flatten (map :dirname) (remove nil?) set))
 
 (defn-spec overwrites-ignored? boolean?
   "returns `true` if given archive file would unpack over *any* ignored addon.
diff --git a/src/strongbox/cli.clj b/src/strongbox/cli.clj
index a9a6705f..bef16a3f 100644
--- a/src/strongbox/cli.clj
+++ b/src/strongbox/cli.clj
@@ -312,10 +312,11 @@
                  (let [downloaded-file (core/download-addon-guard-affective addon install-dir)
                        existing-dirs (addon/dirname-set addon)
                        updated-dirs (zipfile-locks downloaded-file)
-                       locks-needed (clojure.set/union existing-dirs updated-dirs)]
+                       locks-needed (clojure.set/union existing-dirs updated-dirs)
+                       opts {}]
                    (swap! new-dirs into updated-dirs)
                    (utils/with-lock current-locks locks-needed
-                     (core/install-addon-affective addon install-dir downloaded-file)
+                     (core/install-addon-guard-affective addon install-dir opts downloaded-file)
                      (core/refresh-addon addon))))]
     (run! #(joblib/create-addon-job! queue-atm % job-fn) updateable-addon-list)
     (joblib/run-jobs! queue-atm core/num-concurrent-downloads)
@@ -363,7 +364,7 @@
           error-messages
           (logging/buffered-log
            :warn
-           (addon/install-addon addon (core/selected-addon-dir) downloaded-file))]
+           (core/install-addon addon (core/selected-addon-dir) downloaded-file))]
       (core/refresh)
       {:label (fs/base-name downloaded-file)
        :error-messages error-messages}))
@@ -372,7 +373,7 @@
   "installs/updates a list of addon zip files in parallel.
   does a clever refresh check afterwards to try and prevent a full refresh from happening.
   very similar code to `install-update-these-in-parallel`."
-  [download-file-list (s/coll-of ::sp/extant-archive-file)]
+  [download-file-list (s/coll-of ::sp/extant-archive-file), opts ::sp/install-opts]
   (let [queue-atm (core/get-state :job-queue)
         install-dir (core/selected-addon-dir)
         current-locks (atom #{})
@@ -389,7 +390,7 @@
                      (let [error-messages
                            (logging/buffered-log
                             :warn
-                            (let [results (addon/install-addon addon install-dir downloaded-file)]
+                            (let [results (core/install-addon addon install-dir downloaded-file opts)]
                               (when-let [installed-addon-dir (some-> results first fs/parent str)]
                                 (core/refresh-addon* installed-addon-dir))))]
 
diff --git a/src/strongbox/core.clj b/src/strongbox/core.clj
index 0d036685..4f802711 100644
--- a/src/strongbox/core.clj
+++ b/src/strongbox/core.clj
@@ -490,6 +490,8 @@
       (try
         (logging/with-addon addon
           (apply wrapped-fn addon args))
+        (catch Exception ex
+          (error ex "uncaught error calling wrapped function"))
         (finally
           (-stop-affecting-addon))))))
 
@@ -536,9 +538,11 @@
             (fs/delete downloaded-file)
             (warn "removed bad addon."))
 
+        ;; test is duplicated in `install-addon` as it's possible to just download and check addon without installing it.
         (addon/overwrites-ignored? downloaded-file (get-state :installed-addon-list))
         (error "refusing to install addon that will overwrite an ignored addon.")
 
+        ;; test is duplicated in `install-addon` as it's possible to just download and check addon without installing it.
         (addon/overwrites-pinned? downloaded-file (get-state :installed-addon-list))
         (error "refusing to install addon that will overwrite a pinned addon.")
 
@@ -549,31 +553,66 @@
 
 (defn-spec install-addon (s/or :ok (s/coll-of ::sp/extant-file), :passed-tests true?, :error nil?)
   "downloads an addon and installs it, bypassing checks. see `install-addon-guard`."
-  [addon :addon/installable, install-dir ::sp/extant-dir, downloaded-file (s/nilable ::sp/extant-archive-file)]
+  [addon (s/or :ok :addon/installable, :also-ok :addon/nfo-input-minimum), install-dir ::sp/extant-dir, downloaded-file (s/nilable ::sp/extant-archive-file), opts ::sp/install-opts]
   (when downloaded-file
     (try
-      (addon/install-addon addon install-dir downloaded-file)
+
+      ;; because addons can enter strongbox via downloading or via the user,
+      ;; we can't rely on all checks having happened at download time.
+      ;; this means some checks will be duplicated for downloaded files
+      ;; with different consequences for failing.
+      ;; for example, if the addon is invalid at download time, delete it.
+      ;; if the addon is invalid at install time, ignore it.
+
+      (cond
+        (not (zip/valid-zip-file? downloaded-file))
+        (error "failed to read addon zip file, possibly corrupt or not a zip file.")
+
+        (not (zip/valid-addon-zip-file? downloaded-file))
+        (error "refusing to install, addon zip file contains top-level files or a top-level directory missing a .toc file.")
+
+        (and (addon/overwrites-ignored? downloaded-file (get-state :installed-addon-list))
+             (not (:overwrite-ignored? opts)))
+        (error "refusing to install addon that will overwrite an ignored addon.")
+
+        (and (addon/overwrites-pinned? downloaded-file (get-state :installed-addon-list))
+             (not (:unpin-pinned? opts)))
+        (error "refusing to install addon that will overwrite a pinned addon.")
+
+        :else (addon/install-addon addon install-dir downloaded-file opts))
+
+      (catch Exception ex
+        (error ex "Uncaught exception installing addon"))
+
       (finally
-        (addon/post-install addon install-dir (get-state :cfg :preferences :addon-zips-to-keep))))))
+        ;; future: post-install steps for addons installed manually are skipped because there is no `:name` value,
+        ;; only a `grouped-id` value.
+        (when (:name addon)
+          (addon/post-install addon install-dir (get-state :cfg :preferences :addon-zips-to-keep)))))))
 
-(def install-addon-affective
-  (affects-addon-wrapper install-addon))
+;; 2024-09-01: disabled, I want all installation to go through `install-addon-guard`
+#_(def install-addon-affective
+    (affects-addon-wrapper install-addon))
 
 (defn-spec install-addon-guard (s/or :ok (s/coll-of ::sp/extant-file), :passed-tests true?, :error nil?)
   "downloads an addon and installs it, handling http and non-http errors, bad zip files, bad addons, bad directories."
   ([addon :addon/installable]
-   (install-addon-guard addon (selected-addon-dir) false))
+   (install-addon-guard addon (selected-addon-dir) {}))
 
   ([addon :addon/installable, install-dir ::sp/extant-dir]
-   (install-addon-guard addon install-dir false))
+   (install-addon-guard addon install-dir {}))
+
+  ([addon :addon/installable, install-dir ::sp/extant-dir, opts ::sp/install-opts]
+   (install-addon-guard addon install-dir opts (download-addon-guard addon install-dir)))
 
-  ([addon :addon/installable, install-dir ::sp/extant-dir, test-only? boolean?]
-   (when-let [downloaded-file (download-addon-guard addon install-dir)]
-     (if test-only?
-       ;; addon was successfully downloaded and verified as being sound. stop here.
-       true
-       ;; else, install addon
-       (install-addon addon install-dir downloaded-file)))))
+  ([addon :addon/installable, install-dir ::sp/extant-dir, opts ::sp/install-opts, downloaded-file (s/nilable ::sp/extant-archive-file)]
+   (let [{:keys [test-only?]} opts]
+     (when downloaded-file
+       (if test-only?
+         ;; addon was successfully downloaded and verified as being sound. stop here.
+         true
+         ;; else, install addon
+         (install-addon addon install-dir downloaded-file opts))))))
 
 (def install-addon-guard-affective
   (affects-addon-wrapper install-addon-guard))
@@ -1251,7 +1290,7 @@
               ;; not necessary when updating the user-catalogue.
               _ (if-not attempt-dry-run?
                   true
-                  (or (install-addon-guard addon (selected-addon-dir) true)
+                  (or (install-addon-guard addon (selected-addon-dir) {:test-only? true})
                       (error "failed dry-run installation")))]
 
              ;; if-let* was successful!
@@ -1739,22 +1778,15 @@
       ;; todo: could this be a half-refresh instead?
       (refresh))))
 
-;; todo: move to ui.cli
-#_(defn-spec remove-addon nil?
-    "removes given installed addon"
-    [installed-addon :addon/installed]
-    (logging/with-addon installed-addon
-      (addon/remove-addon! (selected-addon-dir) installed-addon))
-    (refresh))
-
-;; todo: move to ui.cli
 (defn-spec remove-many-addons nil?
   "deletes each of the addons in the given `toc-list` and then calls `refresh`"
   [installed-addon-list :addon/toc-list]
   (let [addon-dir (selected-addon-dir)]
     (doseq [installed-addon installed-addon-list]
       (logging/with-addon installed-addon
-        (addon/remove-addon! addon-dir installed-addon)))
+        (if (addon/ignored? installed-addon)
+          (error "refusing to delete ignored addon:" (:label installed-addon))
+          (addon/remove-addon! addon-dir installed-addon))))
     (refresh)))
 
 ;;
diff --git a/src/strongbox/jfx.clj b/src/strongbox/jfx.clj
index 152acb86..8f27f5e3 100644
--- a/src/strongbox/jfx.clj
+++ b/src/strongbox/jfx.clj
@@ -1007,10 +1007,12 @@
   (when-let [abs-path-list (file-chooser {:filters [{:description "ZIP files" :extensions ["*.zip"]}]
                                           :type :open-multi
                                           :initial-dir (core/selected-addon-dir)})]
-    (doseq [{:keys [error-messages label]} (cli/install-addons-from-file-in-parallel abs-path-list)]
-      (when-not (empty? error-messages)
-        (let [msg (message-list (format "warnings/errors while installing \"%s\"" label) error-messages)]
-          (alert :warning msg {:wait? false})))))
+    (let [opts {:overwrite-ignored? true
+                :unpin-pinned? true}]
+      (doseq [{:keys [error-messages label]} (cli/install-addons-from-file-in-parallel abs-path-list opts)]
+        (when-not (empty? error-messages)
+          (let [msg (message-list (format "warnings/errors while installing \"%s\"" label) error-messages)]
+            (alert :warning msg {:wait? false}))))))
   nil)
 
 (defn exit-handler
diff --git a/src/strongbox/nfo.clj b/src/strongbox/nfo.clj
index 107954e3..b92e6e85 100644
--- a/src/strongbox/nfo.clj
+++ b/src/strongbox/nfo.clj
@@ -261,6 +261,11 @@
 
 ;; ignoring
 
+(defn-spec ignore :addon/nfo
+  "add a `ignore?` flag to given `nfo` data."
+  [nfo :addon/nfo]
+  (assoc nfo :ignore? true))
+
 (defn-spec ignore! nil?
   "prevent any changes made by strongbox to this addon. 
   explicitly ignores this addon by setting the `ignore?` flag to `true`."
@@ -286,6 +291,11 @@
   [install-dir ::sp/extant-dir, addon-dirname ::sp/dirname, version :addon/pinned-version]
   (update-nfo! install-dir addon-dirname {:pinned-version version}))
 
+(defn-spec unpin :addon/nfo
+  "remove pin flag from given `nfo` data"
+  [nfo :addon/nfo]
+  (dissoc nfo :pinned-version))
+
 (defn-spec unpin! nil?
   "removes `:pinned-version` from a specific addon's nfo file, if it exists"
   [install-dir ::sp/extant-dir, addon-dirname ::sp/dirname]
diff --git a/src/strongbox/specs.clj b/src/strongbox/specs.clj
index 0e3e0e1a..336fbb1e 100644
--- a/src/strongbox/specs.clj
+++ b/src/strongbox/specs.clj
@@ -503,3 +503,12 @@
                                             :github/requests-limit-reset-minutes
                                             :github/requests-remaining
                                             :github/requests-used]))
+
+;; addon/install-addon
+
+(s/def ::test-only? boolean?)
+(s/def ::overwrite-ignored? boolean?)
+(s/def ::unpin-pinned? boolean?)
+(s/def ::install-opts (s/keys :opt-un [::test-only?
+                                       ::overwrite-ignored?
+                                       ::unpin-pinned?]))
diff --git a/test/strongbox/addon_test.clj b/test/strongbox/addon_test.clj
index ee6cef1e..a8e1a46f 100644
--- a/test/strongbox/addon_test.clj
+++ b/test/strongbox/addon_test.clj
@@ -405,6 +405,22 @@
           (is (= expected (helper/install-dir-contents)))
           (is (clojure.string/starts-with? error-message error-prefix)))))))
 
+(deftest remove-addon--ignored-addon
+  (testing "removing an ignored addon with addon/remove-addon! emits a warning but otherwise removes addon"
+    (let [install-dir (helper/install-dir)
+          addon {:name "nom" :label "Nom" :description ""
+                 :interface-version-list [90100]
+                 :installed-version "0.1"
+                 :supported-game-tracks [:retail]
+                 :dirname "./EveryAddon"
+                 :ignore? true}
+          _ (fs/mkdir (utils/join install-dir "EveryAddon"))
+          messages (logging/buffered-log
+                    :warn
+                    (addon/remove-addon! install-dir addon))
+          expected-messages ["deleting ignored addon: Nom"]]
+      (is (= expected-messages messages)))))
+
 ;;
 
 (deftest test-pinned-dir-list
@@ -490,6 +506,22 @@
       (is (addon/overwrites-pinned? downloaded-file [pinned-addon]))
       (is (not (addon/overwrites-pinned? downloaded-file [addon]))))))
 
+(deftest test-overwrites-ignored?
+  (testing "addon zip files that would extract over an ignored addon are correctly detected"
+    (let [downloaded-file (fixture-path "everyaddon--1-2-3.zip") ;; ./EveryAddon
+          addon {:name "EveryAddon",
+                 :dirname "EveryAddon",
+                 :label "Every Addon",
+                 :description ""
+                 :interface-version-list [80300]
+                 :installed-version "1.2.3"
+                 :supported-game-tracks [:retail]
+                 :group-id "foo"
+                 :primary? true}
+          ignored-addon (assoc addon :ignore? true)]
+      (is (addon/overwrites-ignored? downloaded-file [ignored-addon]))
+      (is (not (addon/overwrites-pinned? downloaded-file [addon]))))))
+
 (deftest test-updateable?
   (testing "an addon's 'updateable' states"
     (let [cases [;; no update available
@@ -784,9 +816,11 @@
                        :source-map-list [{:source "curseforge", :source-id 2}]}]
 
         updates {:group-id "foobar"}
-        expected-nfo (mapv #(merge % updates) original-nfo)]
+        expected-nfo (mapv #(merge % updates) original-nfo)
+
+        opts {}]
 
-    (addon/install-addon addon (helper/install-dir) zipfile)
+    (addon/install-addon addon (helper/install-dir) zipfile opts)
 
     ;; sanity checks
     (is (= ["EveryAddon-BundledAddon" "EveryOtherAddon"] (helper/install-dir-contents)))
diff --git a/test/strongbox/cli_test.clj b/test/strongbox/cli_test.clj
index 9b4464e7..761cd170 100644
--- a/test/strongbox/cli_test.clj
+++ b/test/strongbox/cli_test.clj
@@ -4,7 +4,8 @@
    [strongbox.cli :as cli]
    [clj-http.fake :refer [with-global-fake-routes-in-isolation]]
    [strongbox
-    ;;[nfo :as nfo]
+    [nfo :as nfo]
+    [addon :as addon]
     [specs :as specs]
     [utils :as utils]
     [logging :as logging]
@@ -905,87 +906,108 @@
         expected #{"EveryAddon" "EveryAddon-BundledAddon"}]
     (is (= expected (cli/zipfile-locks zipfile-fixture)))))
 
-#_(deftest install-addon-from-file
-    (testing "an addon can be installed from a zip file."
-      (with-redefs [utils/unique-id (constantly "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee")]
+(deftest install-addon-from-file-in-parallel
+  (testing "an addon can be installed from a zip file."
+    (with-redefs [utils/unique-id (constantly "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee")]
+      (with-running-app
+        (let [install-dir (helper/install-dir)
+              fixture (helper/fixture-path "everyaddon--0-1-2.zip")
+              opts {}
+              expected [{:description "Does what no other addon does, slightly differently",
+                         :dirname "EveryAddon",
+                         :dirsize 0
+                         :group-addons [{:description "Does what no other addon does, slightly differently",
+                                         :dirname "EveryAddon",
+                                         :dirsize 0
+                                         :group-id "everyaddon-aaaaaaaa",
+                                         :installed-version "1.2.3",
+                                         :interface-version-list [70000],
+                                         :label "EveryAddon 1.2.3",
+                                         :name "everyaddon",
+                                         :primary? true,
+                                         :supported-game-tracks [:retail]}
+                                        {:description "A useful addon that everyone bundles with their own.",
+                                         :dirname "EveryAddon-BundledAddon",
+                                         :dirsize 0
+                                         :group-id "everyaddon-aaaaaaaa",
+                                         :installed-version "a.b.c",
+                                         :interface-version-list [80000],
+                                         :label "BundledAddon a.b.c",
+                                         :name "bundledaddon-a-b-c",
+                                         :primary? false,
+                                         :supported-game-tracks [:retail]}],
+                         :group-id "everyaddon-aaaaaaaa",
+                         :installed-version "1.2.3",
+                         :interface-version-list [70000],
+                         :label "EveryAddon 1.2.3",
+                         :name "everyaddon",
+                         :primary? true,
+                         :supported-game-tracks [:retail]}]
+
+              expected-nfo {:group-id "everyaddon-aaaaaaaa", :primary? true}]
+          (cli/install-addons-from-file-in-parallel [fixture] opts)
+          (is (= expected (core/get-state :installed-addon-list)))
+          (is (= expected-nfo (nfo/read-nfo-file install-dir "EveryAddon"))))))))
+
+(deftest install-addon-from-file--then-update
+  (testing "an addon can be installed from a zip file."
+    (let [fixture (helper/fixture-path "everyaddon--0-1-2.zip")
+          dummy-catalogue (slurp (fixture-path "catalogue--v2--everyaddon.json"))
+          dummy-host-response (slurp (fixture-path "everyaddon--wowinterface--detail.json"))
+          update-fixture (fixture-path "everyaddon--7-8-9.zip")
+
+          fake-routes {"https://raw.githubusercontent.com/ogri-la/strongbox-catalogue/master/short-catalogue.json"
+                       {:get (fn [req] {:status 200 :body dummy-catalogue})}
+
+                       "https://api.mmoui.com/v3/game/WOW/filedetails/1.json"
+                       {:get (fn [req] {:status 200 :body dummy-host-response})}
+
+                       "https://cdn.wowinterface.com/downloads/getfile.php?id=1"
+                       {:get (fn [req] {:status 200 :body (helper/file-to-lazy-byte-array update-fixture)})}}
+
+          expected-nfo {:source "wowinterface",
+                        :source-id 1,
+                        :group-id "https://www.wowinterface.com/downloads/info1",
+
+                        :installed-game-track :retail,
+                        :installed-version "7.8.9",
+                        :name "everyaddon",
+                        :primary? true,
+                        :source-map-list [{:source "wowinterface", :source-id 1}]}]
+
+      (with-global-fake-routes-in-isolation fake-routes
         (with-running-app
           (let [install-dir (helper/install-dir)
-                fixture (helper/fixture-path "everyaddon--0-1-2.zip")
-                expected [{:description "Does what no other addon does, slightly differently",
-                           :dirname "EveryAddon",
-                           :group-addons [{:description "Does what no other addon does, slightly differently",
-                                           :dirname "EveryAddon",
-                                           :group-id "everyaddon-aaaaaaaa",
-                                           :installed-version "1.2.3",
-                                           :interface-version-list [70000],
-                                           :label "EveryAddon 1.2.3",
-                                           :name "everyaddon",
-                                           :primary? true,
-                                           :supported-game-tracks [:retail]}
-                                          {:description "A useful addon that everyone bundles with their own.",
-                                           :dirname "EveryAddon-BundledAddon",
-                                           :group-id "everyaddon-aaaaaaaa",
-                                           :installed-version "a.b.c",
-                                           :interface-version-list [80000],
-                                           :label "BundledAddon a.b.c",
-                                           :name "bundledaddon-a-b-c",
-                                           :primary? false,
-                                           :supported-game-tracks [:retail]}],
-                           :group-id "everyaddon-aaaaaaaa",
-                           :installed-version "1.2.3",
-                           :interface-version-list [70000],
-                           :label "EveryAddon 1.2.3",
-                           :name "everyaddon",
-                           :primary? true,
-                           :supported-game-tracks [:retail],
-                           :update? false}]
-
-                expected-nfo {:group-id "everyaddon-aaaaaaaa", :primary? true}]
-            (cli/install-addon-from-file fixture)
-            (is (= expected (core/get-state :installed-addon-list)))
-            (is (= expected-nfo (nfo/read-nfo-file install-dir "EveryAddon"))))))))
-
-#_(deftest install-addon-from-file--then-update
-    (testing "an addon can be installed from a zip file."
-      (let [fixture (helper/fixture-path "everyaddon--0-1-2.zip")
-            dummy-catalogue (slurp (fixture-path "catalogue--v2--everyaddon.json"))
-            dummy-host-response (slurp (fixture-path "everyaddon--wowinterface--detail.json"))
-            update-fixture (fixture-path "everyaddon--7-8-9.zip")
-
-            fake-routes {"https://raw.githubusercontent.com/ogri-la/strongbox-catalogue/master/short-catalogue.json"
-                         {:get (fn [req] {:status 200 :body dummy-catalogue})}
-
-                         "https://api.mmoui.com/v3/game/WOW/filedetails/1.json"
-                         {:get (fn [req] {:status 200 :body dummy-host-response})}
-
-                         "https://cdn.wowinterface.com/downloads/getfile.php?id=1"
-                         {:get (fn [req] {:status 200 :body (helper/file-to-lazy-byte-array update-fixture)})}}
-
-          ;;expected []
-            expected-nfo {:source "wowinterface",
-                          :source-id 1,
-                          :group-id "https://www.wowinterface.com/downloads/info1",
-
-                          :installed-game-track :retail,
-                          :installed-version "7.8.9",
-                          :name "everyaddon",
-                          :primary? true,
-                          :source-map-list [{:source "wowinterface", :source-id 1}]}]
+                opts {}
+                _ (cli/install-addons-from-file-in-parallel [fixture] opts)
+                _ (core/refresh)
+                addon (first (core/get-state :installed-addon-list))]
 
-        (with-global-fake-routes-in-isolation fake-routes
-          (with-running-app
-            (let [install-dir (helper/install-dir)
-                  _ (cli/install-addon-from-file fixture)
-                  _ (core/refresh)
-                  addon (first (core/get-state :installed-addon-list))]
+            (is (:matched? addon))
+            (is (:update? addon))
 
-              (is (:matched? addon))
-              (is (:update? addon))
+            (cli/install-update-these-serially [addon])
+            (core/refresh)
 
-              (cli/install-update-these-serially [addon])
-              (core/refresh)
+            (is (= expected-nfo (nfo/read-nfo-file install-dir "EveryAddon")))))))))
 
-              (is (= expected-nfo (nfo/read-nfo-file install-dir "EveryAddon")))))))))
+(deftest install-addon--ignore-then-update-from-file
+  (testing "an addon installed+updated from a zip file respects ignore and pin flags."
+    (with-running-app
+      (helper/install-every-addon!)
+      (core/refresh)
+      (let [addon (first (core/get-state :installed-addon-list))]
+        (addon/ignore! (helper/install-dir) addon)
+        (core/refresh)
+        (let [addon (first (core/get-state :installed-addon-list))
+              update (helper/fixture-path "everyaddon--7-8-9.zip")
+              opts {}]
+
+          (is (:ignore? addon))
+
+          (cli/install-addons-from-file-in-parallel [update] opts)
+          (let [addon (first (core/get-state :installed-addon-list))]
+            (is (:ignore? addon))))))))
 
 (deftest unique-group-id-from-zip-file
   (let [cases [["/foo/bar/baz.zip" "baz-aaaaaaaa"]
diff --git a/test/strongbox/core_test.clj b/test/strongbox/core_test.clj
index a4af477a..415b3c3c 100644
--- a/test/strongbox/core_test.clj
+++ b/test/strongbox/core_test.clj
@@ -568,8 +568,7 @@
             ;; move dummy addon file into place so there is no cache miss
             fname (downloaded-addon-fname (:name helper/addon) (:version helper/addon))
             _ (helper/cp (fixture-path fname) install-dir)
-            test-only? false
-            file-list (core/install-addon-guard helper/addon install-dir test-only?)]
+            file-list (core/install-addon-guard helper/addon install-dir {:test-only? false})]
         (testing "addon directory created, single file written (.strongbox.json nfo file)"
           (is (= (count file-list) 1))
           (is (fs/exists? (first file-list))))))))
@@ -581,8 +580,7 @@
             fname (downloaded-addon-fname (:name helper/addon) (:version helper/addon))
             dest (helper/cp (fixture-path fname) install-dir)
             addon (assoc helper/addon :-testing-zipfile dest)
-            test-only? true
-            result (core/install-addon-guard addon install-dir test-only?)]
+            result (core/install-addon-guard addon install-dir {:test-only? true})]
         (is result) ;; success
 
         ;; ensure nothing was actually unzipped
@@ -596,26 +594,26 @@
             fname (downloaded-addon-fname (:name helper/addon) (:version helper/addon))
             _ (fs/copy (fixture-path "bad-truncated.zip") (utils/join install-dir fname))
 
-            test-only? true
-            result (core/install-addon-guard helper/addon install-dir test-only?)]
+            result (core/install-addon-guard helper/addon install-dir {:test-only? true})]
         (is (not result)) ;; failure
 
         ;; ensure nothing was actually unzipped
         (is (not (fs/exists? (utils/join install-dir "EveryAddon"))))))))
 
-(deftest install-bad-addon
-  (testing "installing a bad addon"
+(deftest install-addon-guard--invalid-zip-file
+  (testing "installing a corrupted zip file"
     (with-global-fake-routes-in-isolation {}
       (let [install-dir (str fs/*cwd*)
             fname (downloaded-addon-fname (:name helper/addon) (:version helper/addon))
             dest (utils/join install-dir fname)
             _ (fs/copy (fixture-path "bad-truncated.zip") dest)
-            addon (assoc helper/addon :-testing-zipfile dest)]
-        (is (nil? (core/install-addon-guard addon install-dir)))
+            addon (assoc helper/addon :-testing-zipfile dest)
+            opts {}]
+        (is (nil? (core/install-addon-guard addon install-dir opts)))
         ;; bad zip file deleted
         (is (= 0 (count (fs/list-dir install-dir))))))))
 
-(deftest install-bundled-addon
+(deftest install-addon-guard--bundled-addon
   (testing "installing a bundled addon"
     (with-running-app
       (let [install-dir (helper/install-dir)
@@ -643,7 +641,7 @@
         (is (= ["EveryAddon" "EveryAddon-BundledAddon" "everyaddon--0-1-2.zip"] directory-list))
         (is (= expected-nfo (nfo/read-nfo-file install-dir "EveryAddon-BundledAddon")))))))
 
-(deftest install-bundled-addon-overwriting-ignored-addon
+(deftest install-addon-guard--bundled-addon-overwriting-ignored-addon
   (testing "installing/unzipping an addon with a shared mutual dependency of an addon that is ignored isn't possible"
     (with-running-app
       (let [install-dir (helper/install-dir)
@@ -666,7 +664,7 @@
           (core/install-addon-guard addon2)
           (is (= ["EveryAddon" "EveryAddon-BundledAddon"] (helper/install-dir-contents))))))))
 
-(deftest install-bundled-addon-overwriting-pinned-addon
+(deftest install-addon-guard--bundled-addon-overwriting-pinned-addon
   (testing "installing/unzipping an addon with a shared mutual dependency of an addon that is pinned isn't possible"
     (with-running-app
       (let [install-dir (helper/install-dir)
@@ -697,7 +695,7 @@
         (core/install-addon-guard addon2)
         (is (= ["EveryAddon" "EveryAddon-BundledAddon"] (helper/install-dir-contents)))))))
 
-(deftest install-addon--lenient-game-track
+(deftest install-addon-guard--lenient-game-track
   (testing "a classic addon can be installed into an addon directory by setting the non-strict (lenient) flag"
     (with-running-app
       (let [install-dir (helper/install-dir)
@@ -712,7 +710,7 @@
 
         (core/install-addon-guard addon install-dir)))))
 
-(deftest install-addon--remove-zip
+(deftest install-addon-guard--remove-zip
   (testing "installing an addon with the `:addon-zips-to-keep` preference set to `0` will delete the zip afterwards"
     (with-running-app
       (let [install-dir (helper/install-dir)
@@ -723,7 +721,7 @@
         (core/install-addon-guard helper/addon install-dir)
         (is (= ["EveryAddon"] (helper/install-dir-contents)))))))
 
-(deftest install-addon--remove-multiple-zips
+(deftest install-addon-guard--remove-multiple-zips
   (testing "installing an addon with the `:addon-zips-to-keep` preference set to `0` will delete the zip afterwards"
     (with-running-app
       (let [install-dir (helper/install-dir)
@@ -762,8 +760,9 @@
       (let [install-dir (helper/install-dir)
             [[addon] downloaded-file] (helper/gen-addon! install-dir {:override {:interface-version-list [0]}})
             install-path (->> addon :toc :dirname (fs/file install-dir) str)
-            expected (-> addon :derived-nfo)]
-        (core/install-addon (:installable addon) install-dir downloaded-file)
+            expected (-> addon :derived-nfo)
+            opts {}]
+        (core/install-addon (:installable addon) install-dir downloaded-file opts)
 
         (is (= expected (core/load-installed-addon install-path)))
         (is (= [expected] (core/get-state :installed-addon-list)))))))
@@ -1793,14 +1792,16 @@
              :primary? false,
              :source "wowinterface",
              :source-id "999",
-             :source-map-list [{:source "wowinterface", :source-id "999"}]}]
+             :source-map-list [{:source "wowinterface", :source-id "999"}]}
+
+            opts {}]
 
         ;; A and B can live happily side by side
-        (core/install-addon (:installable addon-a) install-dir downloaded-file-a)
-        (core/install-addon (:installable addon-b) install-dir downloaded-file-b)
+        (core/install-addon (:installable addon-a) install-dir downloaded-file-a opts)
+        (core/install-addon (:installable addon-b) install-dir downloaded-file-b opts)
 
         ;; C wipes out both A and B
-        (core/install-addon (:installable addon-c3) install-dir downloaded-file-c)
+        (core/install-addon (:installable addon-c3) install-dir downloaded-file-c opts)
 
         ;; because C *completely* replaces both A and B, A and B should be uninstalled by C rather
         ;; than overwritten and turned into mutual dependencies of C.
diff --git a/test/strongbox/nfo_test.clj b/test/strongbox/nfo_test.clj
index b79a6330..fd9731a5 100644
--- a/test/strongbox/nfo_test.clj
+++ b/test/strongbox/nfo_test.clj
@@ -299,7 +299,20 @@
       (is (= expected-raw (nfo/read-nfo-file (install-dir) ignorable-addon-dir)))
       (is (= expected (nfo/read-nfo (install-dir) ignorable-addon-dir))))))
 
-(deftest pin-addon
+(deftest ignore
+  (let [nfo-data {:installed-version "1.2.1"
+                  :installed-game-track :classic
+                  :name "EveryAddon"
+                  :group-id "https://foo.bar"
+                  :primary? true
+                  :source "curseforge"
+                  :source-id 321
+                  :source-map-list [{:source "curseforge" :source-id 321}]
+                  :ignore? false} ;; explicit ignore flag
+        expected (merge nfo-data {:ignore? true})]
+    (is (= expected (nfo/ignore nfo-data)))))
+
+(deftest pin!
   (testing "a pinned addon can be pinned to a specific version"
     (let [nfo-data {:installed-version "1.2.1"
                     :installed-game-track :classic
@@ -314,7 +327,21 @@
       (nfo/pin! (install-dir) addon-dir "a.b.c")
       (is (= expected (nfo/read-nfo (install-dir) addon-dir))))))
 
-(deftest unpin-addon
+(deftest unpin
+  (let [nfo-data {:installed-version "1.2.1"
+                  :installed-game-track :classic
+                  :name "EveryAddon"
+                  :group-id "https://foo.bar"
+                  :primary? true
+                  :source "curseforge"
+                  :source-id 321
+                  :source-map-list [{:source "curseforge", :source-id 123}]
+                  :pinned-version "a.b.c"}
+        expected (dissoc nfo-data :pinned-version)]
+
+    (is (= expected (nfo/unpin nfo-data)))))
+
+(deftest unpin!
   (testing "a pinned addon can be 'unpinned'"
     (let [nfo-data {:installed-version "1.2.1"
                     :installed-game-track :classic