Repository: CSCfi/rems
Pull request #3213 (2023-11-03 10:11:44)

Release 2.34.1
Contains fix to migration issue in v2.34:
- events not selected for migration due to checking :expires-on value type. now only checking that :expires-on exists
- :expires-on is now renamed so it is not present after running migration-up (this would lead to schema error for disallowed key)


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 10c552a9e..66eb17fdf 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,7 +6,14 @@ have notable changes.
 
 ## Unreleased
 
-Changes since v2.34
+Changes since v2.34.1
+
+## v2.34.1 "Santakatu +1" 2023-11-03
+
+**NB: This release contains migrations!**
+
+### Fixes
+- A bug in event selection prevented migration (introduced in v2.34) from applying properly. This release contains a fixed version of that migration.
 
 ## v2.34 "Santakatu" 2023-10-31
 
diff --git a/project.clj b/project.clj
index 0ce435c89..08697c6e0 100644
--- a/project.clj
+++ b/project.clj
@@ -1,4 +1,4 @@
-(defproject rems "2.34"
+(defproject rems "2.34.1"
   :description "Resource Entitlement Management System is a tool for managing access rights to resources, such as research datasets."
   :url "https://github.com/CSCfi/rems"
 
diff --git a/resources/migrations/20231103083019-change-expiration-events-1.edn b/resources/migrations/20231103083019-change-expiration-events-1.edn
new file mode 100644
index 000000000..3e75dadc9
--- /dev/null
+++ b/resources/migrations/20231103083019-change-expiration-events-1.edn
@@ -0,0 +1,3 @@
+{:ns rems.migrations.change-expiration-events-1
+ :up-fn migrate-up
+ :down-fn migrate-down}
diff --git a/src/clj/rems/migrations/change_expiration_events_1.clj b/src/clj/rems/migrations/change_expiration_events_1.clj
new file mode 100644
index 000000000..a9dfab569
--- /dev/null
+++ b/src/clj/rems/migrations/change_expiration_events_1.clj
@@ -0,0 +1,61 @@
+(ns ^{:added "2.34.1"} rems.migrations.change-expiration-events-1
+  "Migrates event `:expires-on` to `:application/expires-on`
+  and removes the `:last-activity`.
+
+  Runs the same migration as `rems.migrations.change-expiration-events`,
+  but fixes issue where no events would get migrated, and renames :expires-on
+  so that migrate-up does not result in schema error due to extra keys."
+  (:require [clojure.set]
+            [hugsql.core :as hugsql]
+            [rems.db.applications]
+            [rems.db.core]
+            [rems.json :as json]))
+
+(def migration-id 20231103083019)
+
+(declare get-events set-event!)
+(hugsql/def-db-fns-from-string
+  "
+-- :name get-events :? :*
+SELECT id, eventdata::TEXT
+FROM application_event;
+
+-- :name set-event! :!
+UPDATE application_event
+SET eventdata = :eventdata::jsonb
+WHERE id = :id;
+")
+
+(defn attributes-up [eventdata]
+  (-> eventdata
+      (clojure.set/rename-keys {:expires-on :application/expires-on})
+      (dissoc :last-activity)))
+
+(defn attributes-down [eventdata]
+  (-> eventdata
+      (clojure.set/rename-keys {:application/expires-on :expires-on})
+      (assoc :last-activity (:event/time eventdata)))) ; NB: not the same but it would be difficult to restore
+
+(defn migrate-event [event migrator]
+  (update event :eventdata migrator))
+
+(def event-from-db #(update % :eventdata json/parse-string))
+
+(defn migrate-events! [conn xf migrator]
+  (doseq [event (sequence (comp (map event-from-db) xf) (get-events conn))
+          :let [new-event (migrate-event event migrator)]]
+    (set-event! conn
+                (update new-event :eventdata json/generate-string))))
+
+(def old-event? #(contains? (:eventdata %) :expires-on))
+(def new-event? #(contains? (:eventdata %) :application/expires-on))
+
+(defn migrate-up [{:keys [conn]}]
+  (migrate-events! conn (filter old-event?) attributes-up))
+
+(defn migrate-down [{:keys [conn]}]
+  (migrate-events! conn (filter new-event?) attributes-down))
+
+(comment
+  (migrate-up {:conn rems.db.core/*db*})
+  (migrate-down {:conn rems.db.core/*db*}))


Test patch:
diff --git a/test/clj/rems/test_migrations.clj b/test/clj/rems/test_migrations.clj
index fe250aec6..afe050f42 100644
--- a/test/clj/rems/test_migrations.clj
+++ b/test/clj/rems/test_migrations.clj
@@ -1,4 +1,5 @@
 (ns ^:integration rems.test-migrations
+  "Migration tests that use database."
   (:require [clojure.test :refer [deftest is testing use-fixtures]]
             [hugsql.core :as hugsql]
             [luminus-migrations.core]
@@ -8,7 +9,9 @@
             [rems.db.testing :refer [test-db-fixture reset-db-fixture rollback-db-fixture]]
             [rems.config]
             [rems.json :as json]
-            [rems.migrations.event-public :as event-public]))
+            [rems.migrations.event-public :as event-public]
+            [rems.migrations.change-expiration-events]
+            [rems.migrations.change-expiration-events-1]))
 
 (use-fixtures
   :once
@@ -79,3 +82,61 @@
              (->> (get-events)
                   (mapv (comp json/parse-string :eventdata))))))))
 
+(deftest test-change-expiration-events
+  (let [create-application!
+        (create-db-fn
+         "-- :name create-application! :insert
+          INSERT INTO catalogue_item_application (id)
+          VALUES (nextval ('catalogue_item_application_id_seq'))
+          RETURNING id;
+          ")
+        add-application-event!
+        (create-db-fn
+         "-- :name add-application-event! :returning-execute :1
+          INSERT INTO application_event (appId, eventData)
+          VALUES (:application, :eventdata::jsonb)
+          RETURNING id, eventData::TEXT;
+          ")
+        get-events #(->> (rems.migrations.change-expiration-events/get-events rems.db.core/*db*)
+                         (sort-by :id))
+        previous-migration-id rems.migrations.change-expiration-events/migration-id]
+
+    (testing "create test data"
+      (rollback-until-just-before previous-migration-id)
+
+      (let [app (first (create-application!))
+            _ (is (:id app))
+            create-event #(do {:application (:id app) :eventdata (json/generate-string %)})]
+        (is (:id (first (add-application-event!
+                         (create-event {:expires-on "2023-11-03T11:53:31.469Z"
+                                        :event/time "2023-10-01T00:00:00.000Z"
+                                        :last-activity "2023-10-27T11:53:31.469Z"})))))
+        (is (:id (first (add-application-event!
+                         (create-event {:expires-on "2100-01-01T00:00:00.000Z"
+                                        :event/time "2099-10-01T00:00:00.000Z"})))))
+        (is (:id (first (add-application-event!
+                         (create-event {:event/actor "alice"})))))
+        (is (= [{:expires-on "2023-11-03T11:53:31.469Z" :event/time "2023-10-01T00:00:00.000Z" :last-activity "2023-10-27T11:53:31.469Z"}
+                {:expires-on "2100-01-01T00:00:00.000Z" :event/time "2099-10-01T00:00:00.000Z"}
+                {:event/actor "alice"}]
+               (->> (get-events)
+                    (mapv (comp json/parse-string :eventdata)))))))
+
+    (testing "migrate up"
+      (rems.migrations.change-expiration-events/migrate-up {:conn rems.db.core/*db*}) ; does not change events but here for consistency
+      (rems.migrations.change-expiration-events-1/migrate-up {:conn rems.db.core/*db*})
+      (is (= [{:application/expires-on "2023-11-03T11:53:31.469Z" :event/time "2023-10-01T00:00:00.000Z"}
+              {:application/expires-on "2100-01-01T00:00:00.000Z" :event/time "2099-10-01T00:00:00.000Z"}
+              {:event/actor "alice"}]
+             (->> (get-events)
+                  (mapv (comp json/parse-string :eventdata))))))
+
+    (testing "migrate down"
+      (rems.migrations.change-expiration-events-1/migrate-down {:conn rems.db.core/*db*})
+      (rems.migrations.change-expiration-events/migrate-down {:conn rems.db.core/*db*})
+      (is (= [{:expires-on "2023-11-03T11:53:31.469Z" :event/time "2023-10-01T00:00:00.000Z" :last-activity "2023-10-01T00:00:00.000Z"}
+              {:expires-on "2100-01-01T00:00:00.000Z" :event/time "2099-10-01T00:00:00.000Z" :last-activity "2099-10-01T00:00:00.000Z"}
+              {:event/actor "alice"}]
+             (->> (get-events)
+                  (mapv (comp json/parse-string :eventdata))))))))
+