Repository: CSCfi/rems (clojure)

Issue:
Do not disable handler actions when application is returned to applicant
Handler often needs to close stale application, and spirit of REMS is that handler can do anything to applications anyway. Only drafts should be hidden (as those are just noise for handlers).

Pull request:
Closes #1358

Patch:
diff --git a/src/clj/rems/application/model.clj b/src/clj/rems/application/model.clj
index 26017b8962..68e87859d0 100644
--- a/src/clj/rems/application/model.clj
+++ b/src/clj/rems/application/model.clj
@@ -18,96 +18,88 @@
   [application _event]
   application)
 
-(def ^:private draft-permissions
-  {:applicant [:application.command/save-draft
-               :application.command/submit
-               :application.command/close
-               :application.command/remove-member
-               :application.command/invite-member
-               :application.command/uninvite-member
-               :application.command/accept-licenses
-               :application.command/change-resources]
-   :member [:application.command/accept-licenses]
-   :handler [:see-everything
-             :application.command/remark
-             :application.command/remove-member
-             :application.command/uninvite-member]
-   :commenter [:see-everything
-               :application.command/remark]
-   :decider [:see-everything
-             :application.command/remark]
-   ;; roles whose permissions don't change
-   :reporter [:see-everything
-              :application.command/remark]
-   :past-commenter [:see-everything
-                    :application.command/remark]
-   :past-decider [:see-everything
-                  :application.command/remark]
+(def ^:private submittable-application-commands
+  #{:application.command/save-draft
+    :application.command/submit
+    :application.command/close
+    :application.command/remove-member
+    :application.command/invite-member
+    :application.command/uninvite-member
+    :application.command/accept-licenses
+    :application.command/change-resources})
+
+(def ^:private non-submittable-application-commands
+  #{:application.command/remove-member
+    :application.command/uninvite-member
+    :application.command/accept-licenses})
+
+(def ^:private handler-all-commands
+  #{:application.command/remark
+    :application.command/add-licenses
+    :application.command/add-member
+    :application.command/change-resources
+    :application.command/remove-member
+    :application.command/invite-member
+    :application.command/uninvite-member
+    :application.command/request-comment
+    :application.command/request-decision
+    :application.command/return
+    :application.command/approve
+    :application.command/reject})
+
+(def ^:private handler-returned-commands
+  (disj handler-all-commands :application.command/return))
+
+(def ^:private created-permissions
+  {:applicant submittable-application-commands
+   :member #{:application.command/accept-licenses}
+   :reporter #{:see-everything
+               :application.command/remark}
    ;; member before accepting an invitation
-   :everyone-else [:application.command/accept-invitation]})
+   :everyone-else #{:application.command/accept-invitation}})
 
 (def ^:private submitted-permissions
-  {:applicant [:application.command/remove-member
-               :application.command/uninvite-member
-               :application.command/accept-licenses]
-   :member [:application.command/accept-licenses]
-   :handler [:see-everything
-             :application.command/remark
-             :application.command/add-licenses
-             :application.command/add-member
-             :application.command/change-resources
-             :application.command/remove-member
-             :application.command/invite-member
-             :application.command/uninvite-member
-             :application.command/request-comment
-             :application.command/request-decision
-             :application.command/return
-             :application.command/approve
-             :application.command/reject]
-   :commenter [:see-everything
-               :application.command/remark
-               :application.command/comment]
-   :past-commenter [:see-everything
-                    :application.command/remark]
-   :decider [:see-everything
-             :application.command/remark
-             :application.command/decide]})
+  {:applicant non-submittable-application-commands
+   :handler (conj handler-all-commands :see-everything)
+   :commenter #{:see-everything
+                :application.command/remark
+                :application.command/comment}
+   :past-commenter #{:see-everything
+                     :application.command/remark}
+   :decider #{:see-everything
+              :application.command/remark
+              :application.command/decide}
+   :past-decider #{:see-everything
+                   :application.command/remark}})
+
+(def ^:private returned-permissions
+  {:applicant submittable-application-commands
+   :handler (conj handler-returned-commands :see-everything)})
 
 (def ^:private approved-permissions
-  {:applicant [:application.command/remove-member
-               :application.command/uninvite-member
-               :application.command/accept-licenses]
-   :member [:application.command/accept-licenses]
-   :handler [:see-everything
-             :application.command/remark
-             :application.command/add-member
-             :application.command/change-resources
-             :application.command/remove-member
-             :application.command/invite-member
-             :application.command/uninvite-member
-             :application.command/close]
-   :commenter [:see-everything
-               :application.command/remark
-               :application.command/comment]
-   :past-commenter [:see-everything
-                    :application.command/remark]
-   :decider [:see-everything
-             :application.command/remark
-             :application.command/decide]})
+  {:applicant non-submittable-application-commands
+   :handler #{:see-everything
+              :application.command/remark
+              :application.command/add-member
+              :application.command/change-resources
+              :application.command/remove-member
+              :application.command/invite-member
+              :application.command/uninvite-member
+              :application.command/close}})
 
 (def ^:private closed-permissions
-  {:applicant []
-   :member []
-   :handler [:see-everything]
-   :commenter [:see-everything]
-   :decider [:see-everything]
-   :everyone-else []})
+  {:applicant #{}
+   :member #{}
+   :handler #{:see-everything}
+   :commenter #{:see-everything}
+   :decider #{:see-everything}
+   :everyone-else #{}})
 
 (defmethod calculate-permissions :application.event/created
   [application event]
   (-> application
       (permissions/give-role-to-users :applicant [(:event/actor event)])
-      (permissions/set-role-permissions draft-permissions)))
+      (permissions/update-role-permissions created-permissions)))
 
 (defmethod calculate-permissions :application.event/member-added
   [application event]
@@ -127,12 +119,12 @@
 (defmethod calculate-permissions :application.event/submitted
   [application _event]
   (-> application
-      (permissions/set-role-permissions submitted-permissions)))
+      (permissions/update-role-permissions submitted-permissions)))
 
 (defmethod calculate-permissions :application.event/returned
   [application _event]
   (-> application
-      (permissions/set-role-permissions draft-permissions)))
+      (permissions/update-role-permissions returned-permissions)))
 
 (defmethod calculate-permissions :application.event/comment-requested
   [application event]
@@ -159,17 +151,17 @@
 (defmethod calculate-permissions :application.event/approved
   [application _event]
   (-> application
-      (permissions/set-role-permissions approved-permissions)))
+      (permissions/update-role-permissions approved-permissions)))
 
 (defmethod calculate-permissions :application.event/rejected
   [application _event]
   (-> application
-      (permissions/set-role-permissions closed-permissions)))
+      (permissions/update-role-permissions closed-permissions)))
 
 (defmethod calculate-permissions :application.event/closed
   [application _event]
   (-> application
-      (permissions/set-role-permissions closed-permissions)))
+      (permissions/update-role-permissions closed-permissions)))
 
 
 ;;;; Application
diff --git a/src/clj/rems/permissions.clj b/src/clj/rems/permissions.clj
index 8749d4200c..840fc0dc8a 100644
--- a/src/clj/rems/permissions.clj
+++ b/src/clj/rems/permissions.clj
@@ -66,7 +66,7 @@
       (is (= #{:role-2} (user-roles app "user-2")))
       (is (= #{:everyone-else} (user-roles app "user-3"))))))
 
-(defn set-role-permissions
+(defn update-role-permissions
   "Sets role specific permissions for the application.
 
    In `permission-map`, the key is the role (a keyword), and the value
@@ -81,39 +81,39 @@
           application
           permission-map))
 
-(deftest test-set-role-permissions
+(deftest test-update-role-permissions
   (testing "adding"
     (is (= {::role-permissions {:role #{:foo :bar}}}
            (-> {}
-               (set-role-permissions {:role [:foo :bar]})))))
+               (update-role-permissions {:role [:foo :bar]})))))
   (testing "updating"
     (is (= {::role-permissions {:role #{:gazonk}}}
            (-> {}
-               (set-role-permissions {:role [:foo :bar]})
-               (set-role-permissions {:role [:gazonk]})))))
+               (update-role-permissions {:role [:foo :bar]})
+               (update-role-permissions {:role [:gazonk]})))))
   (testing "removing"
     (is (= {::role-permissions {:role #{}}}
            (-> {}
-               (set-role-permissions {:role [:foo :bar]})
-               (set-role-permissions {:role []}))))
+               (update-role-permissions {:role [:foo :bar]})
+               (update-role-permissions {:role []}))))
     (is (= {::role-permissions {:role #{}}}
            (-> {}
-               (set-role-permissions {:role [:foo :bar]})
-               (set-role-permissions {:role nil})))))
+               (update-role-permissions {:role [:foo :bar]})
+               (update-role-permissions {:role nil})))))
 
   (testing "can set permissions for multiple roles"
     (is (= {::role-permissions {:role-1 #{:foo}
                                 :role-2 #{:bar}}}
            (-> {}
-               (set-role-permissions {:role-1 [:foo]
+               (update-role-permissions {:role-1 [:foo]
                                       :role-2 [:bar]})))))
   (testing "does not alter unrelated roles"
     (is (= {::role-permissions {:unrelated #{:foo}
                                 :role #{:gazonk}}}
            (-> {}
-               (set-role-permissions {:unrelated [:foo]
+               (update-role-permissions {:unrelated [:foo]
                                       :role [:bar]})
-               (set-role-permissions {:role [:gazonk]}))))))
+               (update-role-permissions {:role [:gazonk]}))))))
 
 (defn remove-permission-from-all [application permission]
   (let [roles (keys (::role-permissions application))]
@@ -127,18 +127,19 @@
     (is (= {::role-permissions {:role-1 #{}
                                 :role-2 #{}}}
            (-> {}
-               (set-role-permissions {:role-1 [:foo]
+               (update-role-permissions {:role-1 [:foo]
                                       :role-2 [:foo]})
                (remove-permission-from-all :foo)))))
   (testing "leaves unrelated permissions unchanged"
     (is (= {::role-permissions {:role #{:bar}}}
            (-> {}
-               (set-role-permissions {:role [:foo :bar]})
+               (update-role-permissions {:role [:foo :bar]})
                (remove-permission-from-all :foo))))))
 
 (defn user-permissions
-  "Returns the specified user's permissions to this application.
-   Union of all role specific permissions."
+  "Returns a set of the specified user's permissions to this application.
+   Union of all role specific permissions. Returns an empty set if no
+   permissions are set for the user."
   [application user]
   (->> (user-roles application user)
        (mapcat (fn [role]
@@ -153,15 +154,15 @@
     (is (= #{:foo}
            (-> {}
                (give-role-to-user :role-1 "user")
-               (set-role-permissions {:role-1 #{:foo}})
+               (update-role-permissions {:role-1 #{:foo}})
                (user-permissions "user")))))
   (testing "multiple roles"
     (is (= #{:foo :bar}
            (-> {}
                (give-role-to-user :role-1 "user")
                (give-role-to-user :role-2 "user")
-               (set-role-permissions {:role-1 #{:foo}
-                                      :role-2 #{:bar}})
+               (update-role-permissions {:role-1 #{:foo}
+                                         :role-2 #{:bar}})
                (user-permissions "user"))))))
 
 (defn cleanup [application]


Test patch:
diff --git a/test/clj/rems/application/test_model.clj b/test/clj/rems/application/test_model.clj
index 584dfe33db..58d0a65268 100644
--- a/test/clj/rems/application/test_model.clj
+++ b/test/clj/rems/application/test_model.clj
@@ -918,8 +918,8 @@
                         (permissions/give-role-to-users :handler ["handler"])
                         (permissions/give-role-to-users :role-1 ["user-1"])
                         (permissions/give-role-to-users :role-2 ["user-2"])
-                        (permissions/set-role-permissions {:role-1 []
-                                                           :role-2 [:foo :bar]}))]
+                        (permissions/update-role-permissions {:role-1 #{}
+                                                              :role-2 #{:foo :bar}}))]
     (testing "users with a role can see the application"
       (is (not (nil? (model/apply-user-permissions application "user-1")))))
     (testing "users without a role cannot see the application"
@@ -944,7 +944,7 @@
                               :application/public true}]
           application (-> application
                           (assoc :application/events all-events)
-                          (permissions/set-role-permissions {:role-1 [:see-everything]}))]
+                          (permissions/update-role-permissions {:role-1 #{:see-everything}}))]
       (testing "privileged users"
         (let [application (model/apply-user-permissions application "user-1")]
           (testing "see all events"