From aa04e68545a0f8eaa4d10b3ed337320884363f4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arne=20Schl=C3=BCter?= Date: Mon, 11 Jun 2018 19:29:49 +0200 Subject: [PATCH] Display api errors as notifications --- src/cljs/airsonic_ui/events.cljs | 54 +++++++++++++------ src/cljs/airsonic_ui/routes.cljs | 8 +-- src/cljs/airsonic_ui/utils/api.cljs | 23 ++++++-- src/cljs/airsonic_ui/views.cljs | 26 +++------ src/cljs/airsonic_ui/views/notifications.cljs | 14 +++++ src/sass/app.sass | 3 +- test/cljs/airsonic_ui/events_test.cljs | 16 +++++- test/cljs/airsonic_ui/fixtures.cljs | 14 +++++ test/cljs/airsonic_ui/utils/api_test.cljs | 21 +++----- 9 files changed, 118 insertions(+), 61 deletions(-) create mode 100644 src/cljs/airsonic_ui/views/notifications.cljs create mode 100644 test/cljs/airsonic_ui/fixtures.cljs diff --git a/src/cljs/airsonic_ui/events.cljs b/src/cljs/airsonic_ui/events.cljs index c067841..37b089e 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,28 +105,32 @@ (let [creds (:credentials db)] (api/url (:server creds) endpoint (merge params (select-keys creds [:u :p]))))) -(defn api-request [{:keys [db]} [_ endpoint k params]] +(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-success k] - :on-failure [::api-failure]}}) + :on-success [:api/good-response] + :on-failure [:api/bad-response]}}) (re-frame/reg-event-fx - :api-request + :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 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/utils/api.cljs b/src/cljs/airsonic_ui/utils/api.cljs index a182123..ee8ce25 100644 --- a/src/cljs/airsonic_ui/utils/api.cljs +++ b/src/cljs/airsonic_ui/utils/api.cljs @@ -26,14 +26,27 @@ (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 (ex-info (:message response) error))) - (-> (get-in response [:subsonic-response]) - (dissoc :status :version) - vals - first))) + (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 3005461..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]] @@ -46,26 +47,12 @@ {:on-click #(dispatch [::events/initialize-db]) :href "#"} (str "Logout (" (:name user) ")")]]]]) -;; 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)]))]) - ;; putting everything together (defn app [route params query] (let [user @(subscribe [::subs/user]) - notifications @(subscribe [::subs/notifications]) content @(subscribe [::subs/current-content])] [:div - [notification-list notifications] [:main.columns [:div.column.is-2.sidebar [sidebar user]] @@ -79,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 aa522c2..4e18acb 100644 --- a/src/sass/app.sass +++ b/src/sass/app.sass @@ -77,7 +77,8 @@ // floating notifications .notifications @extend .container - padding-top: 3.2rem + 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 6c6580b..9b0bfab 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"]) @@ -47,6 +54,11 @@ (defn- first-notification [db] (-> (:notifications db) 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"]))))) 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 9d579ad..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 @@ -9,17 +10,7 @@ (api/url server endpoint {})) (def fixtures - {:default-url (url "http://localhost:8080" "ping") - :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"}}}) + {:default-url (url "http://localhost:8080" "ping")}) (deftest general-url-construction (testing "Handles missing slashes" @@ -43,14 +34,14 @@ (deftest response-handling (testing "Should unwrap responses" - (let [response (get-in fixtures [:responses :ok])] + (let [response (:ok responses)] (is (= (get-in response [:subsonic-response :scanStatus]) (api/unwrap-response response))))) (testing "Should detect errors" - (is (true? (api/is-error? (get-in fixtures [:responses :error])))) - (is (false? (api/is-error? (get-in fixtures [:responses :ok]))))) + (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 (get-in fixtures [:responses :error])] + (let [error-response (:error responses)] (is (thrown? ExceptionInfo (api/unwrap-response error-response))) (try (api/unwrap-response error-response)