Repository: CSCfi/rems (clojure)

Issue:
"Change resources" does not update licenses
When making a new application, adding or deleting resources by "Change resources" does not update the licenses accordingly.

Pull request:
Closes #1377

Patch:
diff --git a/src/clj/rems/application/commands.clj b/src/clj/rems/application/commands.clj
index 2097a1ef26..e121cfa33e 100644
--- a/src/clj/rems/application/commands.clj
+++ b/src/clj/rems/application/commands.clj
@@ -353,7 +353,10 @@
                                               (mapv (:get-catalogue-item injections))
                                               (mapv (fn [catalogue-item]
                                                       {:catalogue-item/id (:id catalogue-item)
-                                                       :resource/ext-id (:resid catalogue-item)})))}
+                                                       :resource/ext-id (:resid catalogue-item)})))
+                  :application/licenses
+                  (mapv (fn [license] {:license/id (:id license)})
+                        ((:get-licenses injections) (:catalogue-item-ids cmd)))}
                  (when (:comment cmd)
                    {:application/comment (:comment cmd)})))))
 
diff --git a/src/clj/rems/application/events.clj b/src/clj/rems/application/events.clj
index bc9887680f..0657007ca8 100644
--- a/src/clj/rems/application/events.clj
+++ b/src/clj/rems/application/events.clj
@@ -106,7 +106,8 @@
          :event/type (s/enum :application.event/resources-changed)
          (s/optional-key :application/comment) s/Str
          :application/resources [{:catalogue-item/id s/Int
-                                  :resource/ext-id s/Str}]))
+                                  :resource/ext-id s/Str}]
+         :application/licenses [{:license/id s/Int}]))
 (s/defschema ReturnedEvent
   (assoc EventBase
          :event/type (s/enum :application.event/returned)
diff --git a/src/clj/rems/application/model.clj b/src/clj/rems/application/model.clj
index f43d85b568..82661d57ed 100644
--- a/src/clj/rems/application/model.clj
+++ b/src/clj/rems/application/model.clj
@@ -299,7 +299,9 @@
   [application event]
   (-> application
       (assoc :application/modified (:event/time event))
-      (assoc :application/resources (vec (:application/resources event)))))
+      (assoc :application/resources (vec (:application/resources event)))
+      (assoc :application/licenses (map #(select-keys % [:license/id])
+                                        (:application/licenses event)))))
 
 (defmethod event-type-specific-application-view :application.event/closed
   [application event]
diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj
index 76e462b3cf..de90b755d6 100644
--- a/src/clj/rems/db/applications.clj
+++ b/src/clj/rems/db/applications.clj
@@ -107,6 +107,11 @@
 
 ;;; Running commands
 
+(defn- get-licenses-for-catalogue-items [catalogue-item-ids]
+  (db/get-licenses
+   {:wfid (get-workflow-id-for-catalogue-items catalogue-item-ids)
+    :items catalogue-item-ids}))
+
 (defn- valid-user? [userid]
   (not (nil? (users/get-user-attributes userid))))
 
@@ -114,7 +119,8 @@
   {:valid-user? valid-user?
    :validate-fields form-validation/validate-fields
    :secure-token secure-token
-   :get-catalogue-item catalogue/get-localized-catalogue-item})
+   :get-catalogue-item catalogue/get-localized-catalogue-item
+   :get-licenses get-licenses-for-catalogue-items})
 
 (declare get-unrestricted-application)
 
@@ -125,7 +131,7 @@
   ;; lots of transaction conflicts when there is contention. This lock
   ;; roughly doubles the throughput for rems.db.test-transactions tests.
   (jdbc/execute! db/*db* ["LOCK TABLE application_event IN SHARE ROW EXCLUSIVE MODE"])
-  (let [app (get-unrestricted-application  (:application-id cmd))
+  (let [app (get-unrestricted-application (:application-id cmd))
         result (commands/handle-command cmd app db-injections)]
     (if (:success result)
       (events/add-event! (:result result))


Test patch:
diff --git a/src/clj/rems/db/test_data.clj b/src/clj/rems/db/test_data.clj
index 7caf95b480..333c7590a8 100644
--- a/src/clj/rems/db/test_data.clj
+++ b/src/clj/rems/db/test_data.clj
@@ -740,6 +740,7 @@
     (create-users-and-roles!)
     (let [res1 (:id (db/create-resource! {:resid "urn:nbn:fi:lb-201403262" :organization "nbn" :owneruserid (+fake-users+ :owner) :modifieruserid (+fake-users+ :owner)}))
           res2 (:id (db/create-resource! {:resid "Extra Data" :organization "nbn" :owneruserid (+fake-users+ :owner) :modifieruserid (+fake-users+ :owner)}))
+          res-with-extra-license (:id (db/create-resource! {:resid "urn:nbn:fi:lb-201403263" :organization "nbn" :owneruserid (+fake-users+ :owner) :modifieruserid (+fake-users+ :owner)}))
           _ (:id (db/create-resource! {:resid "Expired Resource, should not be seen" :organization "nbn" :owneruserid (+fake-users+ :owner) :modifieruserid (+fake-users+ :owner) :end (time/minus (time/now) (time/years 1))}))
           form (create-basic-form! +fake-users+)
           _ (create-archived-form!)
@@ -763,6 +764,7 @@
                                            {"en" "ELFA Corpus, one approval (extra data, disabled)"
                                             "fi" "ELFA-korpus, yksi hyväksyntä (lisäpaketti, pois käytöstä)"})]
       (create-resource-license! res2 "Some test license" (+fake-users+ :owner))
+      (create-resource-license! res-with-extra-license "Extra license" (+fake-users+ :owner))
       (db/set-catalogue-item-state! {:id disabled :enabled false})
       (create-applications! simple (:simple workflows) (+fake-users+ :approver1) (+fake-users+ :approver1))
       (create-bundled-application! simple bundlable (:simple workflows) (+fake-users+ :applicant1) (+fake-users+ :approver1))
@@ -773,6 +775,8 @@
       (let [dynamic (create-catalogue-item! res1 (:dynamic workflows) form
                                             {"en" "Dynamic workflow" "fi" "Dynaaminen työvuo"})]
         (create-dynamic-applications! dynamic (:dynamic workflows) +fake-users+))
+      (create-catalogue-item! res-with-extra-license (:dynamic workflows) form
+                              {"en" "Dynamic workflow with extra license" "fi" "Dynaaminen työvuo ylimääräisellä lisenssillä"})
       (let [thlform (create-thl-demo-form! +fake-users+)
             thl-catid (create-catalogue-item! res1 (:dynamic workflows) thlform {"en" "THL catalogue item" "fi" "THL katalogi-itemi"})]
         (create-member-applications! thl-catid (:dynamic workflows) (+fake-users+ :applicant1) (+fake-users+ :approver1) [{:userid (+fake-users+ :applicant2)}]))
diff --git a/test/clj/rems/api/test_applications.clj b/test/clj/rems/api/test_applications.clj
index 33f7636662..62b148983c 100644
--- a/test/clj/rems/api/test_applications.clj
+++ b/test/clj/rems/api/test_applications.clj
@@ -73,6 +73,12 @@
 (defn- get-ids [applications]
   (set (map :application/id applications)))
 
+(defn- license-ids-for-application [application]
+  (set (map :license/id (:application/licenses application))))
+
+(defn- catalogue-item-ids-for-application [application]
+  (set (map :catalogue-item/id (:application/resources application))))
+
 (defn- get-my-applications [user-id]
   (-> (request :get "/api/my-applications")
       (authenticate "42" user-id)
@@ -271,25 +277,52 @@
               (is (= (:application/request-id request-event)
                      (:application/request-id comment-event)))))))
 
-      (testing "adding and then accepting additonal licenses"
+      (testing "adding and then accepting additional licenses"
         (testing "add licenses"
+          (let [application (get-application application-id user-id)]
+            (is (= #{1 2} (license-ids-for-application application)))
           (is (= {:success true} (send-command handler-id
                                                {:type :application.command/add-licenses
                                                 :application-id application-id
                                                 :licenses [license-id]
-                                                :comment "Please approve these new terms"}))))
-        (testing "applicant can now accept licenses"
+                                                :comment "Please approve these new terms"})))
+          (let [application (get-application application-id user-id)]
+            (is (= #{1 2 5} (license-ids-for-application application))))))
+        (testing "applicant accepts the additional licenses"
           (is (= {:success true} (send-command user-id
                                                {:type :application.command/accept-licenses
                                                 :application-id application-id
                                                 :accepted-licenses [license-id]})))))
 
       (testing "changing resources as handler"
+        (let [application (get-application application-id user-id)]
+          (is (= #{9} (catalogue-item-ids-for-application application)))
+          ;; License #5 was added previously by the handler.
+          (is (= #{1 2 5} (license-ids-for-application application)))
         (is (= {:success true} (send-command handler-id
                                              {:type :application.command/change-resources
                                               :application-id application-id
                                               :catalogue-item-ids [9 10]
-                                              :comment "Here are the correct resources"}))))
+                                              :comment "Here are the correct resources"})))
+        (let [application (get-application application-id user-id)]
+          (is (= #{9 10} (catalogue-item-ids-for-application application)))
+          ;; License #4 is added by the catalogue-item #10, whereas
+          ;; the previously added license #5 is dropped from the list.
+          ;;
+          ;; TODO: The previously added licenses should probably be retained
+          ;; in the licenses after changing resources.
+          (is (= #{1 2 4} (license-ids-for-application application))))))
+
+      (testing "changing resources back as handler"
+        (is (= {:success true} (send-command handler-id
+                                             {:type :application.command/change-resources
+                                              :application-id application-id
+                                              :catalogue-item-ids [9]})))
+        (let [application (get-application application-id user-id)]
+          (is (= #{9} (catalogue-item-ids-for-application application)))
+          ;; After changing resources back, the license #4 added by the
+          ;; now-missing resource is gone, as well.
+          (is (= #{1 2} (license-ids-for-application application)))))
 
       (testing "request-decision"
         (is (= {:success true} (send-command handler-id
@@ -327,6 +360,7 @@
                     "application.event/licenses-added"
                     "application.event/licenses-accepted"
                     "application.event/resources-changed"
+                    "application.event/resources-changed"
                     "application.event/decision-requested"
                     "application.event/decided"
                     "application.event/approved"]
@@ -342,6 +376,7 @@
                     "application.event/licenses-added"
                     "application.event/licenses-accepted"
                     "application.event/resources-changed"
+                    "application.event/resources-changed"
                     "application.event/approved"]
                    applicant-event-types))))))))
 
diff --git a/test/clj/rems/application/test_commands.clj b/test/clj/rems/application/test_commands.clj
index f736a8be4e..fd5654c241 100644
--- a/test/clj/rems/application/test_commands.clj
+++ b/test/clj/rems/application/test_commands.clj
@@ -4,6 +4,7 @@
             [rems.application.commands :as commands]
             [rems.application.events :as events]
             [rems.application.model :as model]
+            [rems.common-util :refer [distinct-by]]
             [rems.form-validation :as form-validation]
             [rems.util :refer [assert-ex getx]])
   (:import [java.util UUID]
@@ -189,17 +190,28 @@
                                                  :event/actor handler-user-id
                                                  :application/comment "This is good"
                                                  :application/id 123}])
+        catalogue-item-to-license {1 1
+                                   2 2
+                                   3 3
+                                   4 1}
         injections {:get-catalogue-item {1 {:id 1 :resid "abc" :formid 1 :wfid 1}
                                          2 {:id 2 :resid "efg" :formid 1 :wfid 1}
                                          3 {:id 3 :resid "hij" :formid 1 :wfid 2}
-                                         4 {:id 4 :resid "klm" :formid 2 :wfid 1}}}]
+                                         4 {:id 4 :resid "klm" :formid 2 :wfid 1}}
+                    :get-licenses
+                    (fn [catalogue-item-ids]
+                      (->> catalogue-item-ids
+                           (mapv (fn [id] {:id (catalogue-item-to-license id)}))
+                           (distinct-by :id)))}]
     (testing "applicant can change draft resources"
       (is (= {:event/type :application.event/resources-changed
               :event/time test-time
               :event/actor applicant-user-id
               :application/id 123
               :application/resources [{:catalogue-item/id 1 :resource/ext-id "abc"}
-                                      {:catalogue-item/id 2 :resource/ext-id "efg"}]}
+                                      {:catalogue-item/id 2 :resource/ext-id "efg"}]
+              :application/licenses [{:license/id 1}
+                                     {:license/id 2}]}
              (ok-command application
                          {:type :application.command/change-resources
                           :actor applicant-user-id
@@ -249,7 +261,9 @@
               :application/id 123
               :application/comment "Changed these for you"
               :application/resources [{:catalogue-item/id 1 :resource/ext-id "abc"}
-                                      {:catalogue-item/id 2 :resource/ext-id "efg"}]}
+                                      {:catalogue-item/id 2 :resource/ext-id "efg"}]
+              :application/licenses [{:license/id 1}
+                                     {:license/id 2}]}
              (ok-command submitted-application
                          {:type :application.command/change-resources
                           :actor handler-user-id
@@ -265,7 +279,10 @@
               :application/resources [{:catalogue-item/id 1 :resource/ext-id "abc"}
                                       {:catalogue-item/id 2 :resource/ext-id "efg"}
                                       {:catalogue-item/id 3 :resource/ext-id "hij"}
-                                      {:catalogue-item/id 4 :resource/ext-id "klm"}]}
+                                      {:catalogue-item/id 4 :resource/ext-id "klm"}]
+              :application/licenses [{:license/id 1}
+                                     {:license/id 2}
+                                     {:license/id 3}]}
              (ok-command submitted-application
                          {:type :application.command/change-resources
                           :actor handler-user-id
@@ -280,7 +297,9 @@
               :application/id 123
               :application/comment "Changed these for you"
               :application/resources [{:catalogue-item/id 1 :resource/ext-id "abc"}
-                                      {:catalogue-item/id 2 :resource/ext-id "efg"}]}
+                                      {:catalogue-item/id 2 :resource/ext-id "efg"}]
+              :application/licenses [{:license/id 1}
+                                     {:license/id 2}]}
              (ok-command approved-application
                          {:type :application.command/change-resources
                           :actor handler-user-id
@@ -296,7 +315,10 @@
               :application/resources [{:catalogue-item/id 1 :resource/ext-id "abc"}
                                       {:catalogue-item/id 2 :resource/ext-id "efg"}
                                       {:catalogue-item/id 3 :resource/ext-id "hij"}
-                                      {:catalogue-item/id 4 :resource/ext-id "klm"}]}
+                                      {:catalogue-item/id 4 :resource/ext-id "klm"}]
+              :application/licenses [{:license/id 1}
+                                     {:license/id 2}
+                                     {:license/id 3}]}
              (ok-command approved-application
                          {:type :application.command/change-resources
                           :actor handler-user-id
diff --git a/test/clj/rems/application/test_model.clj b/test/clj/rems/application/test_model.clj
index 41d4ce7cbc..c7be463655 100644
--- a/test/clj/rems/application/test_model.clj
+++ b/test/clj/rems/application/test_model.clj
@@ -142,6 +142,22 @@
        :end nil
        :enabled true
        :expired false
+       :archived false}
+   34 {:id 34
+       :licensetype "attachment"
+       :title "non-localized title"
+       :textcontent "non-localized filename"
+       :attachment-id 3400
+       :localizations {:en {:title "en title"
+                            :textcontent "en filename"
+                            :attachment-id 3401}
+                       :fi {:title "fi title"
+                            :textcontent "fi filename"
+                            :attachment-id 3402}}
+       :start (DateTime. 100)
+       :end nil
+       :enabled true
+       :expired false
        :archived false}})
 
 (def ^:private get-user
@@ -348,7 +364,11 @@
                                       :application/id 1
                                       :application/resources [{:catalogue-item/id 10 :resource/ext-id "urn:11"}
                                                               {:catalogue-item/id 20 :resource/ext-id "urn:21"}
-                                                              {:catalogue-item/id 30 :resource/ext-id "urn:31"}]})
+                                                              {:catalogue-item/id 30 :resource/ext-id "urn:31"}]
+                                      :application/licenses [{:license/id 30}
+                                                             {:license/id 31}
+                                                             {:license/id 32}
+                                                             {:license/id 34}]})
                         expected-application (deep-merge expected-application
                                                          {:application/last-activity (DateTime. 2600)
                                                           :application/modified (DateTime. 2600)
@@ -364,7 +384,24 @@
                                                                                         :catalogue-item/end nil
                                                                                         :catalogue-item/enabled true
                                                                                         :catalogue-item/expired false
-                                                                                        :catalogue-item/archived false})})]
+                                                                                        :catalogue-item/archived false})
+                                                          :application/licenses (conj (:application/licenses expected-application)
+                                                                                      {:license/id 34
+                                                                                       :license/type :attachment
+                                                                                       :license/title {:en "en title"
+                                                                                                       :fi "fi title"
+                                                                                                       :default "non-localized title"}
+                                                                                       :license/attachment-id {:en 3401
+                                                                                                               :fi 3402
+                                                                                                               :default 3400}
+                                                                                       :license/attachment-filename {:en "en filename"
+                                                                                                                     :fi "fi filename"
+                                                                                                                     :default "non-localized filename"}
+                                                                                       :license/start (DateTime. 100)
+                                                                                       :license/end nil
+                                                                                       :license/expired false
+                                                                                       :license/enabled true
+                                                                                       :license/archived false})})]
                     (is (= expected-application (apply-events events)))))
 
                 (testing "> submitted"
@@ -458,7 +495,11 @@
                                           :application/comment "You should include this resource."
                                           :application/resources [{:catalogue-item/id 10 :resource/ext-id "urn:11"}
                                                                   {:catalogue-item/id 20 :resource/ext-id "urn:21"}
-                                                                  {:catalogue-item/id 30 :resource/ext-id "urn:31"}]})
+                                                                  {:catalogue-item/id 30 :resource/ext-id "urn:31"}]
+                                          :application/licenses [{:license/id 30}
+                                                                 {:license/id 31}
+                                                                 {:license/id 32}
+                                                                 {:license/id 34}]})
                             expected-application (deep-merge expected-application
                                                              {:application/last-activity (DateTime. 3400)
                                                               :application/modified (DateTime. 3400)
@@ -474,7 +515,24 @@
                                                                                             :catalogue-item/end nil
                                                                                             :catalogue-item/enabled true
                                                                                             :catalogue-item/expired false
-                                                                                            :catalogue-item/archived false})})]
+                                                                                            :catalogue-item/archived false})
+                                                              :application/licenses (conj (:application/licenses expected-application)
+                                                                                          {:license/id 34
+                                                                                           :license/type :attachment
+                                                                                           :license/title {:en "en title"
+                                                                                                           :fi "fi title"
+                                                                                                           :default "non-localized title"}
+                                                                                           :license/attachment-id {:en 3401
+                                                                                                                   :fi 3402
+                                                                                                                   :default 3400}
+                                                                                           :license/attachment-filename {:en "en filename"
+                                                                                                                         :fi "fi filename"
+                                                                                                                         :default "non-localized filename"}
+                                                                                           :license/start (DateTime. 100)
+                                                                                           :license/end nil
+                                                                                           :license/expired false
+                                                                                           :license/enabled true
+                                                                                           :license/archived false})})]
                         (is (= expected-application (apply-events events)))))
                     (testing "> licenses added"
                       (let [events (conj events
@@ -528,7 +586,13 @@
                                                   :application/id 1
                                                   :application/comment "I changed the resources"
                                                   :application/resources [{:catalogue-item/id 10 :resource/ext-id "urn:11"}
-                                                                          {:catalogue-item/id 30 :resource/ext-id "urn:31"}]})
+                                                                          {:catalogue-item/id 30 :resource/ext-id "urn:31"}]
+                                                  :application/licenses [{:license/id 30}
+                                                                         {:license/id 31}
+                                                                         {:license/id 32}
+                                                                         ;; Include also the previously added license #33 in the new licenses.
+                                                                         {:license/id 33}
+                                                                         {:license/id 34}]})
                                     expected-application (deep-merge expected-application
                                                                      {:application/last-activity (DateTime. 4500)
                                                                       :application/modified (DateTime. 4500)
@@ -554,7 +618,24 @@
                                                                                                :catalogue-item/end nil
                                                                                                :catalogue-item/enabled true
                                                                                                :catalogue-item/expired false
-                                                                                               :catalogue-item/archived false}]})]
+                                                                                               :catalogue-item/archived false}]
+                                                                      :application/licenses (conj (:application/licenses expected-application)
+                                                                                                  {:license/id 34
+                                                                                                   :license/type :attachment
+                                                                                                   :license/title {:en "en title"
+                                                                                                                   :fi "fi title"
+                                                                                                                   :default "non-localized title"}
+                                                                                                   :license/attachment-id {:en 3401
+                                                                                                                           :fi 3402
+                                                                                                                           :default 3400}
+                                                                                                   :license/attachment-filename {:en "en filename"
+                                                                                                                                 :fi "fi filename"
+                                                                                                                                 :default "non-localized filename"}
+                                                                                                   :license/start (DateTime. 100)
+                                                                                                   :license/end nil
+                                                                                                   :license/expired false
+                                                                                                   :license/enabled true
+                                                                                                   :license/archived false})})]
                                 (is (= expected-application (apply-events events)))))
 
                             (testing "> licenses accepted"