From 4d8a98b5568913120edd0ecbf2635a3a9c92f4f6 Mon Sep 17 00:00:00 2001
From: David Mason <drdmason@gmail.com>
Date: Mon, 31 Jul 2017 01:07:18 +1000
Subject: [PATCH] feat(ZNTA-975): limit to a single phrase detail request at a
 time

Also only fetches phrases that do not yet have detail present.
---
 .../frontend/app/editor/selectors/index.js    | 49 +++++++++----------
 .../app/editor/watchers/phrase-detail.js      | 17 ++++---
 2 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/server/zanata-frontend/src/frontend/app/editor/selectors/index.js b/server/zanata-frontend/src/frontend/app/editor/selectors/index.js
index e8da063c6cb..0031433cf71 100644
--- a/server/zanata-frontend/src/frontend/app/editor/selectors/index.js
+++ b/server/zanata-frontend/src/frontend/app/editor/selectors/index.js
@@ -83,14 +83,31 @@ const getLocale = createSelector(
   (lang, locales) => locales[lang]
 )
 
-/* Phrases and locale object, needed for phrase detail.
- * Locale may be undefined
+/* Visible phrases that do not have data yet.
+ *
+ * Must only use this when phrase detail is not requesting, to avoid requesting
+ * the same detail twice.
+ *
+ * TODO add timestamps to detail items to allow including stale phrases here
  */
-export const getCurrentPagePhrasesAndLocale =
+export const getMissingPhrases = createSelector(
+  getCurrentPagePhrases, getPhrasesDetail,
+  (currentPage, detail) =>
+    currentPage.filter(({ id }) => !detail.hasOwnProperty(id))
+)
+
+const getFetchingPhraseDetail = state => state.phrases.fetchingDetail
+
+/* Data needed for fetching phrase detail.
+ *
+ * Phrases, locale object, and whether details is currently fetching.
+ * Locale may be undefined.
+ */
+export const getPhraseDetailFetchData =
   // deep equal since filtered phrases can be a new array with the same contents
   createSelectorCreator(defaultMemoize, isEqual)(
-    getCurrentPagePhrases, getLocale,
-    (phrases, locale) => ({ phrases, locale })
+    getMissingPhrases, getLocale, getFetchingPhraseDetail,
+    (phrases, locale, fetching) => ({ phrases, locale, fetching })
   )
 
 /* Detail for the current page, falling back on flyweight. */
@@ -102,28 +119,6 @@ export const getCurrentPagePhraseDetail = createSelector(
   }
 )
 
-/* Visible phrases that do not have data yet.
- * Careful here - do not want to keep firing requests for phrases that are
- * already fetching.
- *
- * FIXME could include stale phrases here
- * FIXME need state indicating which phrases have been requested recently
- * Note: this will trigger when fetchingPhrases updates, but usually the list
- *       would be empty since all the ids that made it through before will be
- *       filtered out in the first step.
- */
-// export const getMissingPhrases = createSelector(
-//   getCurrentPagePhrases, getPhrasesDetail,
-//   (currentPage, detail) => {
-//     // TODO add fetchingPhrases set in state
-//     const fetchingPhrases = new Set([])
-//     currentPage
-//       .filter(phrase => !fetchingPhrases.has(phrase.id))
-//       .map(({ id }) => id)
-//       .filter(id => !detail.hasOwnProperty(id))
-//   }
-// )
-
 // may be undefined
 export const getLocation = state => state.routing.locationBeforeTransitions
 // may be undefined
diff --git a/server/zanata-frontend/src/frontend/app/editor/watchers/phrase-detail.js b/server/zanata-frontend/src/frontend/app/editor/watchers/phrase-detail.js
index fad7591a2bc..2efb804d258 100644
--- a/server/zanata-frontend/src/frontend/app/editor/watchers/phrase-detail.js
+++ b/server/zanata-frontend/src/frontend/app/editor/watchers/phrase-detail.js
@@ -8,7 +8,7 @@
  */
 
 import watch from './watch'
-import { getCurrentPagePhrasesAndLocale } from '../selectors'
+import { getPhraseDetailFetchData } from '../selectors'
 import { baseRestUrl } from '../api'
 import { fill, isEmpty, mapValues } from 'lodash'
 import { getJSON } from 'redux-api-middleware'
@@ -23,15 +23,16 @@ import {
   PHRASE_DETAIL_FAILURE
 } from '../actions/phrases-action-types'
 
-// FIXME add state.phrases.fetchingDetail (bool) and only fetch when false.
-//   this requires watching fetchingDetail as well, which will work great to
-//   fetch the next batch after the current batch are merged in.
 export const watchVisiblePhrasesInStore = (store) => {
   const watcher = watch('phrase-detail > watchVisiblePhrasesInStore')(
-    () => getCurrentPagePhrasesAndLocale(store.getState()))
-  store.subscribe(watcher(({ phrases, locale }) => {
-    // optimization: only fetch if ID is not currently fetching and there is no
-    // recent detail in the cache.
+    () => getPhraseDetailFetchData(store.getState()))
+  store.subscribe(watcher(({ phrases, locale, fetching }) => {
+    // Only want one detail request at a time.
+    // Watcher triggers again when this flips to false.
+    if (fetching) {
+      return
+    }
+
     const ids = phrases.map(phrase => phrase.id)
     if (locale && !isEmpty(ids)) {
       // TODO debounce to handle rapid page changes etc?