Make login-flow more robust and add more tests

src/cljs/airsonic_ui/db.cljs

@@ -1,5 +1,4 @@
-(ns airsonic-ui.db
+(ns airsonic-ui.db)
-  (:require [airsonic-ui.routes :as routes]))
 
 (def default-db
   {:notifications (sorted-map)})

src/cljs/airsonic_ui/events.cljs

@@ -12,6 +12,11 @@
  (fn [params]
    (apply println params)))
 
+(defn noop
+  "An event handler that can be used for clarity; doesn't do anything, but might
+  give a name to an event"
+  [cofx _] cofx)
+
 ;; ---
 ;; app boot flow
 ;; * restoring a previous session
@@ -31,10 +36,9 @@
   troubles with our router."
   [{:keys [db store]} _]
   (let [credentials (:credentials store)]
-    {:db (assoc db :credentials credentials)
+    {:dispatch-n [(if credentials
-     :dispatch-n [(if credentials
                     [:init-flow/credentials-found credentials]
-                    [:init-flow/credentials-missing])]
+                    [:init-flow/credentials-not-found])]
      :routes/start-routing nil}))
 
 (re-frame/reg-event-fx
@@ -45,19 +49,21 @@
 (defn credentials-found [_ [_ {:keys [u p server]}]]
   {:dispatch [:credentials/verification-request u p server]})
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx :init-flow/credentials-found credentials-found)
- :init-flow/credentials-found credentials-found)
 
-(re-frame/reg-event-fx
- :init-flow/credentials-missing
 ;; we don't do anything special here, it's just for the sake of clarity
- (fn [_ _] {}))
+
+(defn credentials-not-found
+  [cofx _]
+  (assoc-in cofx [:db :credentials] :credentials/not-found))
+
+(re-frame/reg-event-fx :init-flow/credentials-not-found credentials-not-found)
 
 ;; ---
 ;; auth logic
 ;; ---
 
-(defn-traced credentials-verification-request
+(defn credentials-verification-request
   "Tries to authenticate a user by pinging the server with credentials, saving
   them when the request was successful. Bypasses the request when a user saved
   their credentials."
@@ -66,21 +72,25 @@
                 :uri (api/url server "ping" {:u user :p pass})
                 :response-format (ajax/json-response-format {:keywords? true})
                 :on-success [:credentials/verification-response user pass server]
-                :on-failure [:api/bad-response]}})
+                :on-failure [:credentials/verification-failure]}})
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx :credentials/verification-request credentials-verification-request)
- :credentials/verification-request credentials-verification-request)
 
 (defn credentials-verification-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 server response]]
   {:dispatch (if (api/is-error? response)
-               [:notification/show :error (api/error-msg (api/->exception response))]
+               [:credentials/verification-failure response]
                [:credentials/verified user pass server])})
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx :credentials/verification-response credentials-verification-response)
- :credentials/verification-response credentials-verification-response)
+
+(defn credentials-verification-failure [fx [_ response]]
+  (-> (assoc-in fx [:db :credentials] :credentials/verification-failure)
+      (assoc :dispatch [:notification/show :error (api/error-msg (api/->exception response))])))
+
+(re-frame/reg-event-fx :credentials/verification-failure credentials-verification-failure)
 
 (defn credentials-verified
   "Gets called after the server indicates that the credentials entered by a user
@@ -92,8 +102,7 @@
      :db (assoc db :credentials credentials)
      :dispatch [::logged-in]}))
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx :credentials/verified credentials-verified)
- :credentials/verified credentials-verified)
 
 ;; TODO: We have to find another solution for this once we have routes that
 ;; don't require a login but have the bottom controls
@@ -103,10 +112,10 @@
  (fn [_]
    (.. js/document -documentElement -classList (add "has-navbar-fixed-bottom"))))
 
+
 (defn logged-in
   [cofx _]
-  (let [redirect (or (get-in cofx [:routes/from-query-param :redirect])
+  (let [redirect (or (get-in cofx [:routes/from-query-param :redirect]) [::routes/main])]
-                     [::routes/main])]
     {:routes/navigate redirect
      :show-nav-bar nil}))
 
@@ -117,17 +126,16 @@
 
 (defn logout
   "Clears all credentials and redirects the user to the login page"
-  [_ [_ & args]]
+  [cofx [_ & args]]
   (let [args (apply hash-map args)]
     {:routes/navigate (if-let [redirect (:redirect-to args)]
                         [::routes/login {} {:redirect (routes/encode-route redirect)}]
                         [::routes/login])
      :routes/unset-credentials nil
      :store nil
-     :db db/default-db}))
+     :db (merge (:db cofx) db/default-db {:credentials :credentials/logged-out})}))
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx ::logout logout)
- ::logout logout)
 
 ;; ---
 ;; api interaction
@@ -144,8 +152,7 @@
                 :on-success [:api/good-response]
                 :on-failure [:api/bad-response]}})
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx :api/request api-request)
- :api/request api-request)
 
 (defn good-api-response [fx [_ response]]
   (try
@@ -153,15 +160,13 @@
     (catch ExceptionInfo e
       {:dispatch [:notification/show :error (api/error-msg e)]})))
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx :api/good-response good-api-response)
- :api/good-response good-api-response)
 
 (defn bad-api-response [db event]
   {: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."]})
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx :api/bad-response bad-api-response)
- :api/bad-response bad-api-response)
 
 ;; ---
 ;; musique
@@ -256,12 +261,10 @@
                                                 :message message})
           (assoc :dispatch-later (hide-later level))))))
 
-(re-frame/reg-event-fx
+(re-frame/reg-event-fx :notification/show show-notification)
- :notification/show show-notification)
 
 (defn hide-notification
   [db [_ notification-id]]
   (update db :notifications dissoc notification-id))
 
-(re-frame/reg-event-db
+(re-frame/reg-event-db :notification/hide hide-notification)
- :notification/hide hide-notification)

src/cljs/airsonic_ui/routes.cljs

@@ -100,7 +100,7 @@
    ;; this allows us to encode a complete route in a url fragment; useful for
    ;; doing redirects
    (let [[_ _ query] (current-route)
-         from-param (decode-route (get query param))]
+         from-param (some-> (get query param) (decode-route))]
      (assoc-in coeffects [:routes/from-query-param param] from-param))))
 
 (defn start-routing!

src/cljs/airsonic_ui/subs.cljs

@@ -2,6 +2,14 @@
   (:require [re-frame.core :as re-frame :refer [subscribe]]
             [airsonic-ui.utils.api :as api]))
 
+(defn is-booting?
+  "Predicate to tell whether our app is still in the process of initialization"
+  [{:keys [credentials]} _]
+  (and (not (map? credentials))
+       (not (#{:credentials/not-found :credentials/verification-failure :credentials/logged-out} credentials))))
+
+(re-frame/reg-sub ::is-booting? is-booting?)
+
 ;; can be used to query the user's credentials
 
 (re-frame/reg-sub

src/cljs/airsonic_ui/views.cljs

@@ -68,10 +68,11 @@
        [bottom-bar]])))
 
 (defn main-panel []
-  (let [[route params query] @(subscribe [::subs/current-route])
+  (let [notifications @(subscribe [::subs/notifications])
-        notifications @(subscribe [::subs/notifications])]
+        is-booting? @(subscribe [::subs/is-booting?])
+        [route params query] @(subscribe [::subs/current-route])]
     [:div
      [notification-list notifications]
-     (if route
+     (if is-booting?
-       [app route params query]
+       [:div.app-loading>div.loader]
-       [:div.app-loading>div.loader])]))
+       [app route params query] )]))

test/cljs/airsonic_ui/events_test.cljs

@@ -13,18 +13,17 @@
   (letfn [(no-previous-session []
             (events/restore-previous-session {} [:_]))
           (has-previous-session []
-            (events/restore-previous-session {:store {:u "test"
+            (events/restore-previous-session {:store {:credentials {:u "test"
                                                                     :p "test"
-                                                      :server "https://demo.airsonic.io/"}} [:_]))]
+                                                                    :server "https://demo.airsonic.io/"}}} [:_]))]
     (testing "Should initialize routing after checking for previous credentials"
       (is (contains? (no-previous-session) :routes/start-routing))
       (is (contains? (has-previous-session) :routes/start-routing)))
     (testing "Should indicate success or failure"
-      (is (dispatches? (no-previous-session)) :init-flow/credentials-missing)
+      (is (true? (dispatches? (no-previous-session) :init-flow/credentials-not-found)))
-      (is (dispatches? (has-previous-session)) :init-flow/credentials-found))
+      (is (true? (dispatches? (has-previous-session) :init-flow/credentials-found))))
     (testing "Should send an auth request on success"
-      (is (dispatches? (events/credentials-found {} [:_]) :credentials/verification-request)))
+      (is (true? (dispatches? (events/credentials-found {} [:_]) :credentials/verification-request))))))
-    (testing "Should redirect to the login form when there's no previous session to be restored")))
 
 (deftest authentication
   (testing "Server ping for verifications"
@@ -32,20 +31,23 @@
           fx (events/credentials-verification-request {} [:_ "user" "pass" server])
           request (:http-xhrio fx)]
       (testing "uses correct server url"
-        (is (str/starts-with? (:uri request) server))
+        (let [uri (:uri request)]
-        (is (str/includes? (:uri request) "/ping"))
+          (is (true? (str/starts-with? uri server)))
-        (is (str/includes? (:uri request) "p=pass"))
+          (is (true? (str/includes? uri "/ping")))
-        (is (str/includes? (:uri request) "u=user")))
+          (is (true? (str/includes? uri "p=pass")))
+          (is (true? (str/includes? uri "u=user")))))
       (testing "invokes correct success callback"
         (is (= :credentials/verification-response (first (:on-success request)))))))
-  (testing "Auth response verification"
+  (testing "Auth response"
-    (let [server "https://localhost"
+    (testing "verification for bad responses"
-          fx (events/credentials-verification-response {} [:_ "user" "pass" server (:error responses)])]
+      (let [ev [:_ "user" "pass" "https://localhost"]
-      (is (= (dispatches? fx :notification/show))
+            invalid-credentials (events/credentials-verification-response {} (conj ev (:auth-failure responses)))
-          "shows an error when we have a bad response"))
+            verification-failure (events/credentials-verification-failure {} [:_ (:auth-failure responses)])]
+        (is (true? (dispatches? invalid-credentials :credentials/verification-failure)) "fails for bad responses")
+        (is (true? (dispatches? verification-failure :notification/show)) "shows the failure the the user")))
     (let [server "https://localhost"
           fx (events/credentials-verification-response {} [:_ "username" "password" server (:auth-success responses)])]
-      (is (dispatches? fx [:credentials/verified "username" "password" server]))))
+      (is (true? (dispatches? fx [:credentials/verified "username" "password" server])))))
   (testing "On succesful response"
     (let [credentials {:u "user" :p "pass" :server "https://localhost"}
           fx (events/credentials-verified {} [:_ (:u credentials) (:p credentials) (:server credentials)])]
@@ -54,7 +56,7 @@
     (testing "credentials are saved in the global state"
       (is (= credentials (get-in fx [:db :credentials]))))
     (testing "the login process is finalized"
-      (is (dispatches? fx ::events/logged-in))))))
+      (is (true? (dispatches? fx ::events/logged-in)))))))
 
 (deftest logout
   (let [fx (events/logout {} [:_])]
@@ -65,7 +67,7 @@
     (testing "Should unset authentication in the router"
       (is (contains? fx :routes/unset-credentials)))
     (testing "Should reset the app-db"
-      (is (= db/default-db (:db fx)))))
+      (is (= (every? #(= (get db/default-db %) (get-in fx [:db %])) (keys db/default-db))))))
   (testing "Should be able to keep a redirection parameter"
     (let [redirect [:route {:with-data #{1 2 3 4 5}}]
           fx (events/logout {} [:_ :redirect-to redirect])]

test/cljs/airsonic_ui/fixtures.cljs

@@ -1,8 +1,8 @@
 (ns airsonic-ui.fixtures)
 
 (def responses {:error {:subsonic-response
-                        {:error {:code 40
+                        {:error {:code 50
-                                 :message "Wrong username or password"}
+                                 :message "Incompatible Airsonic REST protocol version. Server must upgrade."}
                          :status "failed"
                          :version "1.15.0"}}
                 :ok {:subsonic-response
@@ -11,7 +11,11 @@
                       :status "ok"
                       :version "1.15.0"}}
                 :auth-success {:subsonic-response {:status "ok"
-                                                   :version "1.15.0"}}})
+                                                   :version "1.15.0"}}
+                :auth-failure {:subsonic-response {:status "failed"
+                                                   :version "1.15.0"
+                                                   :error {:code 40
+                                                           :message "Wrong username or password."}}}})
 
 (def song
   {:artistId 42,

test/cljs/airsonic_ui/subs_test.cljs

@@ -1,9 +1,42 @@
 (ns airsonic-ui.subs-test
   (:require [cljs.test :refer [deftest testing is]]
-            [airsonic-ui.fixtures :refer [song]]
+            [airsonic-ui.db :as db]
+            [airsonic-ui.fixtures :refer [song] :as fixtures]
             [airsonic-ui.utils.api :as api]
+            [airsonic-ui.events :as ev]
             [airsonic-ui.subs :as subs]))
 
+(def creds {:credentials {:u "test"
+                          :p "test"
+                          :server "https://demo.airsonic.io/"}})
+
+(deftest is-booting
+  (testing "Should be true when provided the initial state"
+    (is (true? (subs/is-booting? db/default-db [:_]))))
+  (testing "Should be true when we have credentials but no response yet"
+    (is (true? (-> (ev/restore-previous-session {:store creds} [:_])
+                   (ev/credentials-found [:_])
+                   :db
+                   (subs/is-booting? [:_])))))
+  (testing "Should be false when the login screen is shown"
+    (is (false? (-> (ev/restore-previous-session {} [:_])
+                    (ev/credentials-not-found [:_])
+                    :db
+                    (subs/is-booting? [:_])))))
+  (let [{:keys [u p server]} (:credentials creds)]
+    (testing "Should be false after we verified our credentials with the server"
+      (is (false? (-> (ev/credentials-verified {:db {}} [:_ u p server])
+                      :db
+                      (subs/is-booting? [:_])))))
+    (testing "Should be false after the server rejected our credentials"
+      (is (false? (-> (ev/credentials-verification-failure {} [:_ (:auth-failure fixtures/responses)])
+                      :db
+                      (subs/is-booting? [:_]))))))
+  (testing "Should be false when a user logged out voluntarily"
+    (is (false? (-> (ev/logout {} [:_])
+                    :db
+                    (subs/is-booting? [:_]))))))
+
 (deftest cover-images
   (let [credentials {:server "https://foo.bar"
                      :u "test-user"

test/cljs/airsonic_ui/utils/api_test.cljs

@@ -47,3 +47,11 @@
         (api/unwrap-response error-response)
         (catch ExceptionInfo e
           (= (:error error-response) (ex-data e)))))))
+
+(deftest error-recognition
+  (testing "Should detect error responses"
+    (is (true? (api/is-error? (:error responses))))
+    (is (true? (api/is-error? (:auth-failure responses)))))
+  (testing "Should pass on good responses"
+    (is (false? (api/is-error? (:ok responses))))
+    (is (false? (api/is-error? (:auth-success responses))))))