Restructure app init; fixes #5 and #11

Squashed commit of the following:

commit d4242bf1a390994606b7bd6e630c55338a14aad4
Author: Arne Schlüter <arne@schlueter.is>
Date:   Mon Jul 9 21:12:44 2018 +0200

    Add loading spinner, done with reworked app boot flow; fixes #5 and #11

commit e864ae4e578f96b86f3c0239b79f5224f0bb0020
Author: Arne Schlüter <arne@schlueter.is>
Date:   Mon Jul 9 19:43:02 2018 +0200

    Start restructuring app boot flow

commit a8cdbef80acde9f185a588ab86f8ea6964ebe8ab
Author: Arne Schlüter <arne@schlueter.is>
Date:   Mon Jul 9 14:03:43 2018 +0200

    Ignore rebel readline artifacts

commit 67eae3bc6aa2938ad6748c78b6259e532e66f865
Author: Arne Schlüter <arne@schlueter.is>
Date:   Mon Jul 9 14:03:11 2018 +0200

    Update shadow-cljs and run npm audit fix

.gitignore

@@ -1,4 +1,5 @@
 .nrepl-port
+.rebel_readline_history
 .shadow-cljs/
 node_modules/
 public/*

package.json

@@ -35,15 +35,15 @@
   },
   "devDependencies": {
     "gh-pages": "^1.2.0",
-    "karma": "^2.0.2",
+    "karma": "^2.0.4",
     "karma-chrome-launcher": "^2.2.0",
     "karma-cljs-test": "^0.1.0",
     "karma-growl-reporter": "^1.0.0",
-    "node-sass": "^4.9.0",
+    "node-sass": "^4.9.2",
     "npm-run-all": "^4.1.2",
     "react-flip-move": "^3.0.1",
     "react-highlight.js": "^1.0.7",
     "sass": "^1.3.2",
-    "shadow-cljs": "^2.3.19"
+    "shadow-cljs": "^2.4.17"
   }
 }

src/cljs/airsonic_ui/core.cljs

@@ -21,10 +21,8 @@
   (reagent/render [views/main-panel] (.getElementById js/document "app")))
 
 (defn ^:export init []
-  (routes/start-routing!)
   (storage/reg-co-fx! :airsonic-ui {:fx :store
                                     :cofx :store})
-  (re-frame/dispatch-sync [::events/initialize-db])
+  (re-frame/dispatch-sync [::events/initialize-app])
-  (re-frame/dispatch [::events/try-remember-user])
   (dev-setup)
   (mount-root))

src/cljs/airsonic_ui/db.cljs

@@ -2,6 +2,4 @@
   (:require [airsonic-ui.routes :as routes]))
 
 (def default-db
-  {;; because navigate! executes asynchronously we force to display the login screen first
+  {:notifications (sorted-map)})
-   :current-route [routes/default-route]
-   :notifications (sorted-map)})

src/cljs/airsonic_ui/events.cljs

@@ -4,12 +4,7 @@
             [airsonic-ui.routes :as routes]
             [airsonic-ui.db :as db]
             [airsonic-ui.utils.api :as api]
-            [day8.re-frame.tracing :refer-macros [fn-traced]])) ; <- useful to debug handlers
+            [day8.re-frame.tracing :refer-macros [fn-traced defn-traced]])) ; <- useful to debug handlers
-
-;; this is where all of the event handling takes place; the names put the events into
-;; the following categories:
-;; ::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
@@ -17,71 +12,88 @@
  (fn [params]
    (apply println params)))
 
-;; database reset / init
+;; ---
+;; app boot flow
+;; * restoring a previous session
+;; * initializing the router
+;; * sending out the appropriate requests
+;; ---
 
-(re-frame/reg-event-db
+(re-frame/reg-event-fx
- ::initialize-db
+ ::initialize-app
  (fn [_]
-   db/default-db))
+   {:db db/default-db
+    :dispatch [:init-flow/restore-previous-session]}))
+
+(defn restore-previous-session
+  "See comment above for different steps; what's important here is that we check
+  for a previous session before anything else, otherwise we might run into auth
+  troubles with our router."
+  [{:keys [db store]} _]
+  (let [credentials (:credentials store)]
+    {:db (assoc db :credentials credentials)
+     :dispatch-n [(if credentials
+                    [:init-flow/credentials-found credentials]
+                    [:init-flow/credentials-missing])]
+     :routes/start-routing nil}))
+
+(re-frame/reg-event-fx
+ :init-flow/restore-previous-session
+ [(re-frame/inject-cofx :store)]
+ restore-previous-session)
+
+(defn credentials-found [_ [_ {:keys [u p server]}]]
+  {:dispatch [:credentials/verification-request u p server]})
+
+(re-frame/reg-event-fx
+ :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 [_ _] {}))
 
 ;; ---
 ;; auth logic
 ;; ---
 
-(defn authenticate
+(defn-traced credentials-verification-request
   "Tries to authenticate a user by pinging the server with credentials, saving
-  them when the request was succesful. Bypasses the request when a user saved
+  them when the request was successful. Bypasses the request when a user saved
   their credentials."
-  [{:keys [db]} [_ user pass server]]
+  [_ [_ user pass server]]
-  {:db (assoc-in db [:credentials :server] server)
+  {:http-xhrio {:method :get
-   :http-xhrio {:method :get
                 :uri (api/url server "ping" {:u user :p pass})
                 :response-format (ajax/json-response-format {:keywords? true})
-                :on-success [::verify-auth-response user pass]
+                :on-success [:credentials/verification-response user pass server]
                 :on-failure [:api/bad-response]}})
 
 (re-frame/reg-event-fx
- ::authenticate authenticate)
+ :credentials/verification-request credentials-verification-request)
 
-(defn verify-auth-response
+(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 response]]
+  [fx [_ user pass server response]]
   {:dispatch (if (api/is-error? response)
                [:notification/show :error (api/error-msg (api/->exception response))]
-               [::credentials-verified user pass])})
+               [:credentials/verified user pass server])})
 
 (re-frame/reg-event-fx
- ::verify-auth-response verify-auth-response)
+ :credentials/verification-response credentials-verification-response)
-
-(defn try-remember-user
-  "Enables skipping the auth request when credentials are saved in the
-  local storage; otherwise has no effect"
-  [{:keys [db store]} [_]]
-  (when-let [credentials (:credentials store)]
-    {:db (assoc-in db [:credentials :server] (:server credentials))
-     :dispatch [::credentials-verified (:u credentials) (:p credentials) nil]}))
-
-(re-frame/reg-event-fx
- ::try-remember-user
- [(re-frame/inject-cofx :store)]
- try-remember-user)
 
 (defn credentials-verified
   "Gets called after the server indicates that the credentials entered by a user
-  are correct (see `authenticate`)"
+  are correct (see `credentials-verification-request`)"
-  [{:keys [db store]} [_ user pass]]
+  [{:keys [db]} [_ user pass server]]
-  (let [auth {:u user :p pass}
+  (let [credentials {:u user :p pass :server server}]
-        credentials (merge (:credentials db) auth)]
+    {:routes/set-credentials credentials
-    {:routes/set-credentials auth
      :store {:credentials credentials}
      :db (assoc db :credentials credentials)
      :dispatch [::logged-in]}))
 
 (re-frame/reg-event-fx
- ::credentials-verified
+ :credentials/verified credentials-verified)
- [(re-frame/inject-cofx :store)]
- 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
@@ -91,20 +103,28 @@
  (fn [_]
    (.. js/document -documentElement -classList (add "has-navbar-fixed-bottom"))))
 
-;; we do this in two steps to make sure the credentials are set once we navigate
+(defn logged-in
+  [cofx _]
+  (let [redirect (or (get-in cofx [:routes/from-query-param :redirect])
+                     [::routes/main])]
+    {:routes/navigate redirect
+     :show-nav-bar nil}))
+
 (re-frame/reg-event-fx
  ::logged-in
- (fn [_ _]
+ [(re-frame/inject-cofx :routes/from-query-param :redirect)]
-   {:routes/navigate [::routes/main]
+ logged-in)
-    :show-nav-bar nil}))
 
 (defn logout
   "Clears all credentials and redirects the user to the login page"
-  [_ _]
+  [_ [_ & args]]
-  {:routes/navigate [::routes/login]
+  (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 db/default-db}))
 
 (re-frame/reg-event-fx
  ::logout logout)
@@ -136,11 +156,12 @@
 (re-frame/reg-event-fx
  :api/good-response good-api-response)
 
-(re-frame/reg-event-fx
+(defn bad-api-response [db event]
- :api/bad-response
- (fn [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."]}))
+   :dispatch [:notification/show :error "Communication with server failed. Check browser logs for details."]})
+
+(re-frame/reg-event-fx
+ :api/bad-response bad-api-response)
 
 ;; ---
 ;; musique
@@ -206,8 +227,9 @@
 
 (re-frame/reg-event-fx
  :routes/unauthorized
- (fn [_ _]
+ [(re-frame/inject-cofx :routes/current-route)]
-   {:dispatch [::logout]}))
+ (fn [{:routes/keys [current-route]} _]
+   {:dispatch [::logout :redirect-to current-route]}))
 
 ;; ---
 ;; user messages

src/cljs/airsonic_ui/routes.cljs

@@ -1,5 +1,6 @@
 (ns airsonic-ui.routes
   (:require [bide.core :as r]
+            [cljs.reader :refer [read-string]]
             [re-frame.core :as re-frame]))
 
 (def default-route ::login)
@@ -72,8 +73,41 @@
     (re-frame/dispatch [:routes/navigation route-id params query])
     (re-frame/dispatch [:routes/unauthorized route-id params query])))
 
+(defn encode-route
+  "Takes a parsed route and returns a representation that's suitable for
+  transportation in a uri component"
+  [route]
+  (js/encodeURIComponent (str route)))
+
+(defn decode-route
+  "Decodes and encoded route from a uri component into a parsed route"
+  [encoded-route]
+  (read-string (js/decodeURIComponent encoded-route)))
+
+(defn current-route
+  "Returns the parsed route for window.location.hash"
+  []
+  (r/match router (subs (.. js/window -location -hash) 1)))
+
+(re-frame/reg-cofx
+ :routes/current-route
+ (fn [coeffects _]
+   (assoc coeffects :routes/current-route (current-route))))
+
+(re-frame/reg-cofx
+ :routes/from-query-param
+ (fn [coeffects param]
+   ;; 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))]
+     (assoc-in coeffects [:routes/from-query-param param] from-param))))
+
 (defn start-routing!
   "Initializes the router and makes sure the correct events get dispatched."
-  []
+  ([] (r/start! router {:default default-route
-  (r/start! router {:default default-route
                         :on-navigate on-navigate}))
+  ([_] (start-routing!))) ;; <- 1-arity is for the re-frame effect exposed below
+
+(re-frame/reg-fx
+ :routes/start-routing start-routing!)

src/cljs/airsonic_ui/views.cljs

@@ -6,6 +6,7 @@
             [airsonic-ui.subs :as subs]
 
             [airsonic-ui.views.notifications :refer [notification-list]]
+            [airsonic-ui.views.loading-spinner :refer [loading-spinner]]
             [airsonic-ui.views.breadcrumbs :refer [breadcrumbs]]
             [airsonic-ui.views.bottom-bar :refer [bottom-bar]]
             [airsonic-ui.views.login :refer [login-form]]
@@ -51,6 +52,8 @@
 (defn app [route params query]
   (let [user @(subscribe [::subs/user])
         content @(subscribe [::subs/current-content])]
+    (if (= route ::routes/login)
+      [login-form]
       [:div
        [:main.columns
         [:div.column.is-2.sidebar
@@ -62,13 +65,13 @@
             ::routes/main [most-recent content]
             ::routes/artist-view [artist-detail content]
             ::routes/album-view [album-detail content])]]]
-     [bottom-bar]]))
+       [bottom-bar]])))
 
 (defn main-panel []
   (let [[route params query] @(subscribe [::subs/current-route])
         notifications @(subscribe [::subs/notifications])]
     [:div
      [notification-list notifications]
-     (case route
+     (if route
-       ::routes/login [login-form]
+       [app route params query]
-       [app route params query])]))
+       [:div.app-loading>div.loader])]))

src/cljs/airsonic_ui/views/login.cljs

@@ -16,7 +16,7 @@
         server (r/atom (.. js/window -location -origin))
         submit (fn [e]
                  (.preventDefault e)
-                 (dispatch [::events/authenticate @user @pass @server]))]
+                 (dispatch [:credentials/verification-request @user @pass @server]))]
     (fn []
       [:section.hero.is-fullheight>div.hero-body
        [:div.container.has-text-centered>div.column.is-4.is-offset-4

src/sass/app.sass

@@ -6,6 +6,17 @@
   main
     margin-bottom: 0
 
+// big loading spinner
+.app-loading
+  display: flex
+  justify-content: center
+  align-items: center
+  height: 100vh
+  font-size: 4.8rem
+  color: $grey-light
+  .loader
+    +loader
+
 // navi on the left side
 .sidebar
   min-height: 100vh
@@ -87,11 +98,11 @@
 @keyframes you-spin-my-head-right-round
   from
     transform: rotate(0deg)
-    transform-origin: 50% 48%
+    transform-origin: 49% 50%
 
   to
     transform: rotate(359deg)
-    transform-origin: 50% 48%
+    transform-origin: 49% 50%
 
 .loading-spinner
   .icon

test/cljs/airsonic_ui/events_test.cljs

@@ -8,50 +8,64 @@
 
 (enable-console-print!)
 
+(into [] (conj [[:foo :bar :baz]] nil))
+
+(defn dispatches?
+  "Helper to see whether an event is dispatched in a coeffect; `ev` can either
+  be a whole vector or a keyword which is interpreted as the event name."
+  [cofx ev]
+  (let [all-events (conj (or (:dispatch-n cofx) []) (:dispatch cofx))]
+    (some #(if (vector? ev)
+             (= ev %)
+             (= ev (first %)))
+          all-events)))
+
+(deftest session-restoration
+  (letfn [(no-previous-session []
+            (events/restore-previous-session {} [:_]))
+          (has-previous-session []
+            (events/restore-previous-session {:store {:u "test"
+                                                      :p "test"
+                                                      :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 (dispatches? (has-previous-session)) :init-flow/credentials-found))
+    (testing "Should send an auth request on success"
+      (is (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 "Credential verification"
+  (testing "Server ping for verifications"
     (let [server "https://localhost"
-          fx (events/authenticate {:db {}} [:_ "user" "pass" server])
+          fx (events/credentials-verification-request {} [:_ "user" "pass" server])
           request (:http-xhrio fx)]
       (testing "uses correct server url"
         (is (str/starts-with? (:uri request) server))
-        (is (str/includes? (:uri request) "/ping")))
+        (is (str/includes? (:uri request) "/ping"))
-      (testing "saves the given server location"
+        (is (str/includes? (:uri request) "p=pass"))
-        (is (= server (get-in fx [:db :credentials :server]))))
+        (is (str/includes? (:uri request) "u=user")))
       (testing "invokes correct success callback"
-        (is (= ::events/verify-auth-response (first (:on-success request)))))))
+        (is (= :credentials/verification-response (first (:on-success request)))))))
   (testing "Auth response verification"
-    (is (= :notification/show
+    (let [server "https://localhost"
-           (first (:dispatch (events/verify-auth-response {} [:_ "user" "pass" (:error responses)]))))
+          fx (events/credentials-verification-response {} [:_ "user" "pass" server (:error responses)])]
-        "shows an error when we have an error response")
+      (is (= (dispatches? fx :notification/show))
-    (let [event (:dispatch (events/verify-auth-response {} [:_ "username" "password" (:auth-success responses)]))]
+          "shows an error when we have a bad response"))
-      (is (= [::events/credentials-verified "username" "password"] event))))
+    (let [server "https://localhost"
+          fx (events/credentials-verification-response {} [:_ "username" "password" server (:auth-success responses)])]
+      (is (dispatches? fx [:credentials/verified "username" "password" server]))))
   (testing "On succesful response"
-    (let [creds-before {:server "https://localhost"}
+    (let [credentials {:u "user" :p "pass" :server "https://localhost"}
-          fx (events/credentials-verified {:db {:credentials creds-before}}
+          fx (events/credentials-verified {} [:_ (:u credentials) (:p credentials) (:server credentials)])]
-                                          [:_ "user" "pass"])
-          auth {:u "user" :p "pass"}]
     (testing "credentials are sent to the router for access rights"
-        (is (= auth (:routes/set-credentials fx))))
+      (is (= credentials (:routes/set-credentials fx))))
     (testing "credentials are saved in the global state"
-        (is (= auth (-> (get-in fx [:db :credentials])
+      (is (= credentials (get-in fx [:db :credentials]))))
-                               (select-keys [:u :p])))))
-      (testing "credentials are persisted together with the server address"
-        (is (= (merge creds-before auth) (get-in fx [:store :credentials]))))
     (testing "the login process is finalized"
-        (is (= [::events/logged-in] (:dispatch fx))))))
+      (is (dispatches? fx ::events/logged-in))))))
-  (testing "When remembering previous login data"
-    (let [credentials {:server "http://localhost"
-                       :u "another-user"
-                       :p "some_random_password123"}
-          fx (events/try-remember-user {:store {:credentials credentials}} [:_])]
-      (testing "the auth request is skipped"
-        (is (nil? (:http-xhrio fx))))
-      (testing "we get sent straight to the home page"
-        (is (= ::events/credentials-verified (first (:dispatch fx)))))))
-  (testing "When there's no previous login data"
-    (testing "remembering has no effect"
-      (is (nil? (events/try-remember-user {} [:_]))))))
 
 (deftest logout
   (let [fx (events/logout {} [:_])]
@@ -62,7 +76,11 @@
     (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 (= db/default-db (:db fx)))))
+  (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])]
+      (is (= [::routes/login {:redirect redirect}])))))
 
 (defn- first-notification [fx]
   (-> (get-in fx [:db :notifications]) vals first))

test/cljs/airsonic_ui/routes_test.cljs

@@ -0,0 +1,14 @@
+(ns airsonic-ui.routes-test
+  (:require [airsonic-ui.routes :as routes]
+            [cljs.test :refer [deftest testing is]]))
+
+(def fixtures
+  {:default [::route {:some :data} {:some-more true}]})
+
+(deftest route-encoding
+  (testing "Should return a string with hash-compatible characters"
+    (let [encoded (routes/encode-route (:default fixtures))]
+      (is (string? encoded))
+      (is (re-matches #"^[^#?&=]+$" encoded))))
+  (testing "Should be bijective"
+    (is (= (:default fixtures) (routes/decode-route (routes/encode-route (:default fixtures)))))))