Repository: CSCfi/rems
Pull request #1621 (2019-09-12 16:55:08)

Show all errors preventing submitting an application at once
Part of #1594 

# Definition of Done / Review checklist

## Reviewability
- [x] link to issue
- [x] consider adding screenshots for ease of review

## Documentation
- [x] update changelog if necessary



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b9987f67f..0ea944564 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -62,6 +62,7 @@ Changes since v2.4
   actions from the user (#1596)
 - Optional "More info" link for catalogue items (#1369)
 - Show separately for each license if it has been accepted by the member (#1591)
+- Show all errors preventing application submission at the same time (#1594)
 
 ### Enhancements
 - Improved version information in footer
diff --git a/resources/translations/en.edn b/resources/translations/en.edn
index 47509111c..c22146c47 100644
--- a/resources/translations/en.edn
+++ b/resources/translations/en.edn
@@ -15,12 +15,14 @@
             :comment-placeholder "Comment"
             :decide "Decide"
             :empty "No applications to process."
-            :errors {:invalid-token [:div [:p "Joining the application failed."] [:p "Check the invitation token " [:code "%1"]]]}
+            :errors {:disabled-catalogue-item "Resource no longer available."
+                     :invalid-token [:div [:p "Joining the application failed."] [:p "Check the invitation token " [:code "%1"]]]
+                     :licenses-not-accepted "Terms of use not accepted."
+                     :submission-failed "Submission failed. Check the following errors:"}
             :export-entitlements "Export entitlements as CSV"
             :handled-applications "Processed applications"
             :invite-member "Invite member"
             :licenses-selection "Add terms of use"
-            :licenses-not-accepted-error "Terms of use not accepted."
             :load-application-states "Load application states"
             :member "Member"
             :member-name "Name"
@@ -258,7 +260,7 @@
          :add-comments-not-shown-to-applicant "Add comment (not shown to applicant)"
          :add-comments-shown-to-applicant "Add comment (shown to applicant)"
          :add-remark "Add remark (shown or hidden to applicant)"
-         :alert-disabled-resources "Resources no longer available."
+         :alert-disabled-resources "Resources no longer available:"
          :application "Application"
          :attachment-remove "Remove"
          :checkbox-checked "Yes"
@@ -283,8 +285,7 @@
          :submit "Send application"
          :success "Success"
          :upload "Upload an attachment"
-         :validation {:errors "Submission failed. Check the following errors."
-                      :invalid-user "Invalid user."
+         :validation {:invalid-user "Invalid user."
                       :invalid-value "Invalid value."
                       :required "Field \"%1\" is required."
                       :toolong "Text in field \"%1\" too long."}}
diff --git a/resources/translations/fi.edn b/resources/translations/fi.edn
index 5bd9816a0..f5c05ad3f 100644
--- a/resources/translations/fi.edn
+++ b/resources/translations/fi.edn
@@ -15,12 +15,14 @@
             :comment-placeholder "Kommentti"
             :decide "Päätä"
             :empty "Ei käsiteltäviä hakemuksia."
-            :errors {:invalid-token [:div [:p "Hakemukseen liittyminen epäonnistui."] [:p "Tarkista kutsukoodi " [:code "%1"]]]}
+            :errors {:disabled-catalogue-item "Resurssit eivät ole enää saatavilla."
+                     :invalid-token [:div [:p "Hakemukseen liittyminen epäonnistui."] [:p "Tarkista kutsukoodi " [:code "%1"]]]
+                     :licenses-not-accepted "Käyttöehtoja ei ole hyväksytty."
+                     :submission-failed "Lähettäminen epäonnistui. Tarkasta seuraavat virheet:"}
             :export-entitlements "Lataa käyttöoikeudet CSV-tiedostona"
             :handled-applications "Käsitellyt hakemukset"
             :invite-member "Kutsu jäsen"
             :licenses-selection "Lisää käyttöehtoja"
-            :licenses-not-accepted-error "Käyttöehtoja ei ole hyväksytty."
             :load-application-states "Lataa hakemusten tilat"
             :member "Jäsen"
             :member-name "Nimi"
@@ -256,7 +258,7 @@
          :add-comments-not-shown-to-applicant "Kommentoi (ei näy hakijalle)"
          :add-comments-shown-to-applicant "Kommentoi (näkyy hakijalle)"
          :add-remark "Lisää huomio (näytetään tai piilotetaan hakijalta)"
-         :alert-disabled-resources "Resurssit eivät ole enää saatavilla."
+         :alert-disabled-resources "Resurssit eivät ole enää saatavilla:"
          :application "Tiedot"
          :attachment-remove "Poista"
          :checkbox-checked "Kyllä"
@@ -281,8 +283,7 @@
          :submit "Lähetä hakemus"
          :success "Onnistui"
          :upload "Lisää liitetiedosto"
-         :validation {:errors "Lähettäminen epäonnistui. Tarkasta seuraavat virheet."
-                      :invalid-user "Käyttäjä ei kelpaa."
+         :validation {:invalid-user "Käyttäjä ei kelpaa."
                       :invalid-value "Arvo ei kelpaa."
                       :required "Kenttä \"%1\" on pakollinen."
                       :toolong "Kentän \"%1\" sisältö on liian pitkä."}}
diff --git a/src/clj/rems/application/commands.clj b/src/clj/rems/application/commands.clj
index d418a60ae..3d7cf09bf 100644
--- a/src/clj/rems/application/commands.clj
+++ b/src/clj/rems/application/commands.clj
@@ -4,6 +4,7 @@
             [schema.core :as s]
             [schema-refined.core :as r]
             [rems.application.model :as model]
+            [rems.application-util :refer [accepted-licenses?]]
             [rems.permissions :as permissions]
             [rems.util :refer [assert-ex try-catch-ex]])
   (:import [org.joda.time DateTime]
@@ -203,10 +204,14 @@
                      :when (or (not (getx item :catalogue-item/enabled))
                                (getx item :catalogue-item/archived)
                                (getx item :catalogue-item/expired))]
-                 {:type :disabled-catalogue-item :catalogue-item-id (getx item :catalogue-item/id)})]
+                 {:type :t.actions.errors/disabled-catalogue-item :catalogue-item-id (getx item :catalogue-item/id)})]
     (when (not (empty? errors))
       {:errors (vec errors)})))
 
+(defn- licenses-not-accepted-error [application userid]
+  (when-not (accepted-licenses? application userid)
+    {:errors [{:type :t.actions.errors/licenses-not-accepted}]}))
+
 (defn- invalid-catalogue-items
   "Checks the given catalogue items for validity and merges the errors"
   [catalogue-item-ids injections]
@@ -339,9 +344,11 @@
        :application/accepted-licenses (set (:accepted-licenses cmd))}))
 
 (defmethod command-handler :application.command/submit
-  [_cmd application injections]
-  (or (validation-error application injections)
-      (disabled-catalogue-items-error application injections)
+  [cmd application injections]
+  (or (merge-with concat
+                  (disabled-catalogue-items-error application injections)
+                  (licenses-not-accepted-error application (:actor cmd))
+                  (validation-error application injections))
       (ok {:event/type :application.event/submitted})))
 
 (defmethod command-handler :application.command/approve
diff --git a/src/cljs/rems/administration/create_form.cljs b/src/cljs/rems/administration/create_form.cljs
index 348ae81fd..7ac388fda 100644
--- a/src/cljs/rems/administration/create_form.cljs
+++ b/src/cljs/rems/administration/create_form.cljs
@@ -403,7 +403,7 @@
   (let [form @(rf/subscribe [::form])
         errors @(rf/subscribe [::form-errors])]
     (when errors
-      [:div.alert.alert-danger (text :t.form.validation/errors)
+      [:div.alert.alert-danger (text :t.actions.errors/submission-failed)
        [format-validation-errors errors form]])))
 
 (defn- form-fields [fields]
diff --git a/src/cljs/rems/application.cljs b/src/cljs/rems/application.cljs
index 7c40d8f4a..04ad470d8 100644
--- a/src/cljs/rems/application.cljs
+++ b/src/cljs/rems/application.cljs
@@ -65,21 +65,21 @@
   (dispatch! (str "#/application/" id) replace?))
 
 (defn- format-validation-error [type field]
-  [:li [:a {:href "#"
-            :on-click (in-page-anchor-link (fields/id-to-name (:field/id field)))}
-        (text-format type (localized (:field/title field)))]])
+  [:a {:href "#"
+       :on-click (in-page-anchor-link (fields/id-to-name (:field/id field)))}
+   (text-format type (localized (:field/title field)))])
 
-(defn- format-validation-errors
+(defn- format-submission-errors
   [application errors]
   (let [fields-by-id (->> (get-in application [:application/form :form/fields])
                           (index-by [:field/id]))]
-    [:div (text :t.form.validation/errors)
+    [:div (text :t.actions.errors/submission-failed)
      (into [:ul]
            (concat
-            (for [{:keys [type field-id]} errors
-                  :when field-id]
-              (let [field (get fields-by-id field-id)]
-                (format-validation-error type field)))))]))
+            (for [{:keys [type field-id]} errors]
+              [:li (if field-id
+                     (format-validation-error type (get fields-by-id field-id))
+                     (text type))])))]))
 
 
 ;;;; State
@@ -169,9 +169,6 @@
                        (not (:success response))
                        (flash-message/show-default-error! :top description)
 
-                       (not (accepted-licenses? application userid))
-                       (flash-message/show-error! :top (text :t.actions/licenses-not-accepted-error))
-
                        :else
                        (post! "/api/applications/submit"
                               {:params {:application-id application-id}
@@ -181,8 +178,10 @@
                                               (rf/dispatch [::fetch-application application-id])
                                               (flash-message/show-default-success! :top description))
                                             (do
-                                              (rf/dispatch [::set-validation-errors (:errors response)])
-                                              (flash-message/show-error! :top [format-validation-errors application (:errors response)]))))
+                                              (let [validation-errors
+                                                    (filter :field-id (:errors response))]
+                                                (rf/dispatch [::set-validation-errors validation-errors]))
+                                              (flash-message/show-error! :top [format-submission-errors application (:errors response)]))))
                                :error-handler (flash-message/default-error-handler :top description)})))
           :error-handler (flash-message/default-error-handler :top description)}))
 


Test patch:
diff --git a/test/clj/rems/api/test_applications.clj b/test/clj/rems/api/test_applications.clj
index e33ce6425..e2b2c31ef 100644
--- a/test/clj/rems/api/test_applications.clj
+++ b/test/clj/rems/api/test_applications.clj
@@ -143,7 +143,7 @@
       (is (= {:success true} (send-command user-id
                                            {:type :application.command/accept-licenses
                                             :application-id application-id
-                                            :accepted-licenses []}))))
+                                            :accepted-licenses [license-id1 license-id2]}))))
 
     (testing "save draft"
       (is (= {:success true} (send-command user-id
@@ -424,7 +424,7 @@
                                                             :enabled false})))
       (rems.db.applications/reload-cache!)
       (is (= {:success false
-              :errors [{:type "disabled-catalogue-item" :catalogue-item-id cat-id}]}
+              :errors [{:type "t.actions.errors/disabled-catalogue-item" :catalogue-item-id cat-id}]}
              (send-command user-id {:type :application.command/submit
                                     :application-id app-id}))))
     (testing "submit with archived catalogue item fails"
@@ -434,7 +434,7 @@
                                                              :archived true})))
       (rems.db.applications/reload-cache!)
       (is (= {:success false
-              :errors [{:type "disabled-catalogue-item" :catalogue-item-id cat-id}]}
+              :errors [{:type "t.actions.errors/disabled-catalogue-item" :catalogue-item-id cat-id}]}
              (send-command user-id {:type :application.command/submit
                                     :application-id app-id}))))
     (testing "submit with normal catalogue item succeeds"
diff --git a/test/clj/rems/application/test_commands.clj b/test/clj/rems/application/test_commands.clj
index b12e43450..39c15637a 100644
--- a/test/clj/rems/application/test_commands.clj
+++ b/test/clj/rems/application/test_commands.clj
@@ -23,6 +23,14 @@
                                     :form/id 1
                                     :workflow/id 1
                                     :workflow/type :workflow/dynamic})
+(def ^:private dummy-licenses {1 {:id 1
+                                  :licensetype "link"
+                                  :localizations {:en {:title "en title"
+                                                       :textcontent "en link"
+                                                       :attachment-id 1}
+                                                  :fi {:title "fi title"
+                                                       :textcontent "fi link"
+                                                       :attachment-id 2}}}})
 (def ^:private dummy-workflows {1 {:workflow {:handlers [{:userid handler-user-id
                                                           :name "user"
                                                           :email "user@example.com"}]}}})
@@ -36,7 +44,7 @@
   {:get-workflow dummy-workflows
    :get-form-template dummy-forms
    :get-catalogue-item dummy-get-catalogue-item
-   :get-license (constantly nil)
+   :get-license dummy-licenses
    :get-user (constantly nil)
    :get-users-with-role (constantly nil)
    :get-attachments-for-application (constantly nil)})
@@ -487,7 +495,7 @@
                                                 :resource/ext-id "urn:11"}
                                                {:catalogue-item/id 20
                                                 :resource/ext-id "urn:21"}]
-                       :application/licenses []
+                       :application/licenses [{:license/id 1}]
                        :form/id 40
                        :workflow/id 50
                        :workflow/type :workflow/dynamic}
@@ -497,11 +505,21 @@
                            :application/id app-id
                            :application/field-values {41 "foo"
                                                       42 "bar"}}
+        licenses-accepted-event {:event/type :application.event/licenses-accepted
+                                 :event/time test-time
+                                 :event/actor applicant-user-id
+                                 :application/id app-id
+                                 :application/accepted-licenses #{1}}
         submit-command {:type :application.command/submit
                         :actor applicant-user-id}
-        application (apply-events nil [created-event draft-saved-event])]
+        application-no-licenses (apply-events nil [created-event draft-saved-event])
+        application (apply-events application-no-licenses [licenses-accepted-event])]
 
-    (testing "can submit a valid form"
+    (testing "cannot submit a valid form if licenses are not accepted"
+      (is (= {:errors [{:type :t.actions.errors/licenses-not-accepted}]}
+             (fail-command application-no-licenses submit-command injections))))
+
+    (testing "can submit a valid form when licenses are accepted"
       (is (= {:event/type :application.event/submitted
               :event/time test-time
               :event/actor applicant-user-id
@@ -517,14 +535,15 @@
 
     (testing "cannot submit if catalogue item is disabled"
       (let [disabled (assoc-in application [:application/resources 1 :catalogue-item/enabled] false)]
-        (is (= {:errors [{:type :disabled-catalogue-item, :catalogue-item-id 20}]}
+        (is (= {:errors [{:type :t.actions.errors/disabled-catalogue-item, :catalogue-item-id 20}]}
                (fail-command disabled submit-command injections)))))
 
     (testing "non-applicant cannot submit"
-      (is (= {:errors [{:type :forbidden}]}
-             (fail-command application
-                           (assoc submit-command :actor "non-applicant")
-                           injections))))
+      (let [application (apply-events application [(assoc licenses-accepted-event :event/actor "non-applicant")])]
+        (is (= {:errors [{:type :forbidden}]}
+               (fail-command application
+                             (assoc submit-command :actor "non-applicant")
+                             injections)))))
 
     (testing "cannot submit twice"
       (is (= {:errors [{:type :forbidden}]}
diff --git a/test/clj/rems/db/test_entitlements.clj b/test/clj/rems/db/test_entitlements.clj
index 22411fdc5..9e7bd0230 100644
--- a/test/clj/rems/db/test_entitlements.clj
+++ b/test/clj/rems/db/test_entitlements.clj
@@ -107,6 +107,10 @@
     (let [app-id (test-data/create-application! {:actor applicant :catalogue-item-ids [item1 item2]})]
       (testing "submitted application should not yet cause entitlements"
         (with-stub-server server
+          (test-data/command! {:type :application.command/accept-licenses
+                               :application-id app-id
+                               :accepted-licenses [lic-id1 lic-id2]
+                               :actor applicant})
           (test-data/command! {:type :application.command/submit
                                :application-id app-id
                                :actor applicant})
@@ -119,23 +123,12 @@
           (is (empty? (db/get-entitlements {:application app-id})))
           (is (empty? (stub/recorded-requests server))))
 
-        (testing "approved application should not yet cause entitlements"
+        (testing "approved application, licenses accepted by one user generates entitlements for that user"
           (with-stub-server server
             (test-data/command! {:type :application.command/approve
                                  :application-id app-id
                                  :actor admin
                                  :comment ""})
-            (entitlements-poller/run)
-
-            (is (empty? (db/get-entitlements {:application app-id})))
-            (is (empty? (stub/recorded-requests server)))))
-
-        (testing "approved application, licenses accepted by one user generates entitlements for that user"
-          (with-stub-server server
-            (test-data/command! {:type :application.command/accept-licenses
-                                 :application-id app-id
-                                 :actor applicant
-                                 :accepted-licenses [lic-id1 lic-id2]})
             (test-data/command! {:type :application.command/accept-licenses
                                  :application-id app-id
                                  :actor member