Cache API responses and make sure we remember more than just one

Closes #21.
Squashed commit of the following:

commit 964b29cf127cf51de86543d040bcb6c674b36d7e
Author: Arne Schlüter <arne@schlueter.is>
Date:   Wed Aug 22 17:56:48 2018 +0200

    Pass content for current route nicely to views

commit b469a0a4b69457ddf3a679ac1acc82fbaffdc8fd
Author: Arne Schlüter <arne@schlueter.is>
Date:   Wed Aug 22 16:01:04 2018 +0200

    Add response cache in app-db

commit da9faf89138f42ee544efc64c2e46787091b3dc7
Author: Arne Schlüter <arne@schlueter.is>
Date:   Wed Aug 22 13:40:57 2018 +0200

    Move api helpers and tests to own namespace

src/cljs/airsonic_ui/api/events.cljs

@@ -0,0 +1,51 @@
+(ns airsonic-ui.api.events
+  "This namespace contains all events relevant to API interaction. It contains
+  an event handler which issues requests as well as the appropriate handlers,
+  which dispatch :notification events in case of errors."
+  (:require [re-frame.core :refer [reg-event-fx]]
+            [ajax.core :as ajax]
+            [airsonic-ui.api.helpers :as api]))
+
+(defn- api-url
+  "Small helper function which makes constructing API URLs a bit easier"
+  [db endpoint params]
+  (let [creds (:credentials db)]
+    (api/url (:server creds) endpoint (merge params (select-keys creds [:u :p])))))
+
+(defn- cache-path [endpoint params] [:api/responses [endpoint params]])
+
+(defn api-request
+  "Event handler to issue API request; takes care of authorization based on our
+  current app state."
+  [{: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 endpoint params]
+                :on-failure [:api/failed-response endpoint params]}
+   :db (assoc-in db (conj (cache-path endpoint params) :api/is-loading?) true)})
+
+(reg-event-fx :api/request api-request)
+
+(defn good-api-response
+  "Handles when the server responded. There could still be an error while
+  processing the request on the server side which we have to account for."
+  [fx [_ endpoint params response]]
+  (let [response-cache (cons :db (cache-path endpoint params))]
+    (try
+      (assoc-in fx response-cache (api/unwrap-response response))
+      (catch ExceptionInfo e
+        {:dispatch [:notification/show :error (api/error-msg e)]
+         :db (update-in fx response-cache dissoc :api/is-loading?)}))))
+
+(reg-event-fx :api/good-response good-api-response)
+
+(defn failed-api-response
+  "Handler for catastrophic failures (network errors and such things)"
+  [fx [ev endpoint params]]
+  (let [response-cache (cons :db (cache-path endpoint params))]
+    {:log ["API call gone bad; are CORS headers missing? check for :status 0" ev] ; <- the :log effect is registered in ../events.cljs
+     :dispatch [:notification/show :error "Communication with server failed. Check browser logs for details."]
+     :db (update-in fx response-cache dissoc :api/is-loading?)}))
+
+(reg-event-fx :api/failed-response failed-api-response)

src/cljs/airsonic_ui/api/helpers.cljs

@@ -1,6 +1,5 @@
-(ns airsonic-ui.utils.api
-  (:require [clojure.string :as str]
-            [airsonic-ui.config :as config]))
+(ns airsonic-ui.api.helpers
+  (:require [clojure.string :as str]))
 
 (def default-params {:f "json"
                      :c "airsonic-ui-cljs"

src/cljs/airsonic_ui/api/subs.cljs

@@ -0,0 +1,26 @@
+(ns airsonic-ui.api.subs
+  (:require [clojure.string :as str]
+            [re-frame.core :refer [reg-sub]]))
+
+(defn endpoint->kw
+  "Given an endpoint like `getAlbumList2`, returns a cleaned keyword like
+  `:album-list``.
+
+  Rules: Kebab-case everything, remove prefixes like `get`, `create`, `delete`,
+  `update` and strip trailing numbers."
+  [endpoint-str]
+  (-> (str/replace endpoint-str #"^(get|create|update|delete)" "")
+      (str/replace #"\d+$" "")
+      (str/replace #"([a-z])([A-Z])" (fn [[_ a b]] (str a "-" b)))
+      (str/lower-case)
+      (keyword)))
+
+(defn route-data
+  "Given a list of event vectors, returns that responses for all API requests."
+  [db [_ route-events]]
+  (->> (filter #(= :api/request (first %)) route-events)
+       (mapcat (fn [[_ endpoint params]]
+                 [(endpoint->kw endpoint) (get-in db [:api/responses [endpoint params]])]))
+       (apply hash-map)))
+
+(reg-sub :api/route-data route-data)

src/cljs/airsonic_ui/core.cljs

@@ -4,8 +4,12 @@
             ;; 3rd party effects / coeffects
             [day8.re-frame.http-fx]
             [akiroz.re-frame.storage :as storage]
-            ;; our app
-            [airsonic-ui.audio.core] ; <- just registers effects here
+
+            ;; our app; namespaces that are just required but not used register
+            ;; event handlers, effect handlers or subscriptions
+            [airsonic-ui.audio.core]
+            [airsonic-ui.api.events]
+            [airsonic-ui.api.subs]
             [airsonic-ui.events :as events]
             [airsonic-ui.views :as views]
             [airsonic-ui.config :as config]))

src/cljs/airsonic_ui/events.cljs

@@ -3,7 +3,7 @@
             [ajax.core :as ajax]
             [airsonic-ui.routes :as routes]
             [airsonic-ui.db :as db]
-            [airsonic-ui.utils.api :as api]
+            [airsonic-ui.api.helpers :as api]
             [airsonic-ui.audio.playlist :as playlist]))
 
 (re-frame/reg-fx
@@ -69,7 +69,7 @@
                            :uri (api/url (:server credentials) "ping" (select-keys credentials [:u :p]))
                            :response-format (ajax/json-response-format {:keywords? true})
                            :on-success [:credentials/authentication-response credentials]
-                           :on-failure [:api/bad-response]}))
+                           :on-failure [:api/failed-response]})) ; <- we don't need endpoint and params here because the response is not cached
 
 (re-frame/reg-event-fx :credentials/send-authentication-request authentication-request)
 
@@ -135,37 +135,6 @@
 
 (re-frame/reg-event-fx ::logout logout)
 
-;; ---
-;; api interaction
-;; ---
-
-(defn- api-url [db endpoint params]
-  (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 api-request)
-
-(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-fx :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 :api/bad-response bad-api-response)
-
 ;; ---
 ;; musique
 ;; ---
@@ -244,11 +213,8 @@
 (re-frame/reg-event-fx
  :routes/did-navigate
  (fn [{:keys [db]} [_ route params query]]
-   ;; FIXME: This leads to an ugly "unregistered event handler `nil`" error
-   ;; all the naviagation logic is in routes.cljs; all we need to do here
-   ;; is say what actually happens once we've navigated succesfully
-   {:db (assoc db :current-route [route params query])
-    :dispatch (routes/route-data route params query)}))
+   {:db (assoc db :routes/current-route [route params query])
+    :dispatch-n (routes/route-events route params query)}))
 
 (re-frame/reg-event-fx
  :routes/unauthorized

src/cljs/airsonic_ui/routes.cljs

@@ -11,37 +11,61 @@
              ["/artist/:id" ::artist-view]
              ["/album/:id" ::album-view]]))
 
-; use this in views to construct a url
+;; use this in views to construct a url
 (defn url-for
   ([k] (url-for k {}))
   ([k params] (str "#" (r/resolve router k params))))
 
-; which routes need valid login credentials?
+;; which routes need valid login credentials?
 (def protected-routes #{::main ::artist-view ::album-view})
 
-; which data should be requested for which route? can either be a vector or a function returning a vector
+;; which data should be requested for which route? can either be a vector or a function returning a vector
 
-(defmulti route-data
+(defmulti -route-events
   "Returns the events that take care of correct data being fetched."
   (fn [route-id & _] route-id))
 
-(defmethod route-data :default [route-id params query] []) ; no data
+(defmethod -route-events :default [route-id params query] nil)
 
-(defmethod route-data ::main
+(defmethod -route-events ::main
   [route-id params query]
   [:api/request "getAlbumList2" {:type "recent"
                                  :size 18}])
 
-(defmethod route-data ::artist-view
+(defmethod -route-events ::artist-view
   [route-id params query]
   [:api/request "getArtist" (select-keys params [:id])])
 
-(defmethod route-data ::album-view
+(defmethod -route-events ::album-view
   [route-id params query]
   [:api/request "getAlbum" (select-keys params [:id])])
 
 ;; shouldn't need to change anything below
 
+(defn- n-events?
+  "Predicate that tells us whether a vector is suitable for :dispatch-n"
+  [ev-vec]
+  (or (vector? (first ev-vec))))
+
+(defn route-events
+  "Returns a normalized list of event vectors for a given route."
+  [route-id params query]
+  (let [ev-vec (-route-events route-id params query)]
+    (if (n-events? ev-vec) ev-vec [ev-vec])))
+
+;; subscription returning the matched route for the current hashbang
+
+(re-frame/reg-sub :routes/current-route (fn [db _] (:routes/current-route db)))
+
+;; NOTE: There is some duplication here. The route events are provided as a
+;; subscription but they are also invoked directly in events.cljs. It didn't
+;; seem to justify pulling in a whole library and we need it in our top most view
+
+(re-frame/reg-sub
+ :routes/events-for-current-route
+ (fn [db _] (re-frame/subscribe [:routes/current-route]))
+ (fn [current-route _] (apply route-events current-route)))
+
 ;; these are helper effects we can use to navigate; the first two manage an atom
 ;; holding credentials, which is necessary to restrict certain routes, and the
 ;; last one is used for actual navigation
@@ -50,7 +74,7 @@
 ;; returned unaltered, we just need access to the current app database for
 ;; authentication, which we get with an interceptor
 
-(def ^:private credentials (atom nil))
+(defonce ^:private credentials (atom nil))
 
 (def do-navigation
   "An interceptor which performs the navigation after looking up current

src/cljs/airsonic_ui/subs.cljs

@@ -1,23 +1,22 @@
 (ns airsonic-ui.subs
-  (:require [re-frame.core :as re-frame :refer [subscribe]]
-            [airsonic-ui.audio.playlist :as playlist]
-            [airsonic-ui.utils.api :as api]))
+  (:require [re-frame.core :refer [reg-sub subscribe]]
+            [airsonic-ui.api.helpers :as api]))
 
 (defn is-booting?
   "The boot process starts with setting up routing and continues if we found
   previous credentials and ends when we receive a response from the server."
   [db _]
   ;; so either we don't have any credentials or they are not verified
-  (or (empty? (:current-route db))
+  (or (empty? (:routes/current-route db))
       (and (not (empty? (:credentials db)))
            (not (get-in db [:credentials :verified?])))))
 
-(re-frame/reg-sub ::is-booting? is-booting?)
+(reg-sub ::is-booting? is-booting?)
 
 (defn credentials [db _] (:credentials db))
-(re-frame/reg-sub ::credentials credentials)
+(reg-sub ::credentials credentials)
 
-(re-frame/reg-sub
+(reg-sub
  ::user
  (fn [_ _] [(subscribe [::credentials])])
  (fn [[credentials] _]
@@ -29,26 +28,12 @@
   [[{:keys [server u p]}] [_ song size]]
   (api/cover-url server {:u u :p p} song size))
 
-(re-frame/reg-sub
+(reg-sub
  ::cover-url
  (fn [_ _] [(subscribe [::credentials])])
  cover-url)
 
-;; current hashbang
-
-(re-frame/reg-sub
- ::current-route
- (fn [db _]
-   (:current-route db)))
-
-;; TODO: Make this nice and clean
-
-(re-frame/reg-sub
- ::current-content
- (fn [db _]
-   (:response db)))
-
 ;; user notifications
 
 (defn notifications [db _] (:notifications db))
-(re-frame/reg-sub ::notifications notifications)
+(reg-sub ::notifications notifications)

src/cljs/airsonic_ui/views.cljs

@@ -26,7 +26,7 @@
 (defn most-recent [content]
   [:div
    [:h2.title "Recently played"]
-   [album/listing (:album content)]])
+   [album/listing (get-in content [:album-list :album])]])
 
 (defn sidebar [user]
   [:aside.menu.section
@@ -49,7 +49,8 @@
 
 (defn app [route-id params query]
   (let [user @(subscribe [::subs/user])
-        content @(subscribe [::subs/current-content])]
+        route-events @(subscribe [:routes/events-for-current-route])
+        content @(subscribe [:api/route-data route-events])]
     [:div
      [:main.columns
       [:div.column.is-2.sidebar
@@ -66,7 +67,7 @@
 (defn main-panel []
   (let [notifications @(subscribe [::subs/notifications])
         is-booting? @(subscribe [::subs/is-booting?])
-        [route-id params query] @(subscribe [::subs/current-route])]
+        [route-id params query] @(subscribe [:routes/current-route])]
     [:div
      [notification-list notifications]
      (if is-booting?

test/cljs/airsonic_ui/api/events_test.cljs

@@ -0,0 +1,50 @@
+(ns airsonic-ui.api.events-test
+  (:require [cljs.test :refer-macros [deftest testing is]]
+            [airsonic-ui.api.events :as events]
+            [airsonic-ui.fixtures :as fixtures]))
+
+(enable-console-print!)
+
+(deftest api-failure-notifcations
+  (testing "Should show an error notification when airsonic responds with an error"
+    (let [fx (events/good-api-response {} [:api/good-response "ping" nil (:error fixtures/responses)])
+          ev (:dispatch fx)]
+      (is (= :notification/show (first ev)))
+      (is (= :error (second ev))))))
+
+(deftest cached-api-requests
+  (letfn [(cache [fx [endpoint params]]
+            (get-in fx [:db :api/responses [endpoint params]]))]
+    (testing "Should be cached"
+      (testing "when the response was successful"
+        (let [endpoint "getScanStatus"
+              successful (events/good-api-response {} [:api/good-response endpoint nil (:ok fixtures/responses)])
+              unsuccessful (events/good-api-response {} [:api/good-response endpoint nil (:error fixtures/responses)])]
+          (is (map? (cache successful [endpoint])))
+          (is (nil? (cache unsuccessful [endpoint])))))
+      (testing "in an unwrapped format"
+        (let [endpoint "getScanStatus"
+              fx (events/good-api-response {} [:api/good-response endpoint nil (:ok fixtures/responses)])]
+          (is (= #{:count :scanning} (set (keys (cache fx [endpoint]))))))))
+    (testing "When being issued"
+      (let [endpoint "getScanStatus"
+            fx (events/api-request {:db {:credentials (select-keys fixtures/credentials [:server])}}
+                                   [:api/request endpoint])]
+        (testing "should send an http request"
+          (is (contains? fx :http-xhrio)))
+        (testing "should indicate that a request is ongoing"
+          (is (true? (:api/is-loading? (cache fx [endpoint]))) "for non-cached responses")
+          (is (true? (-> (events/good-api-response fx [:api/good-response endpoint nil (:ok fixtures/responses)])
+                         (events/api-request [:api/request endpoint])
+                         (cache [endpoint])
+                         :api/is-loading?)) "for cached responses"))
+        (testing "should remove the indication that a request is ongoing when there is a response"
+          (is (not (:api/is-loading? (-> (events/good-api-response fx [:api/good-response endpoint nil (:ok fixtures/responses)])
+                                         (cache [endpoint])))) "for a good response")
+          (is (not (:api/is-loading? (-> (merge fx (events/good-api-response fx [:api/good-response endpoint nil (:error fixtures/responses)]))
+                                         (cache [endpoint])))) "when an error is returned")
+          (is (not (:api/is-loading? (-> (merge fx (events/failed-api-response fx [:api/failed-response endpoint]))
+                                         (cache [endpoint])))) "when communication with the server failed"))))
+    (testing "Should be able to avoid the cache"
+      ;; FIXME: Implement this
+      )))

test/cljs/airsonic_ui/api/helpers_test.cljs

@@ -1,8 +1,8 @@
-(ns airsonic-ui.utils.api-test
+(ns airsonic-ui.api.helpers-test
   (:require [cljs.test :refer [deftest testing is]]
             [clojure.string :as str]
             [airsonic-ui.fixtures :refer [responses]]
-            [airsonic-ui.utils.api :as api]))
+            [airsonic-ui.api.helpers :as api]))
 
 (defn- url
   "Construct a url with no params"

test/cljs/airsonic_ui/api/subs_test.cljs

@@ -0,0 +1,29 @@
+(ns airsonic-ui.api.subs-test
+  (:require [cljs.test :refer-macros [deftest testing is]]
+            [airsonic-ui.api.subs :as sub]))
+
+(enable-console-print!)
+
+(deftest endpoint-keywordification
+  (testing "Should strip prefixes"
+    (is (= :artist-info (sub/endpoint->kw "getArtistInfo")))
+    (is (= :jukebox-control (sub/endpoint->kw "jukeboxControl"))))
+  (testing "Should strip trailing numbers"
+    (is (= :album-list (sub/endpoint->kw "getAlbumList2")))
+    (is (= :search (sub/endpoint->kw "search3")))))
+
+(deftest responses-for-route
+  (testing "Should return all cached responses for a route"
+    (let [route-events [[:api/request "getAlbumList2" {:type "recent", :size 18}]
+                        [:event/should-be-ignored]
+                        [:api/request "getArtistInfo" {:id "128"}]]
+          db {:api/responses {["getAlbumList2" {:type "recent" :size 18}]
+                              {:album [{:genre "foo", :artistId "12345"}
+                                       {:genre "electronic", :artistId "9999"}]}
+
+                              ["getArtistInfo" {:id "128"}]
+                              {:biography "Interesting bio"
+                               :largeImageUrl "https://lastfm-img2.akamaized.net/i/u/300x300/fb416b59cd694587aca0b2dec8f41198.png"}}}]
+      (is (= {:album-list (get-in db [:api/responses ["getAlbumList2" {:type "recent" :size 18}]])
+              :artist-info (get-in db [:api/responses ["getArtistInfo" {:id "128"}]])}
+             (sub/route-data db [:api/route-data route-events]))))))

test/cljs/airsonic_ui/events_test.cljs

@@ -60,7 +60,7 @@
     (testing "invokes correct callback on server response"
       (is (= [:credentials/authentication-response fixtures/credentials] (:on-success request))))
     (testing "invokes correct callback when server is not reachable"
-      (is (= [:api/bad-response] (:on-failure request))))))
+      (is (= [:api/failed-response] (:on-failure request))))))
 
 (deftest authentication-response
   (testing "On success"
@@ -104,13 +104,6 @@
         (is (= ::routes/login route-id))
         (is (contains? query :redirect))))))
 
-(deftest api-interaction
-  (testing "Should show an error notification when airsonic responds with an error"
-    (let [fx (events/good-api-response {} [:_ (:error fixtures/responses)])
-          ev (:dispatch fx)]
-      (is (= :notification/show (first ev)))
-      (is (= :error (second ev))))))
-
 (defn- first-notification [fx]
   (-> (get-in fx [:db :notifications]) vals first))
 

test/cljs/airsonic_ui/subs_test.cljs

@@ -1,7 +1,7 @@
 (ns airsonic-ui.subs-test
   (:require [cljs.test :refer [deftest testing is]]
             [airsonic-ui.fixtures :as fixtures]
-            [airsonic-ui.utils.api :as api]
+            [airsonic-ui.api.helpers :as api]
             [airsonic-ui.subs :as subs]))
 
 (deftest booting
@@ -10,17 +10,17 @@
         is-booting? (fn is-booting? [db]
                       (subs/is-booting? db [:subs/is-booting?]))]
     (testing "Should be false when we don't have previous credentials"
-      (is (not (is-booting? {:current-route route})))
-      (is (not (is-booting? {:current-route route
+      (is (not (is-booting? {:routes/current-route route})))
+      (is (not (is-booting? {:routes/current-route route
                              :credentials {}}))) )
     (testing "Should be true when we have unverified credentials"
-      (is (true? (is-booting? {:current-route route
+      (is (true? (is-booting? {:routes/current-route route
                                :credentials fixtures/credentials}))))
     (testing "Should be false when we have verified credentials"
-      (is (not (is-booting? {:current-route route
+      (is (not (is-booting? {:routes/current-route route
                              :credentials verified-credentials}))))
     (testing "Should be true when routing is not yet set up"
-      (is (true? (is-booting? {:current-route nil
+      (is (true? (is-booting? {:routes/current-route nil
                                :credentials verified-credentials}))))))
 
 (deftest cover-images