From e4571fc8902542a4b31d55b29426020d06f9af2a Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Wed, 1 Apr 2020 14:06:47 +0300 Subject: [PATCH 01/35] feat: document :event/actor-attributes in schema --- src/clj/rems/api/schema.clj | 1 + 1 file changed, 1 insertion(+) diff --git a/src/clj/rems/api/schema.clj b/src/clj/rems/api/schema.clj index 7be2651a80..9e324168e9 100644 --- a/src/clj/rems/api/schema.clj +++ b/src/clj/rems/api/schema.clj @@ -61,6 +61,7 @@ (s/defschema Event (assoc events/EventBase + :event/actor-attributes UserWithAttributes s/Keyword s/Any)) (s/defschema Entitlement From 6a2a9459e5a8e9bb39aa52396090b18a583ee579 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Wed, 1 Apr 2020 14:44:30 +0300 Subject: [PATCH 02/35] feat: first version of event notifications --- resources/config-defaults.edn | 4 +++ src/clj/rems/api/services/command.clj | 4 ++- src/clj/rems/event_notification.clj | 21 ++++++++++++++ test/clj/rems/api/test_end_to_end.clj | 41 +++++++++++++++++++++++++-- 4 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 src/clj/rems/event_notification.clj diff --git a/resources/config-defaults.edn b/resources/config-defaults.edn index dd1022838f..d97cd56604 100644 --- a/resources/config-defaults.edn +++ b/resources/config-defaults.edn @@ -54,6 +54,10 @@ :remove nil :ga4gh nil} ;; Url where entitlements are pushed in ga4gh format, see https://github.com/ga4gh-duri/ga4gh-duri.github.io/ + ;; URLs to notify about new events + ;; TODO more docs + :event-notification-targets [] + ;; Which database column to show as the application id. ;; Options: :id, :external-id :application-id-column :external-id diff --git a/src/clj/rems/api/services/command.clj b/src/clj/rems/api/services/command.clj index 240bc9a94a..6067d1e23b 100644 --- a/src/clj/rems/api/services/command.clj +++ b/src/clj/rems/api/services/command.clj @@ -15,6 +15,7 @@ [rems.db.users :as users] [rems.db.workflow :as workflow] [rems.email.core :as email] + [rems.event-notification :as event-notification] [rems.form-validation :as form-validation] [rems.util :refer [secure-token]]) (:import rems.TryAgainException)) @@ -41,7 +42,8 @@ (email/generate-event-emails! new-events) (run-entitlements new-events) (rejecter-bot/run-rejecter-bot new-events) - (approver-bot/run-approver-bot new-events))) + (approver-bot/run-approver-bot new-events) + (event-notification/notify! new-events))) (def ^:private command-injections {:valid-user? users/user-exists? diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj new file mode 100644 index 0000000000..507c992994 --- /dev/null +++ b/src/clj/rems/event_notification.clj @@ -0,0 +1,21 @@ +(ns rems.event-notification + (:require [clj-http.client :as http] + [clojure.tools.logging :as log] + [rems.api.schema :as schema] + [rems.config] + [rems.json :as json])) + +(defn notify! [events] + (doseq [event events] + (let [body (json/generate-string event)] + (when-let [targets (seq (get rems.config/env :event-notification-targets))] + (doseq [target targets] + (try + ;; TODO: switch to PUT, which is harder to test because of a bug in stub-http + (http/post target + {:body body + :content-type :json + :socket-timeout 2500 + :conn-timeout 2500}) + (catch Exception e + (log/error "Event notification failed" e)))))))) diff --git a/test/clj/rems/api/test_end_to_end.clj b/test/clj/rems/api/test_end_to_end.clj index bb0e04c3a2..c9e4c96abd 100644 --- a/test/clj/rems/api/test_end_to_end.clj +++ b/test/clj/rems/api/test_end_to_end.clj @@ -22,7 +22,8 @@ (email/try-send-emails!) (entitlements/process-outbox!)) (with-open [entitlements-server (stub/start! {"/add" {:status 200} - "/remove" {:status 200}})] + "/remove" {:status 200}}) + event-server (stub/start! {"/event" {:status 200}})] ;; TODO should test emails with a mock smtp server (let [email-atom (atom [])] (with-redefs [rems.config/env (assoc rems.config/env @@ -31,7 +32,8 @@ :languages [:en] :mail-from "rems@rems.rems" :entitlements-target {:add (str (:uri entitlements-server) "/add") - :remove (str (:uri entitlements-server) "/remove")}) + :remove (str (:uri entitlements-server) "/remove")} + :event-notification-targets [(str (:uri event-server) "/event")]) postal.core/send-message (fn [_host message] (swap! email-atom conj message))] (let [api-key "42" owner-id "owner" @@ -267,7 +269,40 @@ (testing "fetch application as applicant" (let [application (api-call :get (str "/api/applications/" application-id) nil api-key applicant-id)] - (is (= "application.state/closed" (:application/state application))))))))))) + (is (= "application.state/closed" (:application/state application))))) + + (testing "event notifications" + (let [events (for [r (stub/recorded-requests event-server)] + (-> r + :body + (get "postData") + json/parse-string + (select-keys [:application/id :event/type :event/actor + :application/field-values :application/resources :application/forms])))] + (is (= [{:application/id application-id + :event/type "application.event/created" + :event/actor applicant-id + :application/resources [{:resource/ext-id resource-ext-id :catalogue-item/id catalogue-item-id} + {:resource/ext-id resource-ext-id2 :catalogue-item/id catalogue-item-id2}] + :application/forms [{:form/id form-id} {:form/id form-id2}]} + {:application/id application-id + :event/type "application.event/draft-saved" + :event/actor applicant-id + :application/field-values [{:value "e2e test contents" :field "fld1" :form form-id} + {:value "e2e test contents 2" :field "e2e_fld_2" :form form-id2}]} + {:application/id application-id + :event/type "application.event/licenses-accepted" + :event/actor applicant-id} + {:application/id application-id + :event/type "application.event/submitted" + :event/actor applicant-id} + {:application/id application-id + :event/type "application.event/approved" + :event/actor handler-id} + {:application/id application-id + :event/type "application.event/closed" + :event/actor handler-id}] + events)))))))))) (deftest test-approver-rejecter-bots (let [api-key "42" From a3a1a6f31e7f281e318d94662c6448a56e2f054f Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Thu, 2 Apr 2020 12:22:16 +0300 Subject: [PATCH 03/35] feat: outbox for event notifications --- src/clj/rems/api/services/command.clj | 2 +- src/clj/rems/db/outbox.clj | 5 ++- src/clj/rems/event_notification.clj | 53 +++++++++++++++++++++------ test/clj/rems/api/test_end_to_end.clj | 6 ++- 4 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/clj/rems/api/services/command.clj b/src/clj/rems/api/services/command.clj index 6067d1e23b..a5eae91378 100644 --- a/src/clj/rems/api/services/command.clj +++ b/src/clj/rems/api/services/command.clj @@ -43,7 +43,7 @@ (run-entitlements new-events) (rejecter-bot/run-rejecter-bot new-events) (approver-bot/run-approver-bot new-events) - (event-notification/notify! new-events))) + (event-notification/queue-notifications! new-events))) (def ^:private command-injections {:valid-user? users/user-exists? diff --git a/src/clj/rems/db/outbox.clj b/src/clj/rems/db/outbox.clj index 138f728207..e01a51a5ea 100644 --- a/src/clj/rems/db/outbox.clj +++ b/src/clj/rems/db/outbox.clj @@ -11,7 +11,7 @@ (def OutboxData {(s/optional-key :outbox/id) s/Int - :outbox/type (s/enum :email :entitlement-post) + :outbox/type (s/enum :email :entitlement-post :event-notification) :outbox/backoff Duration :outbox/created DateTime :outbox/deadline DateTime @@ -19,7 +19,8 @@ :outbox/latest-attempt (s/maybe DateTime) :outbox/latest-error (s/maybe s/Str) (s/optional-key :outbox/email) s/Any - (s/optional-key :outbox/entitlement-post) s/Any}) + (s/optional-key :outbox/entitlement-post) s/Any + (s/optional-key :outbox/event-notification) s/Any}) (def ^Duration initial-backoff (Duration/standardSeconds 10)) (def ^Duration max-backoff (Duration/standardHours 12)) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index 507c992994..7746eca9a7 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -1,21 +1,52 @@ (ns rems.event-notification (:require [clj-http.client :as http] + [clj-time.core :as time] [clojure.tools.logging :as log] + [mount.core :as mount] [rems.api.schema :as schema] [rems.config] - [rems.json :as json])) + [rems.db.outbox :as outbox] + [rems.json :as json] + [rems.scheduler :as scheduler])) -(defn notify! [events] +(defn- notify! [target body] + (try + ;; TODO: switch to PUT, which is harder to test because of a bug in stub-http + (let [response (http/post target + {:body body + :content-type :json + :socket-timeout 2500 + :conn-timeout 2500}) + status (:status response)] + (when-not (= 200 status) + (log/error "Event notification response status" status) + {:status status})) + (catch Exception e + (log/error "Event notification failed" e) + {:exception (str e)}))) + +(defn process-outbox! [] + (doseq [entry (outbox/get-entries {:type :event-notification :due-now? true})] + (if-let [error (notify! (get-in entry [:outbox/event-notification :target]) + (get-in entry [:outbox/event-notification :body]))] + (let [entry (outbox/attempt-failed! entry error)] + (when (not (:outbox/next-attempt entry)) + (log/warn "all attempts to send event notification id " (:outbox/id entry) "failed"))) + (outbox/attempt-succeeded! (:outbox/id entry))))) + +(mount/defstate event-notification-poller + :start (scheduler/start! process-outbox! (.toStandardDuration (time/seconds 10))) + :stop (scheduler/stop! event-notification-poller)) + +(defn- add-to-outbox! [target body] + (outbox/put! {:outbox/type :event-notification + :outbox/deadline (time/plus (time/now) (time/days 1)) ;; hardcoded for now + :outbox/event-notification {:target target + :body body}})) + +(defn queue-notifications! [events] (doseq [event events] (let [body (json/generate-string event)] (when-let [targets (seq (get rems.config/env :event-notification-targets))] (doseq [target targets] - (try - ;; TODO: switch to PUT, which is harder to test because of a bug in stub-http - (http/post target - {:body body - :content-type :json - :socket-timeout 2500 - :conn-timeout 2500}) - (catch Exception e - (log/error "Event notification failed" e)))))))) + (add-to-outbox! target body)))))) diff --git a/test/clj/rems/api/test_end_to_end.clj b/test/clj/rems/api/test_end_to_end.clj index c9e4c96abd..7df177147c 100644 --- a/test/clj/rems/api/test_end_to_end.clj +++ b/test/clj/rems/api/test_end_to_end.clj @@ -6,6 +6,7 @@ [rems.application.rejecter-bot :as rejecter-bot] [rems.db.entitlements :as entitlements] [rems.email.core :as email] + [rems.event-notification :as event-notification] [rems.json :as json] [stub-http.core :as stub])) @@ -20,7 +21,8 @@ (deftest test-end-to-end (testing "clear poller backlog" (email/try-send-emails!) - (entitlements/process-outbox!)) + (entitlements/process-outbox!) + (event-notification/process-outbox!)) (with-open [entitlements-server (stub/start! {"/add" {:status 200} "/remove" {:status 200}}) event-server (stub/start! {"/event" {:status 200}})] @@ -271,6 +273,8 @@ api-key applicant-id)] (is (= "application.state/closed" (:application/state application))))) + (event-notification/process-outbox!) + (testing "event notifications" (let [events (for [r (stub/recorded-requests event-server)] (-> r From 9ddbb68a36e51673b3761c7f2216ab19314be8c7 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 6 Apr 2020 14:52:58 +0300 Subject: [PATCH 04/35] feat: allow filtering event notifications by type --- src/clj/rems/event_notification.clj | 8 +++++++- test/clj/rems/api/test_end_to_end.clj | 18 +++++------------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index 7746eca9a7..a122adb207 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -44,9 +44,15 @@ :outbox/event-notification {:target target :body body}})) +(defn wants? [target event] + (let [whitelist (:event-types target)] + (or (empty? whitelist) + (some? (some #{(:event/type event)} whitelist))))) + (defn queue-notifications! [events] (doseq [event events] (let [body (json/generate-string event)] (when-let [targets (seq (get rems.config/env :event-notification-targets))] (doseq [target targets] - (add-to-outbox! target body)))))) + (when (wants? target event) + (add-to-outbox! (:url target) body))))))) diff --git a/test/clj/rems/api/test_end_to_end.clj b/test/clj/rems/api/test_end_to_end.clj index 7df177147c..8edef9a58e 100644 --- a/test/clj/rems/api/test_end_to_end.clj +++ b/test/clj/rems/api/test_end_to_end.clj @@ -35,7 +35,10 @@ :mail-from "rems@rems.rems" :entitlements-target {:add (str (:uri entitlements-server) "/add") :remove (str (:uri entitlements-server) "/remove")} - :event-notification-targets [(str (:uri event-server) "/event")]) + :event-notification-targets [{:url (str (:uri event-server) "/event") + :event-types [:application.event/created + :application.event/submitted + :application.event/approved]}]) postal.core/send-message (fn [_host message] (swap! email-atom conj message))] (let [api-key "42" owner-id "owner" @@ -282,29 +285,18 @@ (get "postData") json/parse-string (select-keys [:application/id :event/type :event/actor - :application/field-values :application/resources :application/forms])))] + :application/resources :application/forms])))] (is (= [{:application/id application-id :event/type "application.event/created" :event/actor applicant-id :application/resources [{:resource/ext-id resource-ext-id :catalogue-item/id catalogue-item-id} {:resource/ext-id resource-ext-id2 :catalogue-item/id catalogue-item-id2}] :application/forms [{:form/id form-id} {:form/id form-id2}]} - {:application/id application-id - :event/type "application.event/draft-saved" - :event/actor applicant-id - :application/field-values [{:value "e2e test contents" :field "fld1" :form form-id} - {:value "e2e test contents 2" :field "e2e_fld_2" :form form-id2}]} - {:application/id application-id - :event/type "application.event/licenses-accepted" - :event/actor applicant-id} {:application/id application-id :event/type "application.event/submitted" :event/actor applicant-id} {:application/id application-id :event/type "application.event/approved" - :event/actor handler-id} - {:application/id application-id - :event/type "application.event/closed" :event/actor handler-id}] events)))))))))) From 268a23d3f7557283b28efbfc8a3c27231ad26b6d Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 6 Apr 2020 14:57:19 +0300 Subject: [PATCH 05/35] feat: make outbox processing order more stable --- resources/sql/queries.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql index 50c3e6dab4..ba3cc8e0c3 100644 --- a/resources/sql/queries.sql +++ b/resources/sql/queries.sql @@ -552,7 +552,7 @@ WHERE 1 = 1 /*~ (when (:ids params) */ AND id IN (:v*:ids) /*~ ) ~*/ -; +ORDER BY id ASC; -- :name update-outbox! :! UPDATE outbox From cff3f830e5a2a0985c2aa81f0dc5a446309b5dbd Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 6 Apr 2020 15:08:00 +0300 Subject: [PATCH 06/35] feat: docs and validation for :event-notification-targets config option --- resources/config-defaults.edn | 11 +++++++++-- src/clj/rems/application/events.clj | 3 +++ src/clj/rems/config.clj | 12 ++++++++++-- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/resources/config-defaults.edn b/resources/config-defaults.edn index d97cd56604..da0a17e81c 100644 --- a/resources/config-defaults.edn +++ b/resources/config-defaults.edn @@ -54,8 +54,15 @@ :remove nil :ga4gh nil} ;; Url where entitlements are pushed in ga4gh format, see https://github.com/ga4gh-duri/ga4gh-duri.github.io/ - ;; URLs to notify about new events - ;; TODO more docs + ;; URLs to notify about new events. Each target should contain + ;; a :url, and optionally :event-types to receive only the specified + ;; event types. + ;; + ;; Example: + ;; + ;; :event-notification-targets [{:url "http://events/everything"} + ;; {:url "http://events/filtered" + ;; :event-types [:application.event/created :application.event/submitted]}] :event-notification-targets [] ;; Which database column to show as the application id. diff --git a/src/clj/rems/application/events.clj b/src/clj/rems/application/events.clj index 537012842a..0f70ad8daf 100644 --- a/src/clj/rems/application/events.clj +++ b/src/clj/rems/application/events.clj @@ -168,6 +168,9 @@ :application.event/revoked RevokedEvent :application.event/submitted SubmittedEvent}) +(def event-types + (keys event-schemas)) + (s/defschema Event (apply r/dispatch-on :event/type (flatten (seq event-schemas)))) diff --git a/src/clj/rems/config.clj b/src/clj/rems/config.clj index a179852c1a..0cc53db0c9 100644 --- a/src/clj/rems/config.clj +++ b/src/clj/rems/config.clj @@ -8,6 +8,7 @@ [cprop.tools :refer [merge-maps]] [mount.core :refer [defstate]] [rems.application.commands :as commands] + [rems.application.events :as events] [rems.json :as json]) (:import [java.io FileNotFoundException] [org.joda.time Period])) @@ -52,8 +53,15 @@ (assert (.endsWith url "/") (str ":public-url should end with /:" (pr-str url)))) (when-let [invalid-commands (seq (remove (set commands/command-names) (:disable-commands config)))] - (log/warn "Unrecognized values in :disable-commands : " (pr-str invalid-commands)) - (log/warn "Supported-values: " (pr-str commands/command-names))) + (log/warn "Unrecognized values in :disable-commands :" (pr-str invalid-commands)) + (log/warn "Supported-values:" (pr-str commands/command-names))) + (doseq [target (:event-notification-targets config)] + (when-let [invalid-events (seq (remove (set events/event-types) (:event-types target)))] + (log/warn "Unrecognized event types in event notification target" + (pr-str target) + ":" + (pr-str invalid-events)) + (log/warn "Supported event types:" (pr-str events/event-types)))) (assert (not (empty? (:organizations config))) ":organizations can not be empty") (when-let [invalid-keys (seq (remove known-config-keys (keys config)))] From 8dc6c4a1f5d00e09f5722e6f11e4942b0e56e69b Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 6 Apr 2020 15:28:54 +0300 Subject: [PATCH 07/35] fix: outbox expects string errors, not map --- src/clj/rems/event_notification.clj | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index a122adb207..9bdf939e50 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -20,10 +20,10 @@ status (:status response)] (when-not (= 200 status) (log/error "Event notification response status" status) - {:status status})) + (str "status " status))) (catch Exception e (log/error "Event notification failed" e) - {:exception (str e)}))) + (str "exception " e)))) (defn process-outbox! [] (doseq [entry (outbox/get-entries {:type :event-notification :due-now? true})] From 5136b3302fc90f901ddee0e9fa4629115a18333c Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 6 Apr 2020 15:32:10 +0300 Subject: [PATCH 08/35] feat: send application state along with event notification --- src/clj/rems/event_notification.clj | 8 +++++++- test/clj/rems/api/test_end_to_end.clj | 16 ++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index 9bdf939e50..173077a9f2 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -5,6 +5,7 @@ [mount.core :as mount] [rems.api.schema :as schema] [rems.config] + [rems.db.applications :as applications] [rems.db.outbox :as outbox] [rems.json :as json] [rems.scheduler :as scheduler])) @@ -51,7 +52,12 @@ (defn queue-notifications! [events] (doseq [event events] - (let [body (json/generate-string event)] + (let [;; TODO: get-unrestricted-application doesn't have a public + ;; schema and includes internal stuff like ::latest-review-request-by-user. + ;; Need to figure out a non-user-specific version of get-application + application (applications/get-unrestricted-application (:application/id event)) + event-with-app (assoc event :event/application application) + body (json/generate-string event-with-app)] (when-let [targets (seq (get rems.config/env :event-notification-targets))] (doseq [target targets] (when (wants? target event) diff --git a/test/clj/rems/api/test_end_to_end.clj b/test/clj/rems/api/test_end_to_end.clj index 8edef9a58e..90aae51bce 100644 --- a/test/clj/rems/api/test_end_to_end.clj +++ b/test/clj/rems/api/test_end_to_end.clj @@ -285,19 +285,27 @@ (get "postData") json/parse-string (select-keys [:application/id :event/type :event/actor - :application/resources :application/forms])))] + :application/resources :application/forms + :event/application]) + (update :event/application select-keys [:application/id :application/state])))] (is (= [{:application/id application-id :event/type "application.event/created" :event/actor applicant-id :application/resources [{:resource/ext-id resource-ext-id :catalogue-item/id catalogue-item-id} {:resource/ext-id resource-ext-id2 :catalogue-item/id catalogue-item-id2}] - :application/forms [{:form/id form-id} {:form/id form-id2}]} + :application/forms [{:form/id form-id} {:form/id form-id2}] + :event/application {:application/id application-id + :application/state "application.state/draft"}} {:application/id application-id :event/type "application.event/submitted" - :event/actor applicant-id} + :event/actor applicant-id + :event/application {:application/id application-id + :application/state "application.state/submitted"}} {:application/id application-id :event/type "application.event/approved" - :event/actor handler-id}] + :event/actor handler-id + :event/application {:application/id application-id + :application/state "application.state/approved"}}] events)))))))))) (deftest test-approver-rejecter-bots From b23b66038e105248e6556af13e0acf52ef191a53 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 09:32:48 +0300 Subject: [PATCH 09/35] deps: upgrade stub-http to get support for PUT bodies --- project.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project.clj b/project.clj index 581fe3b3c2..5b80e06941 100644 --- a/project.clj +++ b/project.clj @@ -169,7 +169,7 @@ [figwheel-sidecar "0.5.19" :exclusions [org.clojure/tools.nrepl com.fasterxml.jackson.core/jackson-core]] [re-frisk "0.5.4.1"] [ring/ring-mock "0.4.0" :exclusions [cheshire]] - [se.haleby/stub-http "0.2.7"]] + [se.haleby/stub-http "0.2.8"]] :plugins [[lein-ancient "0.6.15"] [lein-doo "0.1.11"] From 998d9340da034f3f171684229edb559bdc5ba6b1 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 09:34:10 +0300 Subject: [PATCH 10/35] feat: use PUT for event notifications --- src/clj/rems/event_notification.clj | 11 +++++------ test/clj/rems/api/test_end_to_end.clj | 6 ++++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index 173077a9f2..73e2f4c760 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -12,12 +12,11 @@ (defn- notify! [target body] (try - ;; TODO: switch to PUT, which is harder to test because of a bug in stub-http - (let [response (http/post target - {:body body - :content-type :json - :socket-timeout 2500 - :conn-timeout 2500}) + (let [response (http/put target + {:body body + :content-type :json + :socket-timeout 2500 + :conn-timeout 2500}) status (:status response)] (when-not (= 200 status) (log/error "Event notification response status" status) diff --git a/test/clj/rems/api/test_end_to_end.clj b/test/clj/rems/api/test_end_to_end.clj index 90aae51bce..4f8087ffba 100644 --- a/test/clj/rems/api/test_end_to_end.clj +++ b/test/clj/rems/api/test_end_to_end.clj @@ -279,15 +279,17 @@ (event-notification/process-outbox!) (testing "event notifications" - (let [events (for [r (stub/recorded-requests event-server)] + (let [requests (stub/recorded-requests event-server) + events (for [r requests] (-> r :body - (get "postData") + (get "content") json/parse-string (select-keys [:application/id :event/type :event/actor :application/resources :application/forms :event/application]) (update :event/application select-keys [:application/id :application/state])))] + (is (every? (comp #{"PUT"} :method) requests)) (is (= [{:application/id application-id :event/type "application.event/created" :event/actor applicant-id From 80e7b65e898cada7e6050d23deb4e1c032627a7c Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 10:54:04 +0300 Subject: [PATCH 11/35] feat: initial version of /api/application/:id/raw endpoint --- src/clj/rems/api/applications.clj | 12 +++++++++++- src/clj/rems/api/schema.clj | 12 ++++++++++++ test/clj/rems/api/test_applications.clj | 18 ++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/clj/rems/api/applications.clj b/src/clj/rems/api/applications.clj index 348bf608e8..70752dc996 100644 --- a/src/clj/rems/api/applications.clj +++ b/src/clj/rems/api/applications.clj @@ -245,7 +245,7 @@ ;; the path parameter matches also non-numeric paths, so this route must be after all overlapping routes (GET "/:application-id" [] - :summary "Get application by `application-id`" + :summary "Get application by `application-id`. Application is customized for the requesting user (e.g. event visibility, permissions, etc)." :roles #{:logged-in} :path-params [application-id :- (describe s/Int "application id")] :responses {200 {:schema Application} @@ -254,6 +254,16 @@ (ok app) (api-util/not-found-json-response))) + (GET "/:application-id/raw" [] + :summary "Get application by `application-id`. Application is not customized." + :roles #{:reporter :owner} + :path-params [application-id :- (describe s/Int "application id")] + :responses {200 {:schema ApplicationRaw} + 404 {:schema s/Str :description "Not found"}} + (if-let [app (applications/get-unrestricted-application application-id)] + (ok app) + (api-util/not-found-json-response))) + (GET "/:application-id/experimental/pdf" request :summary "PDF export of application (EXPERIMENTAL)" :roles #{:logged-in :api-key} diff --git a/src/clj/rems/api/schema.clj b/src/clj/rems/api/schema.clj index 9e324168e9..5cfaa13553 100644 --- a/src/clj/rems/api/schema.clj +++ b/src/clj/rems/api/schema.clj @@ -258,6 +258,18 @@ :application/permissions Permissions :application/attachments [ApplicationAttachment]}) +(s/defschema ApplicationRaw + (-> Application + (dissoc :application/permissions + :application/roles + :application/invited-members s/Any) + (assoc :application/invitation-tokens s/Any + :application/past-members s/Any + (s/optional-key :rems.application.model/latest-decision-request-by-user) s/Any + (s/optional-key :rems.application.model/latest-review-request-by-user) s/Any + :rems.permissions/role-permissions s/Any + :rems.permissions/user-roles s/Any))) + (s/defschema ApplicationOverview (dissoc Application :application/events diff --git a/test/clj/rems/api/test_applications.clj b/test/clj/rems/api/test_applications.clj index 7d884602be..97080f99bf 100644 --- a/test/clj/rems/api/test_applications.clj +++ b/test/clj/rems/api/test_applications.clj @@ -1229,3 +1229,21 @@ app-id))) (is (contains? (get-ids (get-handled-todos decider)) app-id))))) + +(deftest test-application-raw + (let [api-key "42" + applicant "alice" + handler "developer" + reporter "reporter" + app-id (test-data/create-application! {:actor applicant})] + (testing "applicant can't get raw application" + (is (response-is-forbidden? (api-response :get (str "/api/applications/" app-id "/raw") nil + api-key applicant)))) + (testing "reporter can get raw application" + (is (= {:application/id app-id + :rems.permissions/user-roles {(keyword applicant) ["applicant"] + (keyword handler) ["handler"] + (keyword reporter) ["reporter"]}} + (-> (api-call :get (str "/api/applications/" app-id "/raw") nil + api-key reporter) + (select-keys [:application/id :rems.permissions/user-roles]))))))) From ae6c6b6054e71004e2f952ec1c4484ed94005eb8 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 10:59:59 +0300 Subject: [PATCH 12/35] doc: some TODO comments --- src/clj/rems/api/schema.clj | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/clj/rems/api/schema.clj b/src/clj/rems/api/schema.clj index 5cfaa13553..1242a5870e 100644 --- a/src/clj/rems/api/schema.clj +++ b/src/clj/rems/api/schema.clj @@ -263,12 +263,12 @@ (dissoc :application/permissions :application/roles :application/invited-members s/Any) - (assoc :application/invitation-tokens s/Any - :application/past-members s/Any - (s/optional-key :rems.application.model/latest-decision-request-by-user) s/Any - (s/optional-key :rems.application.model/latest-review-request-by-user) s/Any - :rems.permissions/role-permissions s/Any - :rems.permissions/user-roles s/Any))) + (assoc :application/invitation-tokens s/Any ;; TODO this should be hidden + :application/past-members s/Any ;; TODO schema for this + (s/optional-key :rems.application.model/latest-decision-request-by-user) s/Any ;; TODO hide? + (s/optional-key :rems.application.model/latest-review-request-by-user) s/Any ;; TODO hide? + :rems.permissions/role-permissions s/Any ;; TODO schema for this + :rems.permissions/user-roles s/Any))) ;; TODO schema for this (s/defschema ApplicationOverview (dissoc Application From 496d999e0613f8f088b92721bc1ffa215af5b035 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 15:33:22 +0300 Subject: [PATCH 13/35] feat: make ApplicationRaw closer to Application --- src/clj/rems/api/applications.clj | 2 +- src/clj/rems/api/schema.clj | 9 ++------- src/clj/rems/application/model.clj | 2 +- src/clj/rems/db/applications.clj | 11 +++++++++++ 4 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/clj/rems/api/applications.clj b/src/clj/rems/api/applications.clj index 70752dc996..601dbdac06 100644 --- a/src/clj/rems/api/applications.clj +++ b/src/clj/rems/api/applications.clj @@ -260,7 +260,7 @@ :path-params [application-id :- (describe s/Int "application id")] :responses {200 {:schema ApplicationRaw} 404 {:schema s/Str :description "Not found"}} - (if-let [app (applications/get-unrestricted-application application-id)] + (if-let [app (applications/get-application-raw application-id)] (ok app) (api-util/not-found-json-response))) diff --git a/src/clj/rems/api/schema.clj b/src/clj/rems/api/schema.clj index 1242a5870e..6a8f2b6d9a 100644 --- a/src/clj/rems/api/schema.clj +++ b/src/clj/rems/api/schema.clj @@ -261,13 +261,8 @@ (s/defschema ApplicationRaw (-> Application (dissoc :application/permissions - :application/roles - :application/invited-members s/Any) - (assoc :application/invitation-tokens s/Any ;; TODO this should be hidden - :application/past-members s/Any ;; TODO schema for this - (s/optional-key :rems.application.model/latest-decision-request-by-user) s/Any ;; TODO hide? - (s/optional-key :rems.application.model/latest-review-request-by-user) s/Any ;; TODO hide? - :rems.permissions/role-permissions s/Any ;; TODO schema for this + :application/roles) + (assoc :rems.permissions/role-permissions s/Any ;; TODO schema for this :rems.permissions/user-roles s/Any))) ;; TODO schema for this (s/defschema ApplicationOverview diff --git a/src/clj/rems/application/model.clj b/src/clj/rems/application/model.clj index 02f0a8b718..2df20faed5 100644 --- a/src/clj/rems/application/model.clj +++ b/src/clj/rems/application/model.clj @@ -604,7 +604,7 @@ (defn apply-privacy [application roles] (transform [:application/forms ALL :form/fields ALL] #(apply-field-privacy % roles) application)) -(defn- hide-non-public-information [application] +(defn hide-non-public-information [application] (-> application hide-invitation-tokens ;; these are not used by the UI, so no need to expose them (especially the user IDs) diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj index 8942d47004..fc2a3b5cf3 100644 --- a/src/clj/rems/db/applications.clj +++ b/src/clj/rems/db/applications.clj @@ -90,6 +90,11 @@ :blacklisted? #(cache/lookup-or-miss blacklist-cache [%1 %2] (fn [[userid resource]] (blacklist/blacklisted? userid resource)))}) +;; TODO rename +;; get-unrestricted-application -> get-application-internal +;; get-application-raw -> get-application +;; get-application -> get-application-for-user + (defn get-unrestricted-application "Returns the full application state without any user permission checks and filtering of sensitive information. Don't expose via APIs." @@ -99,6 +104,12 @@ nil ; application not found (model/build-application-view events fetcher-injections)))) +(defn get-application-raw + "Full application state with internal information hidden. No personalized for any users. Suitable for public APIs" + [application-id] + (when-let [application (get-unrestricted-application application-id)] + (model/hide-non-public-information application))) + (defn get-application "Returns the part of application state which the specified user is allowed to see. Suitable for returning from public APIs as-is." From f854b7963a2f337bc3ed7968a1159a277a6ae515 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 15:35:55 +0300 Subject: [PATCH 14/35] feat: rename :rems.permissions/* to :application/* since they're now public via the ApplicationRaw schema --- src/clj/rems/api/schema.clj | 4 +- src/clj/rems/db/applications.clj | 6 +-- src/clj/rems/permissions.clj | 68 ++++++++++++------------ test/clj/rems/api/test_applications.clj | 8 +-- test/clj/rems/application/test_model.clj | 6 +-- 5 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/clj/rems/api/schema.clj b/src/clj/rems/api/schema.clj index 6a8f2b6d9a..9a3cff5cab 100644 --- a/src/clj/rems/api/schema.clj +++ b/src/clj/rems/api/schema.clj @@ -262,8 +262,8 @@ (-> Application (dissoc :application/permissions :application/roles) - (assoc :rems.permissions/role-permissions s/Any ;; TODO schema for this - :rems.permissions/user-roles s/Any))) ;; TODO schema for this + (assoc :application/role-permissions s/Any ;; TODO schema for this + :application/user-roles s/Any))) ;; TODO schema for this (s/defschema ApplicationOverview (dissoc Application diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj index fc2a3b5cf3..7f0781202d 100644 --- a/src/clj/rems/db/applications.clj +++ b/src/clj/rems/db/applications.clj @@ -154,7 +154,7 @@ (defn- group-apps-by-user [apps] (->> apps (mapcat (fn [app] - (for [user (keys (:rems.permissions/user-roles app))] + (for [user (keys (:application/user-roles app))] (when-let [app (model/apply-user-permissions app user)] [user app])))) (reduce (fn [apps-by-user [user app]] @@ -176,7 +176,7 @@ (defn- group-roles-by-user [apps] (->> apps - (mapcat (fn [app] (:rems.permissions/user-roles app))) + (mapcat (fn [app] (:application/user-roles app))) (reduce (fn [roles-by-user [user roles]] (update roles-by-user user set/union roles)) {}))) @@ -193,7 +193,7 @@ (defn- group-users-by-role [apps] (->> apps (mapcat (fn [app] - (for [[user roles] (:rems.permissions/user-roles app) + (for [[user roles] (:application/user-roles app) role roles] [user role]))) (reduce (fn [users-by-role [user role]] diff --git a/src/clj/rems/permissions.clj b/src/clj/rems/permissions.clj index 6bf8142e1f..53ae9da47e 100644 --- a/src/clj/rems/permissions.clj +++ b/src/clj/rems/permissions.clj @@ -7,7 +7,7 @@ (defn- give-role-to-user [application role user] (assert (keyword? role) {:role role}) (assert (string? user) {:user user}) - (update-in application [::user-roles user] conj-set role)) + (update-in application [:application/user-roles user] conj-set role)) (defn give-role-to-users [application role users] (reduce (fn [app user] @@ -24,39 +24,39 @@ (assert (keyword? role) {:role role}) (assert (string? user) {:user user}) (-> application - (update-in [::user-roles user] disj role) - (update ::user-roles dissoc-if-empty user))) + (update-in [:application/user-roles user] disj role) + (update :application/user-roles dissoc-if-empty user))) (defn user-roles [application user] - (let [specific-roles (set (get-in application [::user-roles user]))] + (let [specific-roles (set (get-in application [:application/user-roles user]))] (if (seq specific-roles) specific-roles #{:everyone-else}))) (deftest test-user-roles (testing "give first role" - (is (= {::user-roles {"user" #{:role-1}}} + (is (= {:application/user-roles {"user" #{:role-1}}} (-> {} (give-role-to-user :role-1 "user"))))) (testing "give more roles" - (is (= {::user-roles {"user" #{:role-1 :role-2}}} + (is (= {:application/user-roles {"user" #{:role-1 :role-2}}} (-> {} (give-role-to-user :role-1 "user") (give-role-to-user :role-2 "user"))))) (testing "remove some roles" - (is (= {::user-roles {"user" #{:role-1}}} + (is (= {:application/user-roles {"user" #{:role-1}}} (-> {} (give-role-to-user :role-1 "user") (give-role-to-user :role-2 "user") (remove-role-from-user :role-2 "user"))))) (testing "remove all roles" - (is (= {::user-roles {}} + (is (= {:application/user-roles {}} (-> {} (give-role-to-user :role-1 "user") (remove-role-from-user :role-1 "user"))))) (testing "give a role to multiple users" - (is (= {::user-roles {"user-1" #{:role-1} - "user-2" #{:role-1}}} + (is (= {:application/user-roles {"user-1" #{:role-1} + "user-2" #{:role-1}}} (-> {} (give-role-to-users :role-1 ["user-1" "user-2"]))))) (testing "multiple users, get the roles of a single user" @@ -78,39 +78,39 @@ [application permission-map] (reduce (fn [application [role permissions]] (assert (keyword? role) {:role role}) - (assoc-in application [::role-permissions role] (set permissions))) + (assoc-in application [:application/role-permissions role] (set permissions))) application permission-map)) (deftest test-update-role-permissions (testing "adding" - (is (= {::role-permissions {:role #{:foo :bar}}} + (is (= {:application/role-permissions {:role #{:foo :bar}}} (-> {} (update-role-permissions {:role [:foo :bar]}))))) (testing "updating" - (is (= {::role-permissions {:role #{:gazonk}}} + (is (= {:application/role-permissions {:role #{:gazonk}}} (-> {} (update-role-permissions {:role [:foo :bar]}) (update-role-permissions {:role [:gazonk]}))))) (testing "removing" - (is (= {::role-permissions {:role #{}}} + (is (= {:application/role-permissions {:role #{}}} (-> {} (update-role-permissions {:role [:foo :bar]}) (update-role-permissions {:role []})))) - (is (= {::role-permissions {:role #{}}} + (is (= {:application/role-permissions {:role #{}}} (-> {} (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}}} + (is (= {:application/role-permissions {:role-1 #{:foo} + :role-2 #{:bar}}} (-> {} (update-role-permissions {:role-1 [:foo] :role-2 [:bar]}))))) (testing "does not alter unrelated roles" - (is (= {::role-permissions {:unrelated #{:foo} - :role #{:gazonk}}} + (is (= {:application/role-permissions {:unrelated #{:foo} + :role #{:gazonk}}} (-> {} (update-role-permissions {:unrelated [:foo] :role [:bar]}) @@ -136,7 +136,7 @@ (set (map :permission rules)))))) (defn- map-permissions [application f] - (update application ::role-permissions #(map-kv-vals f %))) + (update application :application/role-permissions #(map-kv-vals f %))) (defn- permissions-for-role [rules role] (set/union (get rules role #{}) @@ -154,16 +154,16 @@ (update-role-permissions {:role-1 [:foo :bar]}) (update-role-permissions {:role-2 [:foo :bar]}))] (testing "disallow a permission for all roles" - (is (= {::role-permissions {:role-1 #{:bar} - :role-2 #{:bar}}} + (is (= {:application/role-permissions {:role-1 #{:bar} + :role-2 #{:bar}}} (blacklist app (compile-rules [{:permission :foo}]))))) (testing "disallow a permission for a single role" - (is (= {::role-permissions {:role-1 #{:bar} - :role-2 #{:foo :bar}}} + (is (= {:application/role-permissions {:role-1 #{:bar} + :role-2 #{:foo :bar}}} (blacklist app (compile-rules [{:role :role-1 :permission :foo}]))))) (testing "multiple rules" - (is (= {::role-permissions {:role-1 #{:bar} - :role-2 #{:foo}}} + (is (= {:application/role-permissions {:role-1 #{:bar} + :role-2 #{:foo}}} (blacklist app (compile-rules [{:role :role-1 :permission :foo} {:role :role-2 :permission :bar}]))))))) @@ -179,16 +179,16 @@ (update-role-permissions {:role-1 [:foo :bar]}) (update-role-permissions {:role-2 [:foo :bar]}))] (testing "allow a permission for all roles" - (is (= {::role-permissions {:role-1 #{:foo} - :role-2 #{:foo}}} + (is (= {:application/role-permissions {:role-1 #{:foo} + :role-2 #{:foo}}} (whitelist app (compile-rules [{:permission :foo}]))))) (testing "allow a permission for a single role" - (is (= {::role-permissions {:role-1 #{:foo} - :role-2 #{}}} + (is (= {:application/role-permissions {:role-1 #{:foo} + :role-2 #{}}} (whitelist app (compile-rules [{:role :role-1 :permission :foo}]))))) (testing "multiple rules" - (is (= {::role-permissions {:role-1 #{:foo} - :role-2 #{:bar}}} + (is (= {:application/role-permissions {:role-1 #{:foo} + :role-2 #{:bar}}} (whitelist app (compile-rules [{:role :role-1 :permission :foo} {:role :role-2 :permission :bar}]))))))) @@ -199,7 +199,7 @@ [application user] (->> (user-roles application user) (mapcat (fn [role] - (get-in application [::role-permissions role]))) + (get-in application [:application/role-permissions role]))) set)) (deftest test-user-permissions @@ -222,4 +222,4 @@ (user-permissions "user")))))) (defn cleanup [application] - (dissoc application ::user-roles ::role-permissions)) + (dissoc application :application/user-roles :application/role-permissions)) diff --git a/test/clj/rems/api/test_applications.clj b/test/clj/rems/api/test_applications.clj index 97080f99bf..bad6c6fd90 100644 --- a/test/clj/rems/api/test_applications.clj +++ b/test/clj/rems/api/test_applications.clj @@ -1241,9 +1241,9 @@ api-key applicant)))) (testing "reporter can get raw application" (is (= {:application/id app-id - :rems.permissions/user-roles {(keyword applicant) ["applicant"] - (keyword handler) ["handler"] - (keyword reporter) ["reporter"]}} + :application/user-roles {(keyword applicant) ["applicant"] + (keyword handler) ["handler"] + (keyword reporter) ["reporter"]}} (-> (api-call :get (str "/api/applications/" app-id "/raw") nil api-key reporter) - (select-keys [:application/id :rems.permissions/user-roles]))))))) + (select-keys [:application/id :application/user-roles]))))))) diff --git a/test/clj/rems/application/test_model.clj b/test/clj/rems/application/test_model.clj index 816f2fac75..835d17b526 100644 --- a/test/clj/rems/application/test_model.clj +++ b/test/clj/rems/application/test_model.clj @@ -263,7 +263,7 @@ events) (defn state-role-permissions [application] - (->> (:rems.permissions/role-permissions application) + (->> (:application/role-permissions application) (map (fn [[role permissions]] {:state (:application/state application) :role role @@ -1142,8 +1142,8 @@ :event/actor "handler" :application/comment "looks good" :event/actor-attributes {:userid "handler" :email "handler@example.com" :name "Handler"}}] - :rems.permissions/user-roles {"handler" #{:handler}, "reporter1" #{:reporter}} - :rems.permissions/role-permissions nil + :application/user-roles {"handler" #{:handler}, "reporter1" #{:reporter}} + :application/role-permissions nil :application/description "foo" :application/forms [{:form/id 40 :form/title "form title" From af7fe9d1229e1c3a40a9643c8886c2398889d814 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 15:46:46 +0300 Subject: [PATCH 15/35] refactor: get-application-raw instead ofget-unrestricted-application at most use sites --- src/clj/rems/application/approver_bot.clj | 2 +- src/clj/rems/application/rejecter_bot.clj | 2 +- src/clj/rems/application/search.clj | 2 +- src/clj/rems/email/core.clj | 2 +- src/clj/rems/event_notification.clj | 5 +---- test/clj/rems/api/test_blacklist.clj | 2 +- test/clj/rems/api/test_catalogue_items.clj | 2 +- test/clj/rems/api/test_workflows.clj | 4 ++-- test/clj/rems/application/test_search.clj | 2 +- test/clj/rems/db/test_csv.clj | 4 ++-- test/clj/rems/db/test_entitlements.clj | 2 +- 11 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/clj/rems/application/approver_bot.clj b/src/clj/rems/application/approver_bot.clj index 9085ea7667..5d551f6c56 100644 --- a/src/clj/rems/application/approver_bot.clj +++ b/src/clj/rems/application/approver_bot.clj @@ -19,5 +19,5 @@ :comment ""}])) (defn run-approver-bot [new-events] - (doall (mapcat #(generate-commands % (applications/get-unrestricted-application (:application/id %))) + (doall (mapcat #(generate-commands % (applications/get-application-raw (:application/id %))) new-events))) diff --git a/src/clj/rems/application/rejecter_bot.clj b/src/clj/rems/application/rejecter_bot.clj index 534d2ec201..d653c08c9e 100644 --- a/src/clj/rems/application/rejecter_bot.clj +++ b/src/clj/rems/application/rejecter_bot.clj @@ -19,5 +19,5 @@ :actor bot-userid}])) (defn run-rejecter-bot [new-events] - (doall (mapcat #(generate-commands % (applications/get-unrestricted-application (:application/id %))) + (doall (mapcat #(generate-commands % (applications/get-application-raw (:application/id %))) new-events))) diff --git a/src/clj/rems/application/search.clj b/src/clj/rems/application/search.clj index 48aa91799b..26ac4a57c5 100644 --- a/src/clj/rems/application/search.clj +++ b/src/clj/rems/application/search.clj @@ -104,7 +104,7 @@ (let [app-ids (distinct (map :application/id events))] (log/info "Start indexing" (count app-ids) "applications...") (doseq [app-id app-ids] - (index-application! writer (applications/get-unrestricted-application app-id))) + (index-application! writer (applications/get-application-raw app-id))) (log/info "Finished indexing" (count app-ids) "applications"))) (.maybeRefresh searcher-manager) (swap! search-index assoc ::last-processed-event-id (:event/id (last events))))))) diff --git a/src/clj/rems/email/core.clj b/src/clj/rems/email/core.clj index c0e19e3ba8..78da677593 100644 --- a/src/clj/rems/email/core.clj +++ b/src/clj/rems/email/core.clj @@ -21,7 +21,7 @@ (defn- event-to-emails [event] (when-let [app-id (:application/id event)] (template/event-to-emails (rems.application.model/enrich-event event users/get-user (constantly nil)) - (applications/get-unrestricted-application app-id)))) + (applications/get-application-raw app-id)))) (defn- enqueue-email! [email] (outbox/put! {:outbox/type :email diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index 73e2f4c760..a5c6b9a59f 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -51,10 +51,7 @@ (defn queue-notifications! [events] (doseq [event events] - (let [;; TODO: get-unrestricted-application doesn't have a public - ;; schema and includes internal stuff like ::latest-review-request-by-user. - ;; Need to figure out a non-user-specific version of get-application - application (applications/get-unrestricted-application (:application/id event)) + (let [application (applications/get-application-raw (:application/id event)) event-with-app (assoc event :event/application application) body (json/generate-string event-with-app)] (when-let [targets (seq (get rems.config/env :event-notification-targets))] diff --git a/test/clj/rems/api/test_blacklist.clj b/test/clj/rems/api/test_blacklist.clj index 7e715d0068..0196fa5dc9 100644 --- a/test/clj/rems/api/test_blacklist.clj +++ b/test/clj/rems/api/test_blacklist.clj @@ -55,7 +55,7 @@ cat-id (test-data/create-catalogue-item! {:resource-id res-id-2}) app-id (test-data/create-application! {:catalogue-item-ids [cat-id] :actor "user2"}) - get-app #(applications/get-unrestricted-application app-id)] + get-app #(applications/get-application-raw app-id)] (testing "initially no blacklist" (is (= [] (fetch {}))) (is (= [] diff --git a/test/clj/rems/api/test_catalogue_items.clj b/test/clj/rems/api/test_catalogue_items.clj index 52c2c2b7ff..b0376883b2 100644 --- a/test/clj/rems/api/test_catalogue_items.clj +++ b/test/clj/rems/api/test_catalogue_items.clj @@ -93,7 +93,7 @@ (is (:success create)) (let [app-id (test-data/create-application! {:catalogue-item-ids [id] :actor "alice"}) - get-app #(applications/get-unrestricted-application app-id)] + get-app #(applications/get-application-raw app-id)] (is (= {:sv "http://info.se"} (:catalogue-item/infourl (first (:application/resources (get-app)))))) diff --git a/test/clj/rems/api/test_workflows.clj b/test/clj/rems/api/test_workflows.clj index 30493ad734..cc8b9833bf 100644 --- a/test/clj/rems/api/test_workflows.clj +++ b/test/clj/rems/api/test_workflows.clj @@ -166,7 +166,7 @@ (fn [app] (set (mapv :userid (get-in app [:application/workflow :workflow.dynamic/handlers]))))] (sync-with-database-time) (testing "application is initialized with the correct set of handlers" - (let [app (applications/get-unrestricted-application app-id)] + (let [app (applications/get-application-raw app-id)] (is (= #{"handler" "carl"} (application->handler-user-ids app))))) @@ -207,7 +207,7 @@ handler)))) (testing "application is updated when handlers are changed" - (let [app (applications/get-unrestricted-application app-id)] + (let [app (applications/get-application-raw app-id)] (is (= #{"owner" "alice"} (application->handler-user-ids app))))))) diff --git a/test/clj/rems/application/test_search.clj b/test/clj/rems/application/test_search.clj index 40606d74df..16b73860f3 100644 --- a/test/clj/rems/application/test_search.clj +++ b/test/clj/rems/application/test_search.clj @@ -39,7 +39,7 @@ (testing "find by ID" (let [app-id (test-data/create-application! {:actor "alice"}) - app (applications/get-unrestricted-application app-id)] + app (applications/get-application-raw app-id)] (is (= #{app-id} (search/find-applications (str app-id))) "app ID, any field") (is (= #{app-id} (search/find-applications (str "id:" app-id))) "app ID") (is (= #{app-id} (search/find-applications (str "id:\"" (:application/external-id app) "\""))) "external ID"))) diff --git a/test/clj/rems/db/test_csv.clj b/test/clj/rems/db/test_csv.clj index d57a398265..156ae0caff 100644 --- a/test/clj/rems/db/test_csv.clj +++ b/test/clj/rems/db/test_csv.clj @@ -85,8 +85,8 @@ :form-id form-id}) app-id (test-data/create-application! {:catalogue-item-ids [cat-id] :actor applicant}) - external-id (:application/external-id (applications/get-unrestricted-application app-id)) - get-application #(applications/get-unrestricted-application app-id)] + external-id (:application/external-id (applications/get-application-raw app-id)) + get-application #(applications/get-application-raw app-id)] (testing "draft applications not included as default" (is (= "" diff --git a/test/clj/rems/db/test_entitlements.clj b/test/clj/rems/db/test_entitlements.clj index 61cb1a55d3..c8b132348b 100644 --- a/test/clj/rems/db/test_entitlements.clj +++ b/test/clj/rems/db/test_entitlements.clj @@ -157,7 +157,7 @@ :accepted-licenses [lic-id1]}) ; only accept some licenses (is (= {applicant #{lic-id1 lic-id2} member #{lic-id1}} - (:application/accepted-licenses (applications/get-unrestricted-application app-id)))) + (:application/accepted-licenses (applications/get-application-raw app-id)))) (entitlements/process-outbox!) From a58100eea8c1f50837961ec9084b2dd6bd8e967b Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 15:57:36 +0300 Subject: [PATCH 16/35] refactor: rename get-application -> get-application-for-user --- src/clj/rems/api/applications.clj | 4 +- src/clj/rems/api/services/attachment.clj | 4 +- src/clj/rems/api/services/licenses.clj | 2 +- src/clj/rems/db/applications.clj | 3 +- src/clj/rems/db/test_data.clj | 4 +- test/clj/rems/api/test_applications.clj | 52 ++++++++++++------------ test/clj/rems/test_db.clj | 2 +- test/clj/rems/test_pdf.clj | 4 +- test/clj/rems/test_performance.clj | 2 +- 9 files changed, 38 insertions(+), 39 deletions(-) diff --git a/src/clj/rems/api/applications.clj b/src/clj/rems/api/applications.clj index 601dbdac06..2270c26ad4 100644 --- a/src/clj/rems/api/applications.clj +++ b/src/clj/rems/api/applications.clj @@ -250,7 +250,7 @@ :path-params [application-id :- (describe s/Int "application id")] :responses {200 {:schema Application} 404 {:schema s/Str :description "Not found"}} - (if-let [app (applications/get-application (getx-user-id) application-id)] + (if-let [app (applications/get-application-for-user (getx-user-id) application-id)] (ok app) (api-util/not-found-json-response))) @@ -282,7 +282,7 @@ :roles #{:logged-in} :path-params [application-id :- (describe s/Int "application id")] :produces ["application/pdf"] - (if-let [app (applications/get-application (getx-user-id) application-id)] + (if-let [app (applications/get-application-for-user (getx-user-id) application-id)] (with-language context/*lang* #(-> app (pdf/application-to-pdf-bytes) diff --git a/src/clj/rems/api/services/attachment.clj b/src/clj/rems/api/services/attachment.clj index 3a6b4ea5a1..90081d949f 100644 --- a/src/clj/rems/api/services/attachment.clj +++ b/src/clj/rems/api/services/attachment.clj @@ -27,7 +27,7 @@ (= user-id (:attachment/user attachment)) attachment - (contains-attachment? (applications/get-application user-id (:application/id attachment)) + (contains-attachment? (applications/get-application-for-user user-id (:application/id attachment)) attachment-id) attachment @@ -35,7 +35,7 @@ (throw-forbidden)))) (defn add-application-attachment [user-id application-id file] - (let [application (applications/get-application user-id application-id)] + (let [application (applications/get-application-for-user user-id application-id)] (when-not (some (set/union commands/commands-with-comments #{:application.command/save-draft}) (:application/permissions application)) diff --git a/src/clj/rems/api/services/licenses.clj b/src/clj/rems/api/services/licenses.clj index 0b555757c2..5d51d1f702 100644 --- a/src/clj/rems/api/services/licenses.clj +++ b/src/clj/rems/api/services/licenses.clj @@ -50,7 +50,7 @@ :attachment/type (:type attachment)})) (defn get-application-license-attachment [user-id application-id license-id language] - (when-let [app (applications/get-application user-id application-id)] + (when-let [app (applications/get-application-for-user user-id application-id)] (when-let [license (some #(when (= license-id (:license/id %)) %) (:application/licenses app))] (when-let [attachment-id (get-in license [:license/attachment-id language])] diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj index 7f0781202d..8d5f8974bc 100644 --- a/src/clj/rems/db/applications.clj +++ b/src/clj/rems/db/applications.clj @@ -93,7 +93,6 @@ ;; TODO rename ;; get-unrestricted-application -> get-application-internal ;; get-application-raw -> get-application -;; get-application -> get-application-for-user (defn get-unrestricted-application "Returns the full application state without any user permission @@ -110,7 +109,7 @@ (when-let [application (get-unrestricted-application application-id)] (model/hide-non-public-information application))) -(defn get-application +(defn get-application-for-user "Returns the part of application state which the specified user is allowed to see. Suitable for returning from public APIs as-is." [user-id application-id] diff --git a/src/clj/rems/db/test_data.clj b/src/clj/rems/db/test_data.clj index 8c756a86a8..5d9fa23eb5 100644 --- a/src/clj/rems/db/test_data.clj +++ b/src/clj/rems/db/test_data.clj @@ -245,7 +245,7 @@ :time (or time (time/now))}) (defn fill-form! [{:keys [application-id actor field-value optional-fields] :as command}] - (let [app (applications/get-application actor application-id)] + (let [app (applications/get-application-for-user actor application-id)] (command! (assoc (base-command command) :type :application.command/save-draft :field-values (for [form (:application/forms app) @@ -263,7 +263,7 @@ (or field-value "x"))}))))) (defn accept-licenses! [{:keys [application-id actor] :as command}] - (let [app (applications/get-application actor application-id)] + (let [app (applications/get-application-for-user actor application-id)] (command! (assoc (base-command command) :type :application.command/accept-licenses :accepted-licenses (map :license/id (:application/licenses app)))))) diff --git a/test/clj/rems/api/test_applications.clj b/test/clj/rems/api/test_applications.clj index bad6c6fd90..6db1082fd1 100644 --- a/test/clj/rems/api/test_applications.clj +++ b/test/clj/rems/api/test_applications.clj @@ -47,7 +47,7 @@ handler read-ok-body)) -(defn- get-application [app-id user-id] +(defn- get-application-for-user [app-id user-id] (-> (request :get (str "/api/applications/" app-id)) (authenticate "42" user-id) handler @@ -171,7 +171,7 @@ :application-id application-id})))) (testing "getting application as applicant" - (let [application (get-application application-id user-id)] + (let [application (get-application-for-user application-id user-id)] (is (= "workflow/master" (get-in application [:application/workflow :workflow/type]))) (is (= ["application.event/created" "application.event/licenses-accepted" @@ -185,7 +185,7 @@ (set (get application :application/permissions)))))) (testing "getting application as handler" - (let [application (get-application application-id handler-id)] + (let [application (get-application-for-user application-id handler-id)] (is (= "workflow/master" (get-in application [:application/workflow :workflow/type]))) (is (= #{"application.command/request-review" "application.command/request-decision" @@ -207,7 +207,7 @@ (testing "disabling a command" (with-redefs [rems.config/env (assoc rems.config/env :disable-commands [:application.command/remark])] (testing "handler doesn't see hidden command" - (let [application (get-application application-id handler-id)] + (let [application (get-application-for-user application-id handler-id)] (is (= "workflow/master" (get-in application [:application/workflow :workflow/type]))) (is (= #{"application.command/request-review" "application.command/request-decision" @@ -254,7 +254,7 @@ {:type :application.command/assign-external-id :application-id application-id :external-id "abc123"}))) - (let [application (get-application application-id handler-id)] + (let [application (get-application-for-user application-id handler-id)] (is (= "abc123" (:application/external-id application))))) (testing "application can be returned" @@ -282,7 +282,7 @@ :application-id application-id :comment "What am I commenting on?"})))) (testing "review with request" - (let [eventcount (count (get (get-application application-id handler-id) :events))] + (let [eventcount (count (get (get-application-for-user application-id handler-id) :events))] (testing "requesting review" (is (= {:success true} (send-command handler-id {:type :application.command/request-review @@ -295,7 +295,7 @@ :application-id application-id :comment "Yeah, I dunno"})))) (testing "review was linked to request" - (let [application (get-application application-id handler-id) + (let [application (get-application-for-user application-id handler-id) request-event (get-in application [:application/events eventcount]) review-event (get-in application [:application/events (inc eventcount)])] (is (= (:application/request-id request-event) @@ -303,14 +303,14 @@ (testing "adding and then accepting additional licenses" (testing "add licenses" - (let [application (get-application application-id user-id)] + (let [application (get-application-for-user application-id user-id)] (is (= #{license-id1 license-id2} (license-ids-for-application application))) (is (= {:success true} (send-command handler-id {:type :application.command/add-licenses :application-id application-id :licenses [license-id4] :comment "Please approve these new terms"}))) - (let [application (get-application application-id user-id)] + (let [application (get-application-for-user application-id user-id)] (is (= #{license-id1 license-id2 license-id4} (license-ids-for-application application)))))) (testing "applicant accepts the additional licenses" (is (= {:success true} (send-command user-id @@ -319,7 +319,7 @@ :accepted-licenses [license-id4]}))))) (testing "changing resources as handler" - (let [application (get-application application-id user-id)] + (let [application (get-application-for-user application-id user-id)] (is (= #{cat-item-id2} (catalogue-item-ids-for-application application))) (is (= #{license-id1 license-id2 license-id4} (license-ids-for-application application))) (is (= {:success true} (send-command handler-id @@ -327,7 +327,7 @@ :application-id application-id :catalogue-item-ids [cat-item-id3] :comment "Here are the correct resources"}))) - (let [application (get-application application-id user-id)] + (let [application (get-application-for-user application-id user-id)] (is (= #{cat-item-id3} (catalogue-item-ids-for-application application))) ;; TODO: The previously added licenses should probably be retained in the licenses after changing resources. (is (= #{license-id3} (license-ids-for-application application)))))) @@ -337,7 +337,7 @@ {:type :application.command/change-resources :application-id application-id :catalogue-item-ids [cat-item-id2]}))) - (let [application (get-application application-id user-id)] + (let [application (get-application-for-user application-id user-id)] (is (= #{cat-item-id2} (catalogue-item-ids-for-application application))) (is (= #{license-id1 license-id2} (license-ids-for-application application))))) @@ -367,9 +367,9 @@ (is (= {:success true} (send-command handler-id {:type :application.command/approve :application-id application-id :comment ""}))) - (let [handler-data (get-application application-id handler-id) + (let [handler-data (get-application-for-user application-id handler-id) handler-event-types (map :event/type (get handler-data :application/events)) - applicant-data (get-application application-id user-id) + applicant-data (get-application-for-user application-id user-id) applicant-event-types (map :event/type (get applicant-data :application/events))] (testing "handler can see all events" (is (= {:application/id application-id @@ -434,7 +434,7 @@ (testing "creating" (is (some? application-id)) - (let [created (get-application application-id user-id)] + (let [created (get-application-for-user application-id user-id)] (is (= "application.state/draft" (get created :application/state))))) (testing "getting application as other user is forbidden" @@ -453,7 +453,7 @@ (is (= {:success true} (send-command user-id {:type :application.command/submit :application-id application-id}))) - (let [submitted (get-application application-id user-id)] + (let [submitted (get-application-for-user application-id user-id)] (is (= "application.state/submitted" (get submitted :application/state))) (is (= ["application.event/created" "application.event/submitted"] @@ -467,7 +467,7 @@ :application-id application-id :comment ""}))) (is (= "application.state/closed" - (:application/state (get-application application-id user-id)))))) + (:application/state (get-application-for-user application-id user-id)))))) (deftest test-application-submit (let [owner "owner" @@ -578,7 +578,7 @@ "application.command/save-draft" "application.command/submit" "application.command/uninvite-member"} - (set (:application/permissions (get-application app-id applicant)))))) + (set (:application/permissions (get-application-for-user app-id applicant)))))) (testing "submit" (is (= {:success true} (send-command applicant {:type :application.command/submit @@ -588,7 +588,7 @@ "application.command/copy-as-new" "application.command/remove-member" "application.command/uninvite-member"} - (set (:application/permissions (get-application app-id applicant)))))) + (set (:application/permissions (get-application-for-user app-id applicant)))))) (testing "handler's commands" (is (= #{"application.command/add-licenses" "application.command/add-member" @@ -603,7 +603,7 @@ "application.command/return" "application.command/uninvite-member" "see-everything"} - (set (:application/permissions (get-application app-id handler)))))) + (set (:application/permissions (get-application-for-user app-id handler)))))) (testing "request decision" (is (= {:success true} (send-command handler {:type :application.command/request-decision @@ -615,7 +615,7 @@ "application.command/reject" "application.command/remark" "see-everything"} - (set (:application/permissions (get-application app-id decider)))))) + (set (:application/permissions (get-application-for-user app-id decider)))))) (testing "approve" (is (= {:success true} (send-command decider {:type :application.command/approve @@ -772,7 +772,7 @@ (is (:success response)) (is (number? new-app-id)) (testing "and fetching the copied attachent" - (let [new-app (get-application new-app-id user-id) + (let [new-app (get-application-for-user new-app-id user-id) new-id (get-in new-app [:application/attachments 0 :attachment/id])] (is (number? new-id)) (is (not= id new-id)) @@ -857,7 +857,7 @@ :attachments [{:attachment/id attachment-id}]})))))) (testing "applicant can see attachment" - (let [app (get-application application-id applicant-id) + (let [app (get-application-for-user application-id applicant-id) remark-event (last (:application/events app)) attachment-id (:attachment/id (first (:event/attachments remark-event)))] (is (number? attachment-id)) @@ -940,7 +940,7 @@ {:attachment/id id2}]}))))) (testing "applicant can see the three new attachments" - (let [app (get-application application-id applicant-id) + (let [app (get-application-for-user application-id applicant-id) [close-event approve-event] (reverse (:application/events app)) [close-id1 close-id2] (map :attachment/id (:event/attachments close-event)) [approve-id] (map :attachment/id (:event/attachments approve-event))] @@ -962,7 +962,7 @@ "handler-approve.txt" "handler-close1.txt" "handler-close2.txt"] - (mapv :attachment/filename (:application/attachments (get-application application-id applicant-id)))))) + (mapv :attachment/filename (:application/attachments (get-application-for-user application-id applicant-id)))))) (testing "handler" (is (= ["handler-public-remark.txt" "reviewer-review.txt" @@ -970,7 +970,7 @@ "handler-approve.txt" "handler-close1.txt" "handler-close2.txt"] - (mapv :attachment/filename (:application/attachments (get-application application-id handler-id))))))))) + (mapv :attachment/filename (:application/attachments (get-application-for-user application-id handler-id))))))))) (deftest test-application-api-license-attachments (let [api-key "42" diff --git a/test/clj/rems/test_db.clj b/test/clj/rems/test_db.clj index 2c199fc592..8827bb1b0b 100644 --- a/test/clj/rems/test_db.clj +++ b/test/clj/rems/test_db.clj @@ -49,7 +49,7 @@ :application-id app-id :actor "handler" :comment ""}) - (is (= :application.state/approved (:application/state (applications/get-application applicant app-id)))) + (is (= :application.state/approved (:application/state (applications/get-application-for-user applicant app-id)))) (is (= ["resid111" "resid222"] (sort (map :resid (db/get-entitlements {:application app-id})))) "should create entitlements for both resources"))) diff --git a/test/clj/rems/test_pdf.clj b/test/clj/rems/test_pdf.clj index cc3cfeb143..77668e9ff4 100644 --- a/test/clj/rems/test_pdf.clj +++ b/test/clj/rems/test_pdf.clj @@ -120,8 +120,8 @@ (fn [] (with-fixed-time (time/date-time 2010) (fn [] - (#'pdf/render-application (applications/get-application handler application-id))))))))) + (#'pdf/render-application (applications/get-application-for-user handler application-id))))))))) (testing "pdf rendering succeeds" (is (some? (with-language :en - #(pdf/application-to-pdf-bytes (applications/get-application handler application-id)))))))) + #(pdf/application-to-pdf-bytes (applications/get-application-for-user handler application-id)))))))) diff --git a/test/clj/rems/test_performance.clj b/test/clj/rems/test_performance.clj index 2d39cab7eb..a3d050aa76 100644 --- a/test/clj/rems/test_performance.clj +++ b/test/clj/rems/test_performance.clj @@ -79,7 +79,7 @@ (println "cache size" (mm/measure applications/all-applications-cache)))) (defn benchmark-get-application [] - (let [test-get-application #(applications/get-application "developer" 12)] + (let [test-get-application #(applications/get-application-for-user "developer" 12)] (run-benchmarks [{:name "get-application" :benchmark test-get-application}]))) From bb5f8a9a0c99355517aad5df889da28454cbb0f4 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 16:03:51 +0300 Subject: [PATCH 17/35] refactor: rename get-unrestricted-application -> get-application-internal --- src/clj/rems/api/services/command.clj | 4 ++-- src/clj/rems/db/applications.clj | 7 +++---- src/clj/rems/db/entitlements.clj | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/clj/rems/api/services/command.clj b/src/clj/rems/api/services/command.clj index a5eae91378..27447aff60 100644 --- a/src/clj/rems/api/services/command.clj +++ b/src/clj/rems/api/services/command.clj @@ -28,7 +28,7 @@ (defn- revokes-to-blacklist [new-events] (doseq [event new-events] (when (= :application.event/revoked (:event/type event)) - (let [application (applications/get-unrestricted-application (:application/id event))] + (let [application (applications/get-application-internal (:application/id event))] (doseq [resource (:application/resources application)] (blacklist/add-users-to-blacklist! {:users (application-util/applicant-and-members application) :resource/ext-id (:resource/ext-id resource) @@ -77,7 +77,7 @@ (throw (TryAgainException. e)) (throw e)))) (let [app (when-let [app-id (:application-id cmd)] - (applications/get-unrestricted-application app-id)) + (applications/get-application-internal app-id)) result (commands/handle-command cmd app command-injections)] (when-not (:errors result) (doseq [event (:events result)] diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj index 8d5f8974bc..ab905e36fb 100644 --- a/src/clj/rems/db/applications.clj +++ b/src/clj/rems/db/applications.clj @@ -91,10 +91,9 @@ (blacklist/blacklisted? userid resource)))}) ;; TODO rename -;; get-unrestricted-application -> get-application-internal ;; get-application-raw -> get-application -(defn get-unrestricted-application +(defn get-application-internal "Returns the full application state without any user permission checks and filtering of sensitive information. Don't expose via APIs." [application-id] @@ -106,14 +105,14 @@ (defn get-application-raw "Full application state with internal information hidden. No personalized for any users. Suitable for public APIs" [application-id] - (when-let [application (get-unrestricted-application application-id)] + (when-let [application (get-application-internal application-id)] (model/hide-non-public-information application))) (defn get-application-for-user "Returns the part of application state which the specified user is allowed to see. Suitable for returning from public APIs as-is." [user-id application-id] - (when-let [application (get-unrestricted-application application-id)] + (when-let [application (get-application-internal application-id)] (or (model/apply-user-permissions application user-id) (throw-forbidden)))) diff --git a/src/clj/rems/db/entitlements.clj b/src/clj/rems/db/entitlements.clj index ed29b531d2..c29876e48c 100644 --- a/src/clj/rems/db/entitlements.clj +++ b/src/clj/rems/db/entitlements.clj @@ -190,5 +190,5 @@ :application.event/resources-changed :application.event/revoked} (:event/type event)) - (let [application (applications/get-unrestricted-application (:application/id event))] + (let [application (applications/get-application-internal (:application/id event))] (update-entitlements-for-application application (:event/actor event))))) From fa5d232743991a09f326dd552ffc80af5b62794f Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 16:07:47 +0300 Subject: [PATCH 18/35] refactor: rename get-application-raw -> get-application --- src/clj/rems/api/applications.clj | 2 +- src/clj/rems/application/approver_bot.clj | 2 +- src/clj/rems/application/rejecter_bot.clj | 2 +- src/clj/rems/application/search.clj | 2 +- src/clj/rems/db/applications.clj | 5 +---- src/clj/rems/email/core.clj | 2 +- src/clj/rems/event_notification.clj | 2 +- test/clj/rems/api/test_blacklist.clj | 2 +- test/clj/rems/api/test_catalogue_items.clj | 2 +- test/clj/rems/api/test_workflows.clj | 4 ++-- test/clj/rems/application/test_search.clj | 2 +- test/clj/rems/db/test_csv.clj | 4 ++-- test/clj/rems/db/test_entitlements.clj | 2 +- 13 files changed, 15 insertions(+), 18 deletions(-) diff --git a/src/clj/rems/api/applications.clj b/src/clj/rems/api/applications.clj index 2270c26ad4..c523ff283c 100644 --- a/src/clj/rems/api/applications.clj +++ b/src/clj/rems/api/applications.clj @@ -260,7 +260,7 @@ :path-params [application-id :- (describe s/Int "application id")] :responses {200 {:schema ApplicationRaw} 404 {:schema s/Str :description "Not found"}} - (if-let [app (applications/get-application-raw application-id)] + (if-let [app (applications/get-application application-id)] (ok app) (api-util/not-found-json-response))) diff --git a/src/clj/rems/application/approver_bot.clj b/src/clj/rems/application/approver_bot.clj index 5d551f6c56..1572cb4218 100644 --- a/src/clj/rems/application/approver_bot.clj +++ b/src/clj/rems/application/approver_bot.clj @@ -19,5 +19,5 @@ :comment ""}])) (defn run-approver-bot [new-events] - (doall (mapcat #(generate-commands % (applications/get-application-raw (:application/id %))) + (doall (mapcat #(generate-commands % (applications/get-application (:application/id %))) new-events))) diff --git a/src/clj/rems/application/rejecter_bot.clj b/src/clj/rems/application/rejecter_bot.clj index d653c08c9e..c131304f5d 100644 --- a/src/clj/rems/application/rejecter_bot.clj +++ b/src/clj/rems/application/rejecter_bot.clj @@ -19,5 +19,5 @@ :actor bot-userid}])) (defn run-rejecter-bot [new-events] - (doall (mapcat #(generate-commands % (applications/get-application-raw (:application/id %))) + (doall (mapcat #(generate-commands % (applications/get-application (:application/id %))) new-events))) diff --git a/src/clj/rems/application/search.clj b/src/clj/rems/application/search.clj index 26ac4a57c5..b5ece3843c 100644 --- a/src/clj/rems/application/search.clj +++ b/src/clj/rems/application/search.clj @@ -104,7 +104,7 @@ (let [app-ids (distinct (map :application/id events))] (log/info "Start indexing" (count app-ids) "applications...") (doseq [app-id app-ids] - (index-application! writer (applications/get-application-raw app-id))) + (index-application! writer (applications/get-application app-id))) (log/info "Finished indexing" (count app-ids) "applications"))) (.maybeRefresh searcher-manager) (swap! search-index assoc ::last-processed-event-id (:event/id (last events))))))) diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj index ab905e36fb..4d23abc27d 100644 --- a/src/clj/rems/db/applications.clj +++ b/src/clj/rems/db/applications.clj @@ -90,9 +90,6 @@ :blacklisted? #(cache/lookup-or-miss blacklist-cache [%1 %2] (fn [[userid resource]] (blacklist/blacklisted? userid resource)))}) -;; TODO rename -;; get-application-raw -> get-application - (defn get-application-internal "Returns the full application state without any user permission checks and filtering of sensitive information. Don't expose via APIs." @@ -102,7 +99,7 @@ nil ; application not found (model/build-application-view events fetcher-injections)))) -(defn get-application-raw +(defn get-application "Full application state with internal information hidden. No personalized for any users. Suitable for public APIs" [application-id] (when-let [application (get-application-internal application-id)] diff --git a/src/clj/rems/email/core.clj b/src/clj/rems/email/core.clj index 78da677593..7cb974d838 100644 --- a/src/clj/rems/email/core.clj +++ b/src/clj/rems/email/core.clj @@ -21,7 +21,7 @@ (defn- event-to-emails [event] (when-let [app-id (:application/id event)] (template/event-to-emails (rems.application.model/enrich-event event users/get-user (constantly nil)) - (applications/get-application-raw app-id)))) + (applications/get-application app-id)))) (defn- enqueue-email! [email] (outbox/put! {:outbox/type :email diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index a5c6b9a59f..be32d841e4 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -51,7 +51,7 @@ (defn queue-notifications! [events] (doseq [event events] - (let [application (applications/get-application-raw (:application/id event)) + (let [application (applications/get-application (:application/id event)) event-with-app (assoc event :event/application application) body (json/generate-string event-with-app)] (when-let [targets (seq (get rems.config/env :event-notification-targets))] diff --git a/test/clj/rems/api/test_blacklist.clj b/test/clj/rems/api/test_blacklist.clj index 0196fa5dc9..f68e607cf5 100644 --- a/test/clj/rems/api/test_blacklist.clj +++ b/test/clj/rems/api/test_blacklist.clj @@ -55,7 +55,7 @@ cat-id (test-data/create-catalogue-item! {:resource-id res-id-2}) app-id (test-data/create-application! {:catalogue-item-ids [cat-id] :actor "user2"}) - get-app #(applications/get-application-raw app-id)] + get-app #(applications/get-application app-id)] (testing "initially no blacklist" (is (= [] (fetch {}))) (is (= [] diff --git a/test/clj/rems/api/test_catalogue_items.clj b/test/clj/rems/api/test_catalogue_items.clj index b0376883b2..7c3d3ec88a 100644 --- a/test/clj/rems/api/test_catalogue_items.clj +++ b/test/clj/rems/api/test_catalogue_items.clj @@ -93,7 +93,7 @@ (is (:success create)) (let [app-id (test-data/create-application! {:catalogue-item-ids [id] :actor "alice"}) - get-app #(applications/get-application-raw app-id)] + get-app #(applications/get-application app-id)] (is (= {:sv "http://info.se"} (:catalogue-item/infourl (first (:application/resources (get-app)))))) diff --git a/test/clj/rems/api/test_workflows.clj b/test/clj/rems/api/test_workflows.clj index cc8b9833bf..a9389b9e95 100644 --- a/test/clj/rems/api/test_workflows.clj +++ b/test/clj/rems/api/test_workflows.clj @@ -166,7 +166,7 @@ (fn [app] (set (mapv :userid (get-in app [:application/workflow :workflow.dynamic/handlers]))))] (sync-with-database-time) (testing "application is initialized with the correct set of handlers" - (let [app (applications/get-application-raw app-id)] + (let [app (applications/get-application app-id)] (is (= #{"handler" "carl"} (application->handler-user-ids app))))) @@ -207,7 +207,7 @@ handler)))) (testing "application is updated when handlers are changed" - (let [app (applications/get-application-raw app-id)] + (let [app (applications/get-application app-id)] (is (= #{"owner" "alice"} (application->handler-user-ids app))))))) diff --git a/test/clj/rems/application/test_search.clj b/test/clj/rems/application/test_search.clj index 16b73860f3..3ef5880598 100644 --- a/test/clj/rems/application/test_search.clj +++ b/test/clj/rems/application/test_search.clj @@ -39,7 +39,7 @@ (testing "find by ID" (let [app-id (test-data/create-application! {:actor "alice"}) - app (applications/get-application-raw app-id)] + app (applications/get-application app-id)] (is (= #{app-id} (search/find-applications (str app-id))) "app ID, any field") (is (= #{app-id} (search/find-applications (str "id:" app-id))) "app ID") (is (= #{app-id} (search/find-applications (str "id:\"" (:application/external-id app) "\""))) "external ID"))) diff --git a/test/clj/rems/db/test_csv.clj b/test/clj/rems/db/test_csv.clj index 156ae0caff..5676592606 100644 --- a/test/clj/rems/db/test_csv.clj +++ b/test/clj/rems/db/test_csv.clj @@ -85,8 +85,8 @@ :form-id form-id}) app-id (test-data/create-application! {:catalogue-item-ids [cat-id] :actor applicant}) - external-id (:application/external-id (applications/get-application-raw app-id)) - get-application #(applications/get-application-raw app-id)] + external-id (:application/external-id (applications/get-application app-id)) + get-application #(applications/get-application app-id)] (testing "draft applications not included as default" (is (= "" diff --git a/test/clj/rems/db/test_entitlements.clj b/test/clj/rems/db/test_entitlements.clj index c8b132348b..000283aa96 100644 --- a/test/clj/rems/db/test_entitlements.clj +++ b/test/clj/rems/db/test_entitlements.clj @@ -157,7 +157,7 @@ :accepted-licenses [lic-id1]}) ; only accept some licenses (is (= {applicant #{lic-id1 lic-id2} member #{lic-id1}} - (:application/accepted-licenses (applications/get-application-raw app-id)))) + (:application/accepted-licenses (applications/get-application app-id)))) (entitlements/process-outbox!) From 6aae187f1b0c8db3ee4578e3a95192875eac6959 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 16:11:05 +0300 Subject: [PATCH 19/35] refactor: do less work in queue-notification! when no notification targets are set --- src/clj/rems/event_notification.clj | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index be32d841e4..2518a27446 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -50,11 +50,11 @@ (some? (some #{(:event/type event)} whitelist))))) (defn queue-notifications! [events] - (doseq [event events] - (let [application (applications/get-application (:application/id event)) - event-with-app (assoc event :event/application application) - body (json/generate-string event-with-app)] - (when-let [targets (seq (get rems.config/env :event-notification-targets))] + (when-let [targets (seq (get rems.config/env :event-notification-targets))] + (doseq [event events] + (let [application (applications/get-application (:application/id event)) + event-with-app (assoc event :event/application application) + body (json/generate-string event-with-app)] (doseq [target targets] (when (wants? target event) (add-to-outbox! (:url target) body))))))) From c8267704499f9e7fcf1e9c0e15be7587024cd029 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 17:10:25 +0300 Subject: [PATCH 20/35] test: tests for rems.event-notification --- src/clj/rems/event_notification.clj | 12 +- test/clj/rems/test_event_notification.clj | 158 ++++++++++++++++++++++ 2 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 test/clj/rems/test_event_notification.clj diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index 2518a27446..ef5e7db2be 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -1,6 +1,7 @@ (ns rems.event-notification (:require [clj-http.client :as http] [clj-time.core :as time] + [clojure.test :refer :all] [clojure.tools.logging :as log] [mount.core :as mount] [rems.api.schema :as schema] @@ -14,16 +15,17 @@ (try (let [response (http/put target {:body body + :throw-exceptions false :content-type :json :socket-timeout 2500 :conn-timeout 2500}) status (:status response)] (when-not (= 200 status) (log/error "Event notification response status" status) - (str "status " status))) + (str "failed: " status))) (catch Exception e (log/error "Event notification failed" e) - (str "exception " e)))) + "failed: exception"))) (defn process-outbox! [] (doseq [entry (outbox/get-entries {:type :event-notification :due-now? true})] @@ -49,6 +51,12 @@ (or (empty? whitelist) (some? (some #{(:event/type event)} whitelist))))) +(deftest test-wants? + (let [target {:url "whatever" :event-types [:application.event/submitted :application.event/approved]}] + (is (false? (wants? target {:event/type :application.event/created}))) + (is (true? (wants? target {:event/type :application.event/submitted}))) + (is (true? (wants? target {:event/type :application.event/approved}))))) + (defn queue-notifications! [events] (when-let [targets (seq (get rems.config/env :event-notification-targets))] (doseq [event events] diff --git a/test/clj/rems/test_event_notification.clj b/test/clj/rems/test_event_notification.clj new file mode 100644 index 0000000000..e1aa8eb46d --- /dev/null +++ b/test/clj/rems/test_event_notification.clj @@ -0,0 +1,158 @@ +(ns ^:integration rems.test-event-notification + (:require [clj-time.core :as time] + [clojure.test :refer :all] + [medley.core :refer [dissoc-in]] + [rems.config] + [rems.api.services.command :as command] + [rems.db.test-data :as test-data] + [rems.db.testing :refer [test-db-fixture rollback-db-fixture]] + [rems.event-notification :as event-notification] + [rems.json :as json] + [rems.testing-util :refer [fixed-time-fixture with-user]] + [stub-http.core :as stub])) + +(use-fixtures + :once + test-db-fixture + rollback-db-fixture) + +(deftest test-notify! + (with-open [server (stub/start! {"/ok" {:status 200} + "/broken" {:status 500} + "/timeout" {:status 200 :delay 5000}})] ;; timeout of 2500 in the code + (let [body "body"] + (testing "success" + (is (nil? (#'event-notification/notify! (str (:uri server) "/ok") body))) + ;; TODO check request + ) + (testing "error code" + (is (= "failed: 500" (#'event-notification/notify! (str (:uri server) "/broken") body)))) + (testing "timeout" + (is (= "failed: exception" (#'event-notification/notify! (str (:uri server) "/timeout") body)))) + (testing "invalid url" + (is (= "failed: exception" (#'event-notification/notify! "http://invalid/lol" body))))))) + +(deftest test-event-notification + ;; this is an integration test from commands to notifications + (with-open [server (stub/start! {"/created" {:status 200} + "/all" {:status 200}})] + (with-redefs [rems.config/env (assoc rems.config/env + :event-notification-targets [{:url (str (:uri server) "/created") + :event-types [:application.event/created]} + {:url (str (:uri server) "/all")}])] + (let [get-notifications #(doall + (for [r (stub/recorded-requests server)] + {:path (:path r) + :data (-> r + :body + (get "content") + json/parse-string + (dissoc-in [:event/application :application/events]) + ;; catalogue-item/start is set by the db and can't be easily fixed + (dissoc-in [:event/application :application/resources 0 :catalogue-item/start]))})) + form-id (test-data/create-form! {:form/title "notifications" + :form/fields [{:field/type :text + :field/id "field-1" + :field/title {:en "text field"} + :field/optional false}]}) + handler "handler" + workflow-id (test-data/create-workflow! {:title "wf" + :handlers [handler] + :type :workflow/default}) + ext-id "resres" + res-id (test-data/create-resource! {:resource-ext-id ext-id}) + cat-id (test-data/create-catalogue-item! {:form-id form-id + :resource-id res-id + :workflow-id workflow-id}) + applicant "alice" + app-id (:application-id (command/command! {:type :application.command/create + :actor applicant + :time (time/date-time 2001) + :catalogue-item-ids [cat-id]}))] + (testing "no notifications before outbox is processed" + (is (= [] + (stub/recorded-requests server)))) + (event-notification/process-outbox!) + (testing "created event gets sent to both endpoints" + (let [notifications (get-notifications)] + (is (= 2 (count notifications))) + (is (= #{"/created" "/all"} + (set (map :path notifications)))) + (is (= {:application/external-id "2001/1" + :application/id app-id + :event/time "2001-01-01T00:00:00.000Z" + :workflow/type "workflow/default" + :application/resources [{:resource/ext-id ext-id + :catalogue-item/id cat-id}] + :application/forms [{:form/id form-id}] + :workflow/id workflow-id + :event/actor applicant + :event/type "application.event/created" + :application/licenses [] + :event/application {:application/description "" + :application/invited-members [] + :application/last-activity "2001-01-01T00:00:00.000Z" + :application/attachments [] + :application/licenses [] + :application/created "2001-01-01T00:00:00.000Z" + :application/state "application.state/draft" + :application/role-permissions + {:everyone-else ["application.command/accept-invitation"] + :member ["application.command/copy-as-new" + "application.command/accept-licenses"] + :reporter ["see-everything"] + :applicant ["application.command/copy-as-new" + "application.command/invite-member" + "application.command/submit" + "application.command/remove-member" + "application.command/accept-licenses" + "application.command/uninvite-member" + "application.command/save-draft" + "application.command/close" + "application.command/change-resources"]} + :application/modified "2001-01-01T00:00:00.000Z" + :application/user-roles {:alice ["applicant"] :handler ["handler"]} + :application/external-id "2001/1" + :application/workflow {:workflow/type "workflow/default" + :workflow/id workflow-id + :workflow.dynamic/handlers + [{:email nil :userid "handler" :name nil}]} + :application/blacklist [] + :application/id app-id + :application/todo nil + :application/applicant {:email nil :userid "alice" :name nil} + :application/members [] + :application/resources [{:catalogue-item/end nil + :catalogue-item/expired false + :catalogue-item/enabled true + :resource/id res-id + :catalogue-item/title {} + :catalogue-item/infourl {} + :resource/ext-id ext-id + :catalogue-item/archived false + :catalogue-item/id cat-id}] + :application/accepted-licenses {} + :application/forms [{:form/fields [{:field/value "" + :field/type "text" + :field/title {:en "text field"} + :field/id "field-1" + :field/optional false + :field/visible true}] + :form/title "notifications" + :form/id form-id}]}} + (:data (first notifications)))) + (is (= (:data (first notifications)) + (:data (second notifications)))))) + (command/command! {:application-id app-id + :type :application.command/save-draft + :actor applicant + :time (time/date-time 2001) + :field-values [{:form form-id :field "field-1" :value "my value"}]}) + (event-notification/process-outbox!) + (testing "draft-saved event gets sent only to /all" + (let [requests (get-notifications) + req (last requests)] + (is (= 3 (count requests))) + (is (= "/all" (:path req))) + (is (= "application.event/draft-saved" + (:event/type (:data req)))))))))) From 766baf435198424b4d865da506ccf6f810919383 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 17:20:50 +0300 Subject: [PATCH 21/35] doc: docs/event-notification.md --- docs/event-notification.md | 35 +++++++++++++++++++++++++++++++++++ resources/config-defaults.edn | 2 ++ 2 files changed, 37 insertions(+) create mode 100644 docs/event-notification.md diff --git a/docs/event-notification.md b/docs/event-notification.md new file mode 100644 index 0000000000..aa05de8fa9 --- /dev/null +++ b/docs/event-notification.md @@ -0,0 +1,35 @@ +# Event notification + +REMS can be configured to send events notifications over HTTP. + +## Configuration + +The configuration option `:event-notification-targets` should be an array of targets, each containing: +- `:url`, the URL to send HTTP PUT methods to +- `:event-types`, an array of event types to send. This is optional, and a missing value means "send everything". + +An example: +``` +:event-notification-targets [{:url "http://events/everything"} + {:url "http://events/filtered" + :event-types [:application.event/created :application.event/submitted]}] +``` + +## Schema + +The body of the HTTP PUT request will be a JSON object that contains: + +- `"event/type"`: the type of the event, a string +- `"event/actor"`: who caused this event +- `"event/time"`: when the event occured +- `"application/id"`: the id of the application +- `"event/application"`: the state of the application, in the same format as the `/api/applications/:id/raw` endpoint returns (see Swagger docs) + +Other keys may also be present depending on the event type. + +## Error handling + +Event notifications are retried with exponential backoff for up to 12 +hours. Everything except a HTTP 200 status counts as a failure. +Failures are logged. You can also inspect the `outbox` db table for +the retry state of notifications. diff --git a/resources/config-defaults.edn b/resources/config-defaults.edn index da0a17e81c..4ed1138fdc 100644 --- a/resources/config-defaults.edn +++ b/resources/config-defaults.edn @@ -58,6 +58,8 @@ ;; a :url, and optionally :event-types to receive only the specified ;; event types. ;; + ;; See also: docs/event-notification.md + ;; ;; Example: ;; ;; :event-notification-targets [{:url "http://events/everything"} From 5c04f11850ca4ee0f5d32588beda616f7c2432cc Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 17:21:40 +0300 Subject: [PATCH 22/35] doc: update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ce19423c6..e62baa9b6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Changes since v2.11 ### Additions - REMS sessions now stay alive while the user is active in the browser (#2107). - The `/api/users/active` API lists which users have active sessions at the moment. +- Event notifications over HTTP. See [docs/event-notification.md](docs/event-notification.md) for details. (#2095) ## v2.10 "Riihitontuntie" 2020-04-06 From 61c11a350f42e53f95519e76ee5f074f9bb02034 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 7 Apr 2020 17:25:28 +0300 Subject: [PATCH 23/35] feat: schema for :application/{role-permissions,user-roles} --- src/clj/rems/api/schema.clj | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/clj/rems/api/schema.clj b/src/clj/rems/api/schema.clj index 9a3cff5cab..d9da16b87b 100644 --- a/src/clj/rems/api/schema.clj +++ b/src/clj/rems/api/schema.clj @@ -262,8 +262,8 @@ (-> Application (dissoc :application/permissions :application/roles) - (assoc :application/role-permissions s/Any ;; TODO schema for this - :application/user-roles s/Any))) ;; TODO schema for this + (assoc :application/role-permissions {s/Keyword #{s/Keyword}} + :application/user-roles {s/Str #{s/Keyword}}))) (s/defschema ApplicationOverview (dissoc Application From 50c95c162aeab1272fe4efdea6d50baa76e90142 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Thu, 9 Apr 2020 14:42:01 +0300 Subject: [PATCH 24/35] feat: configurable headers & timeout for event notifications --- docs/event-notification.md | 11 +--------- resources/config-defaults.edn | 13 ++++++++---- src/clj/rems/event_notification.clj | 15 +++++++++----- test/clj/rems/test_event_notification.clj | 25 ++++++++++++++++------- 4 files changed, 38 insertions(+), 26 deletions(-) diff --git a/docs/event-notification.md b/docs/event-notification.md index aa05de8fa9..6a47859d02 100644 --- a/docs/event-notification.md +++ b/docs/event-notification.md @@ -4,16 +4,7 @@ REMS can be configured to send events notifications over HTTP. ## Configuration -The configuration option `:event-notification-targets` should be an array of targets, each containing: -- `:url`, the URL to send HTTP PUT methods to -- `:event-types`, an array of event types to send. This is optional, and a missing value means "send everything". - -An example: -``` -:event-notification-targets [{:url "http://events/everything"} - {:url "http://events/filtered" - :event-types [:application.event/created :application.event/submitted]}] -``` +See `:event-notification-targets` in [config-defaults.edn](../resources/config-defaults.edn). ## Schema diff --git a/resources/config-defaults.edn b/resources/config-defaults.edn index 4ed1138fdc..caaedeb568 100644 --- a/resources/config-defaults.edn +++ b/resources/config-defaults.edn @@ -54,9 +54,11 @@ :remove nil :ga4gh nil} ;; Url where entitlements are pushed in ga4gh format, see https://github.com/ga4gh-duri/ga4gh-duri.github.io/ - ;; URLs to notify about new events. Each target should contain - ;; a :url, and optionally :event-types to receive only the specified - ;; event types. + ;; URLs to notify about new events. An array of targets. Targets can have keys: + ;; :url (mandatory) - the url to send HTTP PUT requests to + ;; :event-types (optional) - an array of event types to send. A missing value means "send everything". + ;; :timeout (optional) - timeout for the PUT in seconds. Defaults to 60s. + ;; :headers (optional) - a map of additional HTTP headers to send. ;; ;; See also: docs/event-notification.md ;; @@ -64,7 +66,10 @@ ;; ;; :event-notification-targets [{:url "http://events/everything"} ;; {:url "http://events/filtered" - ;; :event-types [:application.event/created :application.event/submitted]}] + ;; :event-types [:application.event/created :application.event/submitted] + ;; :timeout 120 + ;; :headers {"Authorization" "abc123" + ;; "X-Header" "value"}}] :event-notification-targets [] ;; Which database column to show as the application id. diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index ef5e7db2be..f4ee6861b3 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -9,16 +9,21 @@ [rems.db.applications :as applications] [rems.db.outbox :as outbox] [rems.json :as json] - [rems.scheduler :as scheduler])) + [rems.scheduler :as scheduler] + [rems.util :refer [getx]])) + +(def ^:private default-timeout 60) (defn- notify! [target body] (try - (let [response (http/put target + (let [timeout-ms (* 1000 (get target :timeout default-timeout)) + response (http/put (getx target :url) {:body body :throw-exceptions false :content-type :json - :socket-timeout 2500 - :conn-timeout 2500}) + :headers (get target :headers) + :socket-timeout timeout-ms + :conn-timeout timeout-ms}) status (:status response)] (when-not (= 200 status) (log/error "Event notification response status" status) @@ -65,4 +70,4 @@ body (json/generate-string event-with-app)] (doseq [target targets] (when (wants? target event) - (add-to-outbox! (:url target) body))))))) + (add-to-outbox! target body))))))) diff --git a/test/clj/rems/test_event_notification.clj b/test/clj/rems/test_event_notification.clj index e1aa8eb46d..7c6b022be3 100644 --- a/test/clj/rems/test_event_notification.clj +++ b/test/clj/rems/test_event_notification.clj @@ -19,18 +19,29 @@ (deftest test-notify! (with-open [server (stub/start! {"/ok" {:status 200} "/broken" {:status 500} - "/timeout" {:status 200 :delay 5000}})] ;; timeout of 2500 in the code + "/timeout" {:status 200 :delay 5000}})] (let [body "body"] (testing "success" - (is (nil? (#'event-notification/notify! (str (:uri server) "/ok") body))) - ;; TODO check request - ) + (is (nil? (#'event-notification/notify! {:url (str (:uri server) "/ok") + :headers {"additional-header" "value"}} + body))) + (let [[req & more] (stub/recorded-requests server)] + (is (empty? more)) + (is (= {:method "PUT" + :path "/ok" + :body {"content" body}} + (select-keys req [:method :path :body]))) + (is (= "value" (get-in req [:headers :additional-header]))))) (testing "error code" - (is (= "failed: 500" (#'event-notification/notify! (str (:uri server) "/broken") body)))) + (is (= "failed: 500" (#'event-notification/notify! {:url (str (:uri server) "/broken")} + body)))) (testing "timeout" - (is (= "failed: exception" (#'event-notification/notify! (str (:uri server) "/timeout") body)))) + (is (= "failed: exception" (#'event-notification/notify! {:url (str (:uri server) "/timeout") + :timeout 2} + body)))) (testing "invalid url" - (is (= "failed: exception" (#'event-notification/notify! "http://invalid/lol" body))))))) + (is (= "failed: exception" (#'event-notification/notify! {:url "http://invalid/lol"} + body))))))) (deftest test-event-notification ;; this is an integration test from commands to notifications From 157408576c8ec018d44b0b631fcbd2fcfe7411d4 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Thu, 9 Apr 2020 15:12:01 +0300 Subject: [PATCH 25/35] test: test that mimics an external id service --- src/clj/rems/event_notification.clj | 1 + test/clj/rems/api/test_over_http.clj | 49 ++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index f4ee6861b3..7653f36049 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -15,6 +15,7 @@ (def ^:private default-timeout 60) (defn- notify! [target body] + (log/info "Sending event notification to" (:url target)) (try (let [timeout-ms (* 1000 (get target :timeout default-timeout)) response (http/put (getx target :url) diff --git a/test/clj/rems/api/test_over_http.clj b/test/clj/rems/api/test_over_http.clj index caf40114c8..f2922b078d 100644 --- a/test/clj/rems/api/test_over_http.clj +++ b/test/clj/rems/api/test_over_http.clj @@ -4,9 +4,12 @@ [clojure.test :refer :all] [rems.api.testing :refer [standalone-fixture]] [rems.config] - [rems.db.test-data :as test-data])) + [rems.db.test-data :as test-data] + [rems.json :as json] + [rems.event-notification :as event-notification] + [stub-http.core :as stub])) -(use-fixtures :once standalone-fixture) +(use-fixtures :each standalone-fixture) (deftest test-api-sql-timeouts (let [api-key "42" @@ -16,7 +19,7 @@ save-draft! #(-> (http/post (str (:public-url rems.config/env) "/api/applications/save-draft") {:throw-exceptions false :as :json - :headers {"x-rems-api-key" "42" + :headers {"x-rems-api-key" api-key "x-rems-user-id" user-id} :content-type :json :form-params {:application-id application-id @@ -49,3 +52,43 @@ (reset! sleep-time nil) (is (= {:status 200 :body {:success true}} (save-draft!)))))))) + +(deftest test-allocate-external-id + ;; this test mimics an external id number service + (with-open [server (stub/start! {"/" (fn [r] + (let [event (json/parse-string (get-in r [:body "content"])) + app-id (:application/id event) + response (http/post (str (:public-url rems.config/env) "/api/applications/assign-external-id") + {:as :json + :headers {"x-rems-api-key" "42" + "x-rems-user-id" "developer"} + :content-type :json + :form-params {:application-id app-id + :external-id "new-id"}})] + (assert (get-in response [:body :success]))) + {:status 200})})] + (with-redefs [rems.config/env (assoc rems.config/env + :event-notification-targets [{:url (:uri server) + :event-types [:application.event/submitted]}])] + (let [api-key "42" + applicant "alice" + cat-id (test-data/create-catalogue-item! {}) + app-id (test-data/create-draft! applicant [cat-id] "value") + get-ext-id #(-> (http/get (str (:public-url rems.config/env) "/api/applications/" app-id) + {:as :json + :headers {"x-rems-api-key" api-key + "x-rems-user-id" applicant}}) + (get-in [:body :application/external-id]))] + (event-notification/process-outbox!) + (is (empty? (stub/recorded-requests server))) + (is (not= "new-id" (get-ext-id))) + (is (-> (http/post (str (:public-url rems.config/env) "/api/applications/submit") + {:as :json + :headers {"x-rems-api-key" api-key + "x-rems-user-id" applicant} + :content-type :json + :form-params {:application-id app-id}}) + (get-in [:body :success]))) + (event-notification/process-outbox!) + (is (= 1 (count (stub/recorded-responses server)))) + (is (= "new-id" (get-ext-id))))))) From 8ba4d1213073182d7b90e1655186cbca47319ed7 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Tue, 14 Apr 2020 09:47:55 +0300 Subject: [PATCH 26/35] doc: fix CHANGELOG.md --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e62baa9b6b..5a2549c1c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,12 +8,14 @@ have notable changes. Changes since v2.11 +### Additions +- Event notifications over HTTP. See [docs/event-notification.md](docs/event-notification.md) for details. (#2095) + ## v2.11 "Kotitontuntie" 2020-04-07 ### Additions - REMS sessions now stay alive while the user is active in the browser (#2107). - The `/api/users/active` API lists which users have active sessions at the moment. -- Event notifications over HTTP. See [docs/event-notification.md](docs/event-notification.md) for details. (#2095) ## v2.10 "Riihitontuntie" 2020-04-06 From 963a4bab865cce22a80bf9a48442ef7bc331ae53 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 08:43:37 +0300 Subject: [PATCH 27/35] fix: typo --- src/clj/rems/db/applications.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj index 4d23abc27d..4303a9e589 100644 --- a/src/clj/rems/db/applications.clj +++ b/src/clj/rems/db/applications.clj @@ -100,7 +100,7 @@ (model/build-application-view events fetcher-injections)))) (defn get-application - "Full application state with internal information hidden. No personalized for any users. Suitable for public APIs" + "Full application state with internal information hidden. Not personalized for any users. Suitable for public APIs." [application-id] (when-let [application (get-application-internal application-id)] (model/hide-non-public-information application))) From 0c22656238fae8ee212488a14478585e1000bdcd Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 08:45:39 +0300 Subject: [PATCH 28/35] doc: better docs for /api/applications/:application-id/raw --- src/clj/rems/api/applications.clj | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/clj/rems/api/applications.clj b/src/clj/rems/api/applications.clj index c523ff283c..cfe7955924 100644 --- a/src/clj/rems/api/applications.clj +++ b/src/clj/rems/api/applications.clj @@ -255,7 +255,9 @@ (api-util/not-found-json-response))) (GET "/:application-id/raw" [] - :summary "Get application by `application-id`. Application is not customized." + :summary (str "Get application by `application-id`. Unlike the /api/applicaitons/:application-id endpoint, " + "the data here isn't customized for the requesting user (see schema for details). Suitable " + "for integrations and exporting applications.") :roles #{:reporter :owner} :path-params [application-id :- (describe s/Int "application id")] :responses {200 {:schema ApplicationRaw} From 2b5d1cb1eb578836fad3ea7fb6558916e97d7fed Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 08:49:40 +0300 Subject: [PATCH 29/35] refactor: golf rems.event-notifications --- src/clj/rems/event_notification.clj | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index 7653f36049..caccf45db6 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -38,7 +38,7 @@ (if-let [error (notify! (get-in entry [:outbox/event-notification :target]) (get-in entry [:outbox/event-notification :body]))] (let [entry (outbox/attempt-failed! entry error)] - (when (not (:outbox/next-attempt entry)) + (when-not (:outbox/next-attempt entry) (log/warn "all attempts to send event notification id " (:outbox/id entry) "failed"))) (outbox/attempt-succeeded! (:outbox/id entry))))) @@ -61,14 +61,21 @@ (let [target {:url "whatever" :event-types [:application.event/submitted :application.event/approved]}] (is (false? (wants? target {:event/type :application.event/created}))) (is (true? (wants? target {:event/type :application.event/submitted}))) + (is (true? (wants? target {:event/type :application.event/approved})))) + (let [target {:url "whatever" :event-types []}] + (is (true? (wants? target {:event/type :application.event/created}))) + (is (true? (wants? target {:event/type :application.event/submitted}))) (is (true? (wants? target {:event/type :application.event/approved}))))) +(defn- notification-body [event] + (-> event + (assoc :event/application (applications/get-application (:application/id event))) + json/generate-string)) + (defn queue-notifications! [events] (when-let [targets (seq (get rems.config/env :event-notification-targets))] - (doseq [event events] - (let [application (applications/get-application (:application/id event)) - event-with-app (assoc event :event/application application) - body (json/generate-string event-with-app)] - (doseq [target targets] - (when (wants? target event) - (add-to-outbox! target body))))))) + (doseq [event events + :let [body (notification-body event)] + target targets + :when (wants? target event)] + (add-to-outbox! target body)))) From 68102a5bbc957b24041230dbb30ef248c525887d Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 08:54:23 +0300 Subject: [PATCH 30/35] test: minor improvements in resms.test-event-notification --- test/clj/rems/test_event_notification.clj | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/clj/rems/test_event_notification.clj b/test/clj/rems/test_event_notification.clj index 7c6b022be3..ecf190af28 100644 --- a/test/clj/rems/test_event_notification.clj +++ b/test/clj/rems/test_event_notification.clj @@ -37,7 +37,7 @@ body)))) (testing "timeout" (is (= "failed: exception" (#'event-notification/notify! {:url (str (:uri server) "/timeout") - :timeout 2} + :timeout 1} body)))) (testing "invalid url" (is (= "failed: exception" (#'event-notification/notify! {:url "http://invalid/lol"} @@ -81,8 +81,7 @@ :time (time/date-time 2001) :catalogue-item-ids [cat-id]}))] (testing "no notifications before outbox is processed" - (is (= [] - (stub/recorded-requests server)))) + (is (empty? (stub/recorded-requests server)))) (event-notification/process-outbox!) (testing "created event gets sent to both endpoints" (let [notifications (get-notifications)] From 6139666f3c44225d17ccf200f5d9115c35e4c066 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 08:58:45 +0300 Subject: [PATCH 31/35] refactor: create rems.db.outbox/get-due-entries --- src/clj/rems/db/entitlements.clj | 4 ++-- src/clj/rems/db/outbox.clj | 3 +++ src/clj/rems/email/core.clj | 2 +- src/clj/rems/event_notification.clj | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/clj/rems/db/entitlements.clj b/src/clj/rems/db/entitlements.clj index c29876e48c..d2770c9713 100644 --- a/src/clj/rems/db/entitlements.clj +++ b/src/clj/rems/db/entitlements.clj @@ -95,7 +95,7 @@ (defn process-outbox! [] (doseq [entry (mapv fix-entry-from-db - (outbox/get-entries {:type :entitlement-post :due-now? true}))] + (outbox/get-due-entries :entitlement-post))] ;; TODO could send multiple entitlements at once instead of one outbox entry at a time (if-let [error (post-entitlements! (:outbox/entitlement-post entry))] (let [entry (outbox/attempt-failed! entry error)] @@ -177,7 +177,7 @@ (when (seq members-to-update) (log/info "updating entitlements on application" application-id) (doseq [[userid resource-ids] entitlements-to-add] - (grant-entitlements! application-id userid resource-ids actor)) + (grant-entitlements! application-id userid resource-ids actor)) (doseq [[userid resource-ids] entitlements-to-remove] (revoke-entitlements! application-id userid resource-ids actor))))) diff --git a/src/clj/rems/db/outbox.clj b/src/clj/rems/db/outbox.clj index e01a51a5ea..3d026f8d6d 100644 --- a/src/clj/rems/db/outbox.clj +++ b/src/clj/rems/db/outbox.clj @@ -74,6 +74,9 @@ due-now? (filter (partial next-attempt-now? (DateTime/now))) ;; TODO move to db? type (filter #(= type (:outbox/type %)))))) +(defn get-due-entries [type] + (get-entries {:type type :due-now? true})) + (defn get-entry-by-id [id] (first (get-entries {:ids [id]}))) diff --git a/src/clj/rems/email/core.clj b/src/clj/rems/email/core.clj index 7cb974d838..24de45f10e 100644 --- a/src/clj/rems/email/core.clj +++ b/src/clj/rems/email/core.clj @@ -111,7 +111,7 @@ (str "failed sending email: " e))))))) (defn try-send-emails! [] - (doseq [email (outbox/get-entries {:type :email :due-now? true})] + (doseq [email (outbox/get-due-entries :email)] (if-let [error (send-email! (:outbox/email email))] (let [email (outbox/attempt-failed! email error)] (when (not (:outbox/next-attempt email)) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index caccf45db6..dfb3ed0e0f 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -34,7 +34,7 @@ "failed: exception"))) (defn process-outbox! [] - (doseq [entry (outbox/get-entries {:type :event-notification :due-now? true})] + (doseq [entry (outbox/get-due-entries :event-notification)] (if-let [error (notify! (get-in entry [:outbox/event-notification :target]) (get-in entry [:outbox/event-notification :body]))] (let [entry (outbox/attempt-failed! entry error)] From 425e04b4e2b9037fd78fe78c07019e8d34e2367b Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 09:04:30 +0300 Subject: [PATCH 32/35] doc: talk more about ordering, retried etc. in event-notification.md --- docs/event-notification.md | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/event-notification.md b/docs/event-notification.md index 6a47859d02..8ec97e7a25 100644 --- a/docs/event-notification.md +++ b/docs/event-notification.md @@ -2,6 +2,19 @@ REMS can be configured to send events notifications over HTTP. +Event notifications are performed _one at a time_, waiting for a HTTP +200 response from the notification endpoint. Everything except a HTTP +200 response counts as a failure. Failed notifications are retried +with exponential backoff for up to 12 hours. + +Due to retries, the event notification endpoint _should be_ idempotent. + +Event notifications are _not guaranteed to be ordered_ by event +creation order, especially when retries occur. + +Event notification failures are logged. You can also inspect the +`outbox` db table for the retry state of notifications. + ## Configuration See `:event-notification-targets` in [config-defaults.edn](../resources/config-defaults.edn). @@ -18,9 +31,3 @@ The body of the HTTP PUT request will be a JSON object that contains: Other keys may also be present depending on the event type. -## Error handling - -Event notifications are retried with exponential backoff for up to 12 -hours. Everything except a HTTP 200 status counts as a failure. -Failures are logged. You can also inspect the `outbox` db table for -the retry state of notifications. From 9604f5d8d5e30d3249e34b4054f07fb5aa9754cc Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 09:08:32 +0300 Subject: [PATCH 33/35] feat: log more details when sending event notifications --- src/clj/rems/event_notification.clj | 9 ++++----- test/clj/rems/test_event_notification.clj | 7 ++++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/clj/rems/event_notification.clj b/src/clj/rems/event_notification.clj index dfb3ed0e0f..f066c2a1f1 100644 --- a/src/clj/rems/event_notification.clj +++ b/src/clj/rems/event_notification.clj @@ -15,11 +15,12 @@ (def ^:private default-timeout 60) (defn- notify! [target body] - (log/info "Sending event notification to" (:url target)) + (log/info "Sending event notification for event" (select-keys body [:application/id :event/type :event/time]) + "to" (:url target)) (try (let [timeout-ms (* 1000 (get target :timeout default-timeout)) response (http/put (getx target :url) - {:body body + {:body (json/generate-string body) :throw-exceptions false :content-type :json :headers (get target :headers) @@ -68,9 +69,7 @@ (is (true? (wants? target {:event/type :application.event/approved}))))) (defn- notification-body [event] - (-> event - (assoc :event/application (applications/get-application (:application/id event))) - json/generate-string)) + (assoc event :event/application (applications/get-application (:application/id event)))) (defn queue-notifications! [events] (when-let [targets (seq (get rems.config/env :event-notification-targets))] diff --git a/test/clj/rems/test_event_notification.clj b/test/clj/rems/test_event_notification.clj index ecf190af28..2ebc210398 100644 --- a/test/clj/rems/test_event_notification.clj +++ b/test/clj/rems/test_event_notification.clj @@ -20,7 +20,7 @@ (with-open [server (stub/start! {"/ok" {:status 200} "/broken" {:status 500} "/timeout" {:status 200 :delay 5000}})] - (let [body "body"] + (let [body {:value 1}] (testing "success" (is (nil? (#'event-notification/notify! {:url (str (:uri server) "/ok") :headers {"additional-header" "value"}} @@ -29,9 +29,10 @@ (is (empty? more)) (is (= {:method "PUT" :path "/ok" - :body {"content" body}} + :body {"content" (json/generate-string body)}} (select-keys req [:method :path :body]))) - (is (= "value" (get-in req [:headers :additional-header]))))) + (is (= "value" (get-in req [:headers :additional-header]))) + )) (testing "error code" (is (= "failed: 500" (#'event-notification/notify! {:url (str (:uri server) "/broken")} body)))) From b7fca1a8807680e42a66f19557b69406d322f564 Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 10:14:19 +0300 Subject: [PATCH 34/35] feat: fix application raw api, gold standard test :field/private was missing --- src/clj/rems/db/applications.clj | 4 +- test/clj/rems/api/test_applications.clj | 111 ++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 9 deletions(-) diff --git a/src/clj/rems/db/applications.clj b/src/clj/rems/db/applications.clj index c76b4daa2d..f96374b8b2 100644 --- a/src/clj/rems/db/applications.clj +++ b/src/clj/rems/db/applications.clj @@ -104,7 +104,9 @@ "Full application state with internal information hidden. Not personalized for any users. Suitable for public APIs." [application-id] (when-let [application (get-application-internal application-id)] - (model/hide-non-public-information application))) + (-> application + (model/hide-non-public-information) + (model/apply-privacy #{:reporter})))) ;; to populate required :field/private attributes (defn get-application-for-user "Returns the part of application state which the specified user diff --git a/test/clj/rems/api/test_applications.clj b/test/clj/rems/api/test_applications.clj index 7fca60b099..e01cf33812 100644 --- a/test/clj/rems/api/test_applications.clj +++ b/test/clj/rems/api/test_applications.clj @@ -1,5 +1,6 @@ (ns ^:integration rems.api.test-applications - (:require [clojure.java.io :as io] + (:require [clj-time.core :as time] + [clojure.java.io :as io] [clojure.string :as str] [clojure.test :refer :all] [rems.api.services.catalogue :as catalogue] @@ -1338,17 +1339,111 @@ (deftest test-application-raw (let [api-key "42" applicant "alice" - handler "developer" + handler "handler" reporter "reporter" - app-id (test-data/create-application! {:actor applicant})] + form-id (test-data/create-form! {:form/title "notifications" + :form/fields [{:field/type :text + :field/id "field-1" + :field/title {:en "text field"} + :field/optional false}]}) + workflow-id (test-data/create-workflow! {:title "wf" + :handlers [handler] + :type :workflow/default}) + ext-id "resres" + res-id (test-data/create-resource! {:resource-ext-id ext-id}) + cat-id (test-data/create-catalogue-item! {:form-id form-id + :resource-id res-id + :workflow-id workflow-id}) + app-id (test-data/create-draft! applicant [cat-id] "raw test" (time/date-time 2010))] + (test-data/create-user! {:eppn applicant :mail "alice@example.com" :commonName "Alice Applicant"}) + (test-data/create-user! {:eppn handler :mail "handler@example.com" :commonName "Hannah Handler"}) + (test-data/create-user! {:eppn reporter :mail "reporter@example.com" :commonName "Robbie Reporter"}) (testing "applicant can't get raw application" (is (response-is-forbidden? (api-response :get (str "/api/applications/" app-id "/raw") nil api-key applicant)))) (testing "reporter can get raw application" - (is (= {:application/id app-id - :application/user-roles {(keyword applicant) ["applicant"] - (keyword handler) ["handler"] - (keyword reporter) ["reporter"]}} + (is (= {:application/description "" + :application/invited-members [] + :application/last-activity "2010-01-01T00:00:00.000Z" + :application/attachments [] + :application/licenses [] + :application/created "2010-01-01T00:00:00.000Z" + :application/state "application.state/draft" + :application/role-permissions + {:everyone-else ["application.command/accept-invitation"] + :member ["application.command/copy-as-new" + "application.command/accept-licenses"] + :reporter ["see-everything"] + :applicant ["application.command/copy-as-new" + "application.command/invite-member" + "application.command/submit" + "application.command/remove-member" + "application.command/accept-licenses" + "application.command/uninvite-member" + "application.command/save-draft" + "application.command/close" + "application.command/change-resources"]} + :application/modified "2010-01-01T00:00:00.000Z" + :application/user-roles {:alice ["applicant"] :handler ["handler"] :reporter ["reporter"]} + :application/external-id "2010/1" + :application/workflow {:workflow/type "workflow/default" + :workflow/id workflow-id + :workflow.dynamic/handlers + [{:email "handler@example.com" :userid "handler" :name "Hannah Handler"}]} + :application/blacklist [] + :application/id app-id + :application/todo nil + :application/applicant {:email "alice@example.com" :userid "alice" :name "Alice Applicant"} + :application/members [] + :application/resources [{:catalogue-item/start "REDACTED" + :catalogue-item/end nil + :catalogue-item/expired false + :catalogue-item/enabled true + :resource/id res-id + :catalogue-item/title {} + :catalogue-item/infourl {} + :resource/ext-id ext-id + :catalogue-item/archived false + :catalogue-item/id cat-id}] + :application/accepted-licenses {:alice []} + :application/forms [{:form/fields [{:field/value "raw test" + :field/type "text" + :field/title {:en "text field"} + :field/id "field-1" + :field/optional false + :field/visible true + :field/private false}] + :form/title "notifications" + :form/id form-id}] + :application/events [{:application/external-id "2010/1" + :event/actor-attributes {:userid "alice" :name "Alice Applicant" :email "alice@example.com"} + :application/id app-id + :event/time "2010-01-01T00:00:00.000Z" + :workflow/type "workflow/default" + :application/resources [{:catalogue-item/id cat-id :resource/ext-id ext-id}] + :application/forms [{:form/id form-id}] + :workflow/id workflow-id + :event/actor "alice" + :event/type "application.event/created" + :event/id 100 + :application/licenses []} + {:event/id 100 + :event/type "application.event/draft-saved" + :event/time "2010-01-01T00:00:00.000Z" + :event/actor "alice" + :application/id app-id + :event/actor-attributes {:userid "alice" :name "Alice Applicant" :email "alice@example.com"} + :application/field-values [{:form form-id :field "field-1" :value "raw test"}]} + {:event/id 100 + :event/type "application.event/licenses-accepted" + :event/time "2010-01-01T00:00:00.000Z" + :event/actor "alice" + :application/id app-id + :event/actor-attributes {:userid "alice" :name "Alice Applicant" :email "alice@example.com"} + :application/accepted-licenses []}]} (-> (api-call :get (str "/api/applications/" app-id "/raw") nil api-key reporter) - (select-keys [:application/id :application/user-roles]))))))) + ;; start is set by the db not easy to mock + (assoc-in [:application/resources 0 :catalogue-item/start] "REDACTED") + ;; event ids are unpredictable + (update :application/events (partial map #(update % :event/id (constantly 100)))))))))) From c5df8dd353dc99db3e9a79117fc16d3528dfbe3f Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Mon, 20 Apr 2020 10:18:19 +0300 Subject: [PATCH 35/35] test: test event notification body against application raw api --- test/clj/rems/test_event_notification.clj | 67 +++-------------------- 1 file changed, 7 insertions(+), 60 deletions(-) diff --git a/test/clj/rems/test_event_notification.clj b/test/clj/rems/test_event_notification.clj index 2ebc210398..93a6149ef3 100644 --- a/test/clj/rems/test_event_notification.clj +++ b/test/clj/rems/test_event_notification.clj @@ -4,17 +4,15 @@ [medley.core :refer [dissoc-in]] [rems.config] [rems.api.services.command :as command] + [rems.api.testing :refer [api-fixture api-call]] [rems.db.test-data :as test-data] - [rems.db.testing :refer [test-db-fixture rollback-db-fixture]] [rems.event-notification :as event-notification] [rems.json :as json] - [rems.testing-util :refer [fixed-time-fixture with-user]] [stub-http.core :as stub])) (use-fixtures :once - test-db-fixture - rollback-db-fixture) + api-fixture) (deftest test-notify! (with-open [server (stub/start! {"/ok" {:status 200} @@ -58,10 +56,7 @@ :data (-> r :body (get "content") - json/parse-string - (dissoc-in [:event/application :application/events]) - ;; catalogue-item/start is set by the db and can't be easily fixed - (dissoc-in [:event/application :application/resources 0 :catalogue-item/start]))})) + json/parse-string)})) form-id (test-data/create-form! {:form/title "notifications" :form/fields [{:field/type :text :field/id "field-1" @@ -85,7 +80,9 @@ (is (empty? (stub/recorded-requests server)))) (event-notification/process-outbox!) (testing "created event gets sent to both endpoints" - (let [notifications (get-notifications)] + (let [notifications (get-notifications) + app-from-raw-api (api-call :get (str "/api/applications/" app-id "/raw") nil + "42" "reporter")] (is (= 2 (count notifications))) (is (= #{"/created" "/all"} (set (map :path notifications)))) @@ -100,57 +97,7 @@ :event/actor applicant :event/type "application.event/created" :application/licenses [] - :event/application {:application/description "" - :application/invited-members [] - :application/last-activity "2001-01-01T00:00:00.000Z" - :application/attachments [] - :application/licenses [] - :application/created "2001-01-01T00:00:00.000Z" - :application/state "application.state/draft" - :application/role-permissions - {:everyone-else ["application.command/accept-invitation"] - :member ["application.command/copy-as-new" - "application.command/accept-licenses"] - :reporter ["see-everything"] - :applicant ["application.command/copy-as-new" - "application.command/invite-member" - "application.command/submit" - "application.command/remove-member" - "application.command/accept-licenses" - "application.command/uninvite-member" - "application.command/save-draft" - "application.command/close" - "application.command/change-resources"]} - :application/modified "2001-01-01T00:00:00.000Z" - :application/user-roles {:alice ["applicant"] :handler ["handler"]} - :application/external-id "2001/1" - :application/workflow {:workflow/type "workflow/default" - :workflow/id workflow-id - :workflow.dynamic/handlers - [{:email nil :userid "handler" :name nil}]} - :application/blacklist [] - :application/id app-id - :application/todo nil - :application/applicant {:email nil :userid "alice" :name nil} - :application/members [] - :application/resources [{:catalogue-item/end nil - :catalogue-item/expired false - :catalogue-item/enabled true - :resource/id res-id - :catalogue-item/title {} - :catalogue-item/infourl {} - :resource/ext-id ext-id - :catalogue-item/archived false - :catalogue-item/id cat-id}] - :application/accepted-licenses {} - :application/forms [{:form/fields [{:field/value "" - :field/type "text" - :field/title {:en "text field"} - :field/id "field-1" - :field/optional false - :field/visible true}] - :form/title "notifications" - :form/id form-id}]}} + :event/application app-from-raw-api} (:data (first notifications)))) (is (= (:data (first notifications)) (:data (second notifications))))))