Repository: CSCfi/rems
Pull request #1140 (2019-04-16 13:28:20)

Add domain reporter role
#833


Patch:
diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql
index 2065a6c7f..f901d3aeb 100644
--- a/resources/sql/queries.sql
+++ b/resources/sql/queries.sql
@@ -657,6 +657,11 @@ DO UPDATE SET userAttrs = :userattrs::jsonb;
 SELECT userId
 FROM users;
 
+-- :name get-users-with-role :? :*
+SELECT userid
+FROM roles
+WHERE role = :role;
+
 -- :name get-user-attributes :? :1
 SELECT userAttrs::TEXT
 FROM users
diff --git a/src/clj/rems/api/applications_v2.clj b/src/clj/rems/api/applications_v2.clj
index 6e3655b63..021f12331 100644
--- a/src/clj/rems/api/applications_v2.clj
+++ b/src/clj/rems/api/applications_v2.clj
@@ -33,10 +33,14 @@
 (defn- get-user [user-id]
   (users/get-user-attributes user-id))
 
+(defn- get-users-with-role [role]
+  (users/get-users-with-role role))
+
 (def ^:private injections {:get-form get-form
                            :get-catalogue-item get-catalogue-item
                            :get-license get-license
-                           :get-user get-user})
+                           :get-user get-user
+                           :get-users-with-role get-users-with-role})
 
 ;; short-lived cache to speed up pollers which get the application
 ;; repeatedly for each event instead of building their own projection
diff --git a/src/clj/rems/api/reviews.clj b/src/clj/rems/api/reviews.clj
index b59ba11eb..a4687bc52 100644
--- a/src/clj/rems/api/reviews.clj
+++ b/src/clj/rems/api/reviews.clj
@@ -2,16 +2,16 @@
   (:require [compojure.api.sweet :refer :all]
             [rems.api.applications-v2 :as applications-v2]
             [rems.api.schema :refer :all]
+            [rems.db.roles :as roles]
             [rems.util :refer [getx-user-id]]
             [ring.util.http-response :refer :all]))
 
+;; TODO: now reporter can see all apps on the review page, but should there be a separate reporting page?
+(def ^:private reviewer-roles
+  #{:handler :commenter :decider :past-commenter :past-decider :reporter})
+
 (defn- review? [application]
-  (and (some #{:handler
-               :commenter
-               :past-commenter
-               :decider
-               :past-decider}
-             (:application/roles application))
+  (and (some reviewer-roles (:application/roles application))
        (not= :application.state/draft (:application/state application))))
 
 (defn get-all-reviews [user-id]
@@ -38,12 +38,12 @@
 
     (GET "/open" []
       :summary "Lists applications which the user needs to review"
-      :roles #{:handler :commenter :decider :past-commenter :past-decider}
+      :roles #{:handler :commenter :decider :past-commenter :past-decider :reporter}
       :return [ApplicationOverview]
       (ok (get-open-reviews (getx-user-id))))
 
     (GET "/handled" []
       :summary "Lists applications which the user has already reviewed"
-      :roles #{:handler :commenter :decider :past-commenter :past-decider}
+      :roles #{:handler :commenter :decider :past-commenter :past-decider :reporter}
       :return [ApplicationOverview]
       (ok (get-handled-reviews (getx-user-id))))))
diff --git a/src/clj/rems/application/model.clj b/src/clj/rems/application/model.clj
index af1f52247..e1043afcb 100644
--- a/src/clj/rems/application/model.clj
+++ b/src/clj/rems/application/model.clj
@@ -28,9 +28,12 @@
                                             :application.command/remove-member
                                             :application.command/uninvite-member]
                                   :commenter [:see-everything]
-                                  :past-commenter [:see-everything]
                                   :decider [:see-everything]
+                                  ;; roles whose permissions don't change
+                                  :reporter [:see-everything]
+                                  :past-commenter [:see-everything]
                                   :past-decider [:see-everything]
+                                  ;; member before accepting an invitation
                                   :everyone-else [:application.command/accept-invitation]})
 
 (def ^:private submitted-permissions {:applicant [:application.command/remove-member
@@ -458,7 +461,7 @@
                            (sort-by :license/id))]
     (merge-lists-by :license/id rich-licenses app-licenses)))
 
-(defn enrich-user-attributes [application get-user]
+(defn- enrich-user-attributes [application get-user]
   (letfn [(enrich-members [members]
             (->> members
                  (map (fn [member]
@@ -469,7 +472,11 @@
             :application/members
             enrich-members)))
 
-(defn enrich-with-injections [application {:keys [get-form get-catalogue-item get-license get-user]}]
+(defn- enrich-super-users [application get-users-with-role]
+  (-> application
+      (permissions/give-role-to-users :reporter (get-users-with-role :reporter))))
+
+(defn enrich-with-injections [application {:keys [get-form get-catalogue-item get-license get-user get-users-with-role]}]
   (let [answer-versions (remove nil? [(::draft-answers application)
                                       (::submitted-answers application)
                                       (::previous-submitted-answers application)])
@@ -491,7 +498,8 @@
         (update :application/resources enrich-resources get-catalogue-item)
         (update :application/licenses enrich-licenses get-license)
         (assoc :application/applicant-attributes (get-user (:application/applicant application)))
-        (enrich-user-attributes get-user))))
+        (enrich-user-attributes get-user)
+        (enrich-super-users get-users-with-role))))
 
 (defn build-application-view [events injections]
   (-> (reduce application-view nil events)
diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj
index fa9ef7515..0b0ce982a 100644
--- a/src/clj/rems/db/applications.clj
+++ b/src/clj/rems/db/applications.clj
@@ -28,7 +28,6 @@
   (:import [org.joda.time DateTime]))
 
 (declare get-dynamic-application-state)
-(declare get-dynamic-application-state-for-user)
 
 (defn draft?
   "Is the given `application-id` for an unsaved draft application?"
@@ -448,7 +447,7 @@
 
 ;;; Public event api
 
-(defn get-application-state
+(defn get-application-state ; TODO: legacy code; remove me
   ([application-id]
    (get-application-state (first (db/get-applications {:id application-id}))
                           (map #(dissoc % :id :appid) ; remove keys not in v1 API
@@ -640,8 +639,7 @@
 (defn get-all-events-since [event-id]
   (map fix-event-from-db (db/get-application-events-since {:id event-id})))
 
-;; TODO: remove "dynamic" from names
-(defn get-dynamic-application-state [application-id]
+(defn get-dynamic-application-state [application-id] ; TODO: legacy code; remove me
   (let [application (or (first (db/get-applications {:id application-id}))
                         (throw (rems.InvalidRequestException.
                                 (str "Application " application-id " not found"))))
@@ -655,13 +653,6 @@
     (assert (is-dynamic-application? application) (pr-str application))
     (dynamic/apply-events application events)))
 
-(defn get-dynamic-application-state-for-user [user-id application-id]
-  (let [application (->> (get-dynamic-application-state application-id)
-                         (dynamic/assoc-possible-commands user-id))]
-    (when-not (may-see-application? user-id application)
-      (throw-forbidden))
-    application))
-
 (defn add-event! [event]
   (db/add-application-event! {:application (:application/id event)
                               :user (:event/actor event)
@@ -760,7 +751,8 @@
 
 (defn command! [cmd]
   (assert (:application-id cmd))
-  (let [app (get-dynamic-application-state (:application-id cmd))
+  (let [events (get-application-events (:application-id cmd))
+        app (dynamic/apply-events nil events)
         result (dynamic/handle-command cmd app db-injections)]
     (if (:success result)
       (add-event! (:result result))
diff --git a/src/clj/rems/db/roles.clj b/src/clj/rems/db/roles.clj
index c5b41725d..ce6c5fcbc 100644
--- a/src/clj/rems/db/roles.clj
+++ b/src/clj/rems/db/roles.clj
@@ -2,7 +2,7 @@
   (:require [rems.db.core :as db]
             [rems.util :refer [errorf]]))
 
-(def +roles+ #{:owner})
+(def +roles+ #{:owner :reporter})
 
 (defn- role-from-db [{role-string :role}]
   (let [role (keyword role-string)]
diff --git a/src/clj/rems/db/users.clj b/src/clj/rems/db/users.clj
index 81c85b1a0..c637d3fe0 100644
--- a/src/clj/rems/db/users.clj
+++ b/src/clj/rems/db/users.clj
@@ -21,9 +21,13 @@
   [userid]
   (json/parse-string (:userattrs (db/get-user-attributes {:user userid}))))
 
-(defn get-all-users
-  []
+(defn get-all-users []
   (->> (db/get-users)
        (map :userid)
        (map get-user-attributes)
        (doall)))
+
+(defn get-users-with-role [role]
+  (->> (db/get-users-with-role {:role (name role)})
+       (map :userid)
+       (doall)))
diff --git a/src/clj/rems/workflow/dynamic.clj b/src/clj/rems/workflow/dynamic.clj
index fc0a7d1e4..effe602a4 100644
--- a/src/clj/rems/workflow/dynamic.clj
+++ b/src/clj/rems/workflow/dynamic.clj
@@ -282,7 +282,7 @@
 
 ;;; Possible commands
 
-(defn possible-commands
+(defn possible-commands ; TODO: legacy code; remove me
   "Returns the commands which the user is authorized to execute."
   [actor application-state]
   (permissions/user-permissions application-state actor))
diff --git a/src/cljc/rems/common_util.cljc b/src/cljc/rems/common_util.cljc
index e89ddf76c..409539947 100644
--- a/src/cljc/rems/common_util.cljc
+++ b/src/cljc/rems/common_util.cljc
@@ -63,7 +63,7 @@
                                      (partition 2)
                                      (map (fn [[x y]] (deep-merge x y)))))
         (map? a) (merge-with deep-merge a b)
-        :else b))
+        :else (merge-with deep-merge b)))
 
 (deftest test-deep-merge
   (testing "merge nil"
@@ -76,10 +76,6 @@
     (is (= {:a 1}
            (deep-merge {:a 1}
                        nil))))
-  (testing "preserve false"
-    (is (= {:b false}
-           (deep-merge {:b :anything}
-                       {:b false}))))
   (testing "merge maps"
     (is (= {:a 2}
            (deep-merge {:a 1}
diff --git a/src/cljs/rems/roles.cljs b/src/cljs/rems/roles.cljs
index 7e736a925..802560484 100644
--- a/src/cljs/rems/roles.cljs
+++ b/src/cljs/rems/roles.cljs
@@ -7,12 +7,7 @@
   (some #{:applicant :member} roles))
 
 (defn is-reviewer? [roles]
-  (some #{:handler
-          :commenter
-          :past-commenter
-          :decider
-          :past-decider}
-        roles))
+  (some #{:handler :commenter :decider :past-commenter :past-decider :reporter} roles))
 
 (defn is-admin? [roles]
   ;; TODO: add admin role (owner is a business user; admin is a technical user)


Test patch:
diff --git a/src/clj/rems/db/test_data.clj b/src/clj/rems/db/test_data.clj
index 1ce53116f..005b4320e 100644
--- a/src/clj/rems/db/test_data.clj
+++ b/src/clj/rems/db/test_data.clj
@@ -26,6 +26,7 @@
    :approver1 "developer"
    :approver2 "bob"
    :owner "owner"
+   :reporter "reporter"
    :reviewer "carl"
    :roleless1 "elsa"
    :roleless2 "frank"})
@@ -38,15 +39,17 @@
    "carl" {:eppn "carl" :mail "carl@example.com" :commonName "Carl Reviewer"}
    "elsa" {:eppn "elsa" :mail "elsa@example.com" :commonName "Elsa Roleless"}
    "frank" {:eppn "frank" :mail "frank@example.com" :commonName "Frank Roleless"}
-   "owner" {:eppn "owner" :mail "owner@example.com" :commonName "Owner"}})
+   "owner" {:eppn "owner" :mail "owner@example.com" :commonName "Owner"}
+   "reporter" {:eppn "reporter" :mail "reporter@example.com" :commonName "Reporter"}})
 
 (def +demo-users+
   {:applicant1 "RDapplicant1@funet.fi"
    :applicant2 "RDapplicant2@funet.fi"
    :approver1 "RDapprover1@funet.fi"
    :approver2 "RDapprover2@funet.fi"
+   :reviewer "RDreview@funet.fi"
    :owner "RDowner@funet.fi"
-   :reviewer "RDreview@funet.fi"})
+   :reporter "RDreporter@funet.fi"})
 
 (def +demo-user-data+
   {"RDapplicant1@funet.fi" {:eppn "RDapplicant1@funet.fi" :mail "RDapplicant1.test@test_example.org" :commonName "RDapplicant1 REMSDEMO1"}
@@ -54,36 +57,40 @@
    "RDapprover1@funet.fi" {:eppn "RDapprover1@funet.fi" :mail "RDapprover1.test@rems_example.org" :commonName "RDapprover1 REMSDEMO"}
    "RDapprover2@funet.fi" {:eppn "RDapprover2@funet.fi" :mail "RDapprover2.test@rems_example.org" :commonName "RDapprover2 REMSDEMO"}
    "RDreview@funet.fi" {:eppn "RDreview@funet.fi" :mail "RDreview.test@rems_example.org" :commonName "RDreview REMSDEMO"}
-   "RDowner@funet.fi" {:eppn "RDowner@funet.fi" :mail "RDowner.test@test_example.org" :commonName "RDowner REMSDEMO"}})
+   "RDowner@funet.fi" {:eppn "RDowner@funet.fi" :mail "RDowner.test@test_example.org" :commonName "RDowner REMSDEMO"}
+   "RDreporter@funet.fi" {:eppn "RDreporter@funet.fi" :mail "RDreporter.test@test_example.org" :commonName "RDreporter REMSDEMO"}})
+
+(defn- create-user! [user-attributes & roles]
+  (let [user (:eppn user-attributes)]
+    (users/add-user! user user-attributes)
+    (doseq [role roles]
+      (roles/add-role! user role))))
 
 (defn- create-users-and-roles! []
   ;; users provided by the fake login
-  (users/add-user! (+fake-users+ :approver1) (+fake-user-data+ (+fake-users+ :approver1)))
-  (users/add-user! (+fake-users+ :applicant1) (+fake-user-data+ (+fake-users+ :applicant1)))
-  (users/add-user! (+fake-users+ :applicant2) (+fake-user-data+ (+fake-users+ :applicant2)))
-  (users/add-user! (+fake-users+ :approver2) (+fake-user-data+ (+fake-users+ :approver2)))
-  (users/add-user! (+fake-users+ :reviewer) (+fake-user-data+ (+fake-users+ :reviewer)))
-  ;; users without roles
-  (users/add-user! (+fake-users+ :roleless1) (+fake-user-data+ (+fake-users+ :roleless1)))
-  (users/add-user! (+fake-users+ :roleless2) (+fake-user-data+ (+fake-users+ :roleless2)))
-  ;; a user to own things
-  (users/add-user! (+fake-users+ :owner) (+fake-user-data+ (+fake-users+ :owner)))
-  (roles/add-role! (+fake-users+ :owner) :owner)
+  (let [users (comp +fake-user-data+ +fake-users+)]
+    (create-user! (users :applicant1))
+    (create-user! (users :applicant2))
+    (create-user! (users :approver1))
+    (create-user! (users :approver2))
+    (create-user! (users :reviewer))
+    (create-user! (users :roleless1))
+    (create-user! (users :roleless2))
+    (create-user! (users :owner) :owner)
+    (create-user! (users :reporter) :reporter))
   ;; invalid user for tests
   (db/add-user! {:user "invalid" :userattrs nil}))
 
 (defn- create-demo-users-and-roles! []
   ;; users used on remsdemo
-  (doseq [applicant [(+demo-users+ :applicant1) (+demo-users+ :applicant2)]]
-    (users/add-user! applicant (+demo-user-data+ applicant)))
-  (doseq [approver [(+demo-users+ :approver1) (+demo-users+ :approver2)]]
-    (users/add-user! approver (+demo-user-data+ approver)))
-  (let [reviewer (+demo-users+ :reviewer)]
-    (users/add-user! reviewer (+demo-user-data+ reviewer)))
-  ;; a user to own things
-  (let [owner (+demo-users+ :owner)]
-    (users/add-user! owner (+demo-user-data+ owner))
-    (roles/add-role! owner :owner)))
+  (let [users (comp +demo-user-data+ +demo-users+)]
+    (create-user! (users :applicant1))
+    (create-user! (users :applicant2))
+    (create-user! (users :approver1))
+    (create-user! (users :approver2))
+    (create-user! (users :reviewer))
+    (create-user! (users :owner) :owner)
+    (create-user! (users :reporter) :reporter)))
 
 (defn- create-expired-form! []
   (let [yesterday (time/minus (time/now) (time/days 1))]
diff --git a/test/clj/rems/api/test_applications.clj b/test/clj/rems/api/test_applications.clj
index 8e703d8ce..ed4ea4eb3 100644
--- a/test/clj/rems/api/test_applications.clj
+++ b/test/clj/rems/api/test_applications.clj
@@ -549,4 +549,8 @@
       (is (not (contains? (get-ids (get-open-reviews "developer"))
                           app-id)))
       (is (contains? (get-ids (get-handled-reviews "developer"))
+                     app-id)))
+
+    (testing "domain reporter can see all applications"
+      (is (contains? (get-ids (get-handled-reviews "reporter"))
                      app-id)))))
diff --git a/test/clj/rems/application/test_model.clj b/test/clj/rems/application/test_model.clj
index d0008c5e3..441f1566a 100644
--- a/test/clj/rems/application/test_model.clj
+++ b/test/clj/rems/application/test_model.clj
@@ -135,11 +135,16 @@
                 :mail "applicant@example.com"
                 :commonName "Applicant"}})
 
+(def ^:private get-users-with-role
+  {:owner ["owner1"]
+   :reporter ["reporter1"]})
+
 (deftest test-application-view
   (let [injections {:get-form get-form
                     :get-catalogue-item get-catalogue-item
                     :get-license get-license
-                    :get-user get-user}
+                    :get-user get-user
+                    :get-users-with-role get-users-with-role}
         apply-events (fn [events]
                        (let [application (-> events
                                              events/validate-events
diff --git a/test/clj/rems/db/test_users.clj b/test/clj/rems/db/test_users.clj
new file mode 100644
index 000000000..b8cf84df0
--- /dev/null
+++ b/test/clj/rems/db/test_users.clj
@@ -0,0 +1,28 @@
+(ns ^:integration rems.db.test-users
+  (:require [clojure.test :refer :all]
+            [rems.db.roles :as roles]
+            [rems.db.testing :refer [rollback-db-fixture test-db-fixture]]
+            [rems.db.users :as users]))
+
+(use-fixtures :once test-db-fixture)
+(use-fixtures :each rollback-db-fixture)
+
+(deftest test-users
+  ;; TODO: enforce that userid and eppn must be same?
+  (users/add-user! "user1" {:eppn "whatever"
+                            :some-attr "some value"})
+
+  (testing "get-user-attributes"
+    (is (= {:eppn "whatever"
+            :some-attr "some value"}
+           (users/get-user-attributes "user1"))))
+
+  (testing "get-all-users"
+    (is (= [{:eppn "whatever"
+             :some-attr "some value"}]
+           (users/get-all-users))))
+
+  (testing "get-users-with-role"
+    (roles/add-role! "user1" :owner)
+    (is (= ["user1"] (users/get-users-with-role :owner)))
+    (is (= [] (users/get-users-with-role :reporter)))))