From ab7519f289bb9cfe7c29f16f23b9ae7b4756d458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arne=20Schl=C3=BCter?= Date: Mon, 11 Jun 2018 19:58:13 +0200 Subject: [PATCH] Add user notifications and display api errors (#10) Closes #2 * Add user notifications * Update re-frame-10x config * Display api errors as notifications * Automatically hide notifications after a while --- shadow-cljs.edn | 5 +- src/cljs/airsonic_ui/db.cljs | 3 +- src/cljs/airsonic_ui/events.cljs | 97 +++++++++++++++---- src/cljs/airsonic_ui/routes.cljs | 8 +- src/cljs/airsonic_ui/subs.cljs | 25 +++-- src/cljs/airsonic_ui/utils/api.cljs | 28 ++++++ src/cljs/airsonic_ui/views.cljs | 12 ++- src/cljs/airsonic_ui/views/notifications.cljs | 14 +++ src/sass/app.sass | 9 ++ test/cljs/airsonic_ui/events_test.cljs | 42 +++++++- test/cljs/airsonic_ui/fixtures.cljs | 14 +++ test/cljs/airsonic_ui/utils/api_test.cljs | 17 ++++ 12 files changed, 232 insertions(+), 42 deletions(-) create mode 100644 src/cljs/airsonic_ui/views/notifications.cljs create mode 100644 test/cljs/airsonic_ui/fixtures.cljs diff --git a/shadow-cljs.edn b/shadow-cljs.edn index ace816b..b145502 100644 --- a/shadow-cljs.edn +++ b/shadow-cljs.edn @@ -14,12 +14,13 @@ ;; for CIDER [cider/cider-nrepl "0.18.0-SNAPSHOT"]] + :nrepl {:port 9000} + :builds {:app {:target :browser :output-dir "public/app/js" :asset-path "/app/js" - :closure-defines {"re_frame.trace.trace_enabled_QMARK_" true - "day8.re_frame.tracing.trace_enabled_QMARK_" true} + :closure-defines {"re_frame.trace.trace_enabled_QMARK_" true} :modules {:main {:entries [airsonic-ui.core]}} :devtools {:http-root "public" :http-port 8080 diff --git a/src/cljs/airsonic_ui/db.cljs b/src/cljs/airsonic_ui/db.cljs index 7f3b701..b8cbcac 100644 --- a/src/cljs/airsonic_ui/db.cljs +++ b/src/cljs/airsonic_ui/db.cljs @@ -3,4 +3,5 @@ (def default-db {;; because navigate! executes asynchronously we force to display the login screen first - :current-route [routes/default-route]}) + :current-route [routes/default-route] + :notifications (sorted-map)}) diff --git a/src/cljs/airsonic_ui/events.cljs b/src/cljs/airsonic_ui/events.cljs index 6b82793..791439a 100644 --- a/src/cljs/airsonic_ui/events.cljs +++ b/src/cljs/airsonic_ui/events.cljs @@ -11,6 +11,12 @@ ;; ::events/something-happening -> relevant to only this app ;; :single-colon/something -> coming from external sources (e.g. :audio/... or :routes/...) that are potentially reusable +(re-frame/reg-fx + ;; a simple effect to keep println statements out of our event handlers + :log + (fn [params] + (apply println params))) + ;; database reset / init (re-frame/reg-event-db @@ -29,12 +35,24 @@ :http-xhrio {:method :get :uri (api/url server "ping" {:u user :p pass}) :response-format (ajax/json-response-format {:keywords? true}) - :on-success [::credentials-verified user pass] - :on-failure [::api-failure]}}) + :on-success [::verify-auth-response user pass] + :on-failure [:api/bad-response]}}) (re-frame/reg-event-fx ::authenticate authenticate) +(defn verify-auth-response + "Since we don't get real status codes, we have to look into the server's + response and see whether we actually sent the correct credentials" + [fx [_ user pass response]] + {:dispatch (if (api/is-error? response) + [:notification/show :error (api/error-msg (api/->exception response))] + [::credentials-verified user pass])}) + +(re-frame/reg-event-fx + ::verify-auth-response + verify-auth-response) + (defn try-remember-user "Enables skipping the auth request when credentials are saved in the local storage; otherwise has no effect" @@ -51,7 +69,7 @@ (defn credentials-verified "Gets called after the server indicates that the credentials entered by a user are correct (see `authenticate`)" - [{:keys [db store]} [_event user pass _response]] + [{:keys [db store]} [_ user pass]] (let [auth {:u user :p pass} credentials (merge (:credentials db) auth)] {:routes/set-credentials auth @@ -87,26 +105,32 @@ (let [creds (:credentials db)] (api/url (:server creds) endpoint (merge params (select-keys creds [:u :p]))))) +(defn api-request [{:keys [db]} [_ endpoint params]] + {:http-xhrio {:method :get + :uri (api-url db endpoint params) + :response-format (ajax/json-response-format {:keywords? true}) + :on-success [:api/good-response] + :on-failure [:api/bad-response]}}) + (re-frame/reg-event-fx - :api-request - (fn [{:keys [db]} [_ endpoint k params]] - {:http-xhrio {:method :get - :uri (api-url db endpoint params) - :response-format (ajax/json-response-format {:keywords? true}) - :on-success [::api-success k] - :on-failure [::api-failure]}})) + :api/request + api-request) -(re-frame/reg-event-db - ::api-success - (fn [db [_ k response]] - ; we "unwrap" the responses - (assoc db :response (-> response :subsonic-response k)))) +(defn good-api-response [fx [_ response]] + (try + (assoc-in fx [:db :response] (api/unwrap-response response)) + (catch ExceptionInfo e + {:dispatch [:notification/show :error (api/error-msg e)]}))) -(re-frame/reg-event-db - ::api-failure +(re-frame/reg-event-fx + :api/good-response + good-api-response) + +(re-frame/reg-event-fx + :api/bad-response (fn [db event] - (println "api call gone bad; CORS headers missing? check for :status 0" event) - db)) + {:log ["API call gone bad; are CORS headers missing? check for :status 0" event] + :dispatch [:notification/show :error "Communication with server failed. Check browser logs for details."]})) ;; musique @@ -173,3 +197,38 @@ {:routes/navigate [routes/default-route] :routes/unset-credentials nil :db db/default-db})) + +;; user messages + +(def notification-duration + {:info 2500 + :error 10000}) + +(defn show-notification + "Displays an informative message to the user" + [fx [_ level message]] + (let [id (.now js/performance) + hide-later (fn [level] + [{:ms (get notification-duration level) + :dispatch [:notification/hide id]}])] + (if (nil? message) + (let [message level + level :info] + (-> (assoc-in fx [:db :notifications id] {:level level + :message message}) + (assoc :dispatch-later (hide-later level)))) + (-> (assoc-in fx [:db :notifications id] {:level level + :message message}) + (assoc :dispatch-later (hide-later level)))))) + +(re-frame/reg-event-fx + :notification/show + show-notification) + +(defn hide-notification + [db [_ notification-id]] + (update db :notifications dissoc notification-id)) + +(re-frame/reg-event-db + :notification/hide + hide-notification) diff --git a/src/cljs/airsonic_ui/routes.cljs b/src/cljs/airsonic_ui/routes.cljs index 60a9a4c..92fa23f 100644 --- a/src/cljs/airsonic_ui/routes.cljs +++ b/src/cljs/airsonic_ui/routes.cljs @@ -28,16 +28,16 @@ (defmethod route-data ::main [route-id params query] - [:api-request "getAlbumList2" :albumList2 {:type "recent" - :size 18}]) + [:api/request "getAlbumList2" {:type "recent" + :size 18}]) (defmethod route-data ::artist-view [route-id params query] - [:api-request "getArtist" :artist (select-keys params [:id])]) + [:api/request "getArtist" (select-keys params [:id])]) (defmethod route-data ::album-view [route-id params query] - [:api-request "getAlbum" :album (select-keys params [:id])]) + [:api/request "getAlbum" (select-keys params [:id])]) ;; shouldn't need to change anything below diff --git a/src/cljs/airsonic_ui/subs.cljs b/src/cljs/airsonic_ui/subs.cljs index 5cfb1a2..36048a2 100644 --- a/src/cljs/airsonic_ui/subs.cljs +++ b/src/cljs/airsonic_ui/subs.cljs @@ -6,40 +6,38 @@ ;; FIXME: this is used for cover images and it's quite ugly tbh (re-frame/reg-sub ::login - (fn [db] + (fn [db _] (select-keys (:credentials db) [:u :p]))) (re-frame/reg-sub ::user - (fn [{:keys [credentials]}] + (fn [{:keys [credentials]} [_]] {:name (:u credentials)})) (re-frame/reg-sub ::server - (fn [db] + (fn [db _] (get-in db [:credentials :server]))) ;; current hashbang (re-frame/reg-sub ::current-route - (fn [db] + (fn [db _] (:current-route db))) -;; --- - ;; TODO: Make this nice and clean (re-frame/reg-sub ::current-content - (fn [db] - (db :response))) + (fn [db _] + (:response db))) (re-frame/reg-sub ; returns info on the current song as is (basically the metadata you can read from the file system) ::currently-playing - (fn [db] - (db :currently-playing))) + (fn [db _] + (:currently-playing db))) (re-frame/reg-sub ::is-playing? @@ -49,3 +47,10 @@ (let [status (:status currently-playing)] (and (not (:paused? status)) (not (:ended? status)))))) + +;; user notifications + +(re-frame/reg-sub + ::notifications + (fn [db _] + (:notifications db))) diff --git a/src/cljs/airsonic_ui/utils/api.cljs b/src/cljs/airsonic_ui/utils/api.cljs index 09aeecf..ee8ce25 100644 --- a/src/cljs/airsonic_ui/utils/api.cljs +++ b/src/cljs/airsonic_ui/utils/api.cljs @@ -22,3 +22,31 @@ (defn cover-url [server credentials item size] (url server "getCoverArt" (merge {:id (:coverArt item) :size size} credentials))) + +(defn is-error? [response] + (= "failed" (get-in response [:subsonic-response :status]))) + +(defn- unwrap-response* [response] + (-> (:subsonic-response response) + (dissoc :status :version) + vals + first)) + +(defn ->exception + "Takes an erroneous response and makes it a real exception" + [response] + (let [error (unwrap-response* response)] + (ex-info (:message response) error))) + +(defn unwrap-response + "Retrieves the actual response body" + [response] + (if (is-error? response) + (let [error (:error response)] + (throw (->exception response))) + (unwrap-response* response))) + +(defn error-msg + [exception-info] + (let [{:keys [code message]} (ex-data exception-info)] + (str "Error " code ": " message))) diff --git a/src/cljs/airsonic_ui/views.cljs b/src/cljs/airsonic_ui/views.cljs index 1aa7d6a..ef0df93 100644 --- a/src/cljs/airsonic_ui/views.cljs +++ b/src/cljs/airsonic_ui/views.cljs @@ -5,6 +5,7 @@ [airsonic-ui.events :as events] [airsonic-ui.subs :as subs] + [airsonic-ui.views.notifications :refer [notification-list]] [airsonic-ui.views.breadcrumbs :refer [breadcrumbs]] [airsonic-ui.views.bottom-bar :refer [bottom-bar]] [airsonic-ui.views.login :refer [login-form]] @@ -65,7 +66,10 @@ [bottom-bar]])) (defn main-panel [] - (let [[route params query] @(subscribe [::subs/current-route])] - (case route - ::routes/login [login-form] - [app route params query]))) + (let [[route params query] @(subscribe [::subs/current-route]) + notifications @(subscribe [::subs/notifications])] + [:div + [notification-list notifications] + (case route + ::routes/login [login-form] + [app route params query])])) diff --git a/src/cljs/airsonic_ui/views/notifications.cljs b/src/cljs/airsonic_ui/views/notifications.cljs new file mode 100644 index 0000000..2e2210f --- /dev/null +++ b/src/cljs/airsonic_ui/views/notifications.cljs @@ -0,0 +1,14 @@ +(ns airsonic-ui.views.notifications + (:require [re-frame.core :refer [dispatch]])) + +;; user notifications + +(defn notification-list [notifications] + [:div.notifications + (for [[id notification] notifications] + (let [class (case (:level notification) + :error "danger" + "info")] + ^{:key id} [:div {:class-name (str "notification is-small is-" class)} + [:button.delete {:on-click #(dispatch [:notification/hide id])}] + (:message notification)]))]) diff --git a/src/sass/app.sass b/src/sass/app.sass index ff7972c..4e18acb 100644 --- a/src/sass/app.sass +++ b/src/sass/app.sass @@ -73,3 +73,12 @@ .table .grow width: 100% + +// floating notifications +.notifications + @extend .container + z-index: 100 + position: fixed + left: 0 + right: 0 + padding-top: 3.2rem diff --git a/test/cljs/airsonic_ui/events_test.cljs b/test/cljs/airsonic_ui/events_test.cljs index 0d88f42..adf1f4d 100644 --- a/test/cljs/airsonic_ui/events_test.cljs +++ b/test/cljs/airsonic_ui/events_test.cljs @@ -1,6 +1,7 @@ (ns airsonic-ui.events-test (:require [cljs.test :refer [deftest testing is]] [clojure.string :as str] + [airsonic-ui.fixtures :refer [responses]] [airsonic-ui.events :as events])) (enable-console-print!) @@ -16,8 +17,14 @@ (testing "saves the given server location" (is (= server (get-in fx [:db :credentials :server])))) (testing "invokes correct success callback" - (is (= ::events/credentials-verified (first (:on-success request))))))) - (testing "On succesfull response" + (is (= ::events/verify-auth-response (first (:on-success request))))))) + (testing "Auth response verification" + (is (= :notification/show + (first (:dispatch (events/verify-auth-response {} [:_ "user" "pass" (:error responses)])))) + "shows an error when we have an error response") + (let [event (:dispatch (events/verify-auth-response {} [:_ "username" "password" (:auth-success responses)]))] + (is (= [::events/credentials-verified "username" "password"] event)))) + (testing "On succesful response" (let [creds-before {:server "https://localhost"} fx (events/credentials-verified {:db {:credentials creds-before}} [:_ "user" "pass"]) @@ -43,3 +50,34 @@ (testing "When there's no previous login data" (testing "remembering has no effect" (is (nil? (events/try-remember-user {} [:_])))))) + +(defn- first-notification [fx] + (-> (get-in fx [:db :notifications]) vals first)) + +(deftest api-interaction + (testing "Should show an error notification when airsonic responds with an error" + (let [fx (events/good-api-response {} [:_ (:error responses)])] + (is (= :error (-> fx :dispatch second)))))) + +(deftest user-notifications + (testing "Should be able to display a message with an assigned level" + (is (= :error (:level (first-notification (events/show-notification {} [:_ :error "foo"]))))) + (is (= :info (:level (first-notification (events/show-notification {} [:_ :info "some other message"])))))) + (testing "Should default to level :info" + (is (= :info (:level (first-notification (events/show-notification {} [:_ "and another one"])))))) + (testing "Should create a unique id for each message" + (let [state (-> + {} + (events/show-notification [:_ :info "Something something"]) + (events/show-notification [:_ :error "Something important"])) + ids (keys (:notifications state))] + (is (= (count ids) (count (set ids)))))) + (testing "Should remove a message, given it's id" + (let [state (events/show-notification {} [:_ "This is a notification"]) + id (-> (:notifications state) + keys + first)] + (is (empty? (:notifications (events/hide-notification state [:_ id])))))) + (testing "Should automatically remove a message after a while" + (let [fx (events/show-notification {} [:_ :info "This is a notification"])] + (is (= :notification/hide (-> (:dispatch-later fx) first :dispatch first)))))) diff --git a/test/cljs/airsonic_ui/fixtures.cljs b/test/cljs/airsonic_ui/fixtures.cljs new file mode 100644 index 0000000..d9d500f --- /dev/null +++ b/test/cljs/airsonic_ui/fixtures.cljs @@ -0,0 +1,14 @@ +(ns airsonic-ui.fixtures) + +(def responses {:error {:subsonic-response + {:error {:code 40 + :message "Wrong username or password"} + :status "failed" + :version "1.15.0"}} + :ok {:subsonic-response + {:scanStatus {:count 10326 + :scanning false} + :status "ok" + :version "1.15.0"}} + :auth-success {:subsonic-response {:status "ok" + :version "1.15.0"}}}) diff --git a/test/cljs/airsonic_ui/utils/api_test.cljs b/test/cljs/airsonic_ui/utils/api_test.cljs index 7e12ee0..3513b17 100644 --- a/test/cljs/airsonic_ui/utils/api_test.cljs +++ b/test/cljs/airsonic_ui/utils/api_test.cljs @@ -1,6 +1,7 @@ (ns airsonic-ui.utils.api-test (:require [cljs.test :refer [deftest testing is]] [clojure.string :as str] + [airsonic-ui.fixtures :refer [responses]] [airsonic-ui.utils.api :as api])) (defn- url @@ -30,3 +31,19 @@ (is (true? (str/includes? (api/cover-url "http://server.tld" {} album -1) (str "id=" (:coverArt album)))))) (testing "Should scale an image to a given size" (is (true? (str/includes? (api/cover-url "http://server.tld" {} album 48) "size=48")))))) + +(deftest response-handling + (testing "Should unwrap responses" + (let [response (:ok responses)] + (is (= (get-in response [:subsonic-response :scanStatus]) + (api/unwrap-response response))))) + (testing "Should detect errors" + (is (true? (api/is-error? (:error responses)))) + (is (false? (api/is-error? (:ok responses))))) + (testing "Should throw an informative error when trying to unwrap an erroneous response" + (let [error-response (:error responses)] + (is (thrown? ExceptionInfo (api/unwrap-response error-response))) + (try + (api/unwrap-response error-response) + (catch ExceptionInfo e + (= (:error error-response) (ex-data e)))))))