Fixed browser back button

Browser Back button followed by Forward button wasn't properly
working.

To reproduce:

- Perform a text search
- Make a facet filter selection
- Press the back button
- Press the forward button
- Note that pressing the forward button will not reapply the filter,
  as it should.

Author: JasonStoltz

src/search-lib/URLManager.js

@@ -177,6 +177,10 @@ export default class URLManager {
       // want to notify that the URL changed.
       if (`?${this.lastPushSearchString}` === location.search) return;
 
+      // Once we've decided to return based on lastPushSearchString, reset
+      // it so that we don't break back / forward button.
+      this.lastPushSearchString = "";
+
       callback(
         paramsToState(
           queryString.parse(location.search, {

src/search-lib/URLManager.test.js

@@ -12,6 +12,50 @@ function createManager() {
   return manager;
 }
 
+const basicParameterState = {
+  filters: [
+    {
+      dependencies: ["underscore", "another"]
+    },
+    {
+      keywords: ["node"]
+    }
+  ],
+  resultsPerPage: 20,
+  searchTerm: "node",
+  sortDirection: "asc",
+  sortField: "name"
+};
+
+const basicParameterStateAsUrl =
+  "?fv-dependencies=underscore&fv-dependencies=another&fv-keywords=node&q=node&size=20&sort-direction=asc&sort-field=name";
+
+const parameterStateWithRangeFilters = {
+  filters: [
+    {
+      date: [
+        {
+          from: 12,
+          to: 4000
+        },
+        {
+          to: 4000
+        }
+      ]
+    },
+    {
+      cost: [
+        {
+          from: 50
+        }
+      ]
+    },
+    {
+      keywords: "node"
+    }
+  ]
+};
+
 it("can be initialized", () => {
   const manager = new URLManager();
   expect(manager).toBeInstanceOf(URLManager);
@@ -58,20 +102,7 @@ describe("#getStateFromURL", () => {
 describe("#pushStateToURL", () => {
   it("will update the url with the url corresponding to the provided state", () => {
     const manager = createManager();
-    manager.pushStateToURL({
-      filters: [
-        {
-          dependencies: ["underscore", "another"]
-        },
-        {
-          keywords: ["node"]
-        }
-      ],
-      resultsPerPage: 20,
-      searchTerm: "node",
-      sortDirection: "asc",
-      sortField: "name"
-    });
+    manager.pushStateToURL(basicParameterState);
     const queryString = manager.history.push.mock.calls[0][0].search;
     expect(queryString).toEqual(
       "?fv-dependencies=underscore&fv-dependencies=another&fv-keywords=node&q=node&size=20&sort-field=name&sort-direction=asc"
@@ -80,31 +111,7 @@ describe("#pushStateToURL", () => {
 
   it("will update the url with range filter types", () => {
     const manager = createManager();
-    manager.pushStateToURL({
-      filters: [
-        {
-          date: [
-            {
-              from: 12,
-              to: 4000
-            },
-            {
-              to: 4000
-            }
-          ]
-        },
-        {
-          cost: [
-            {
-              from: 50
-            }
-          ]
-        },
-        {
-          keywords: "node"
-        }
-      ]
-    });
+    manager.pushStateToURL(parameterStateWithRangeFilters);
     const queryString = manager.history.push.mock.calls[0][0].search;
     expect(queryString).toEqual(
       "?fr-date=12_4000&fr-date=_4000&fr-cost=50_&fv-keywords=node"
@@ -113,20 +120,94 @@ describe("#pushStateToURL", () => {
 });
 
 describe("#onURLStateChange", () => {
+  let manager;
+
+  function setup() {
+    manager = createManager();
+  }
+
+  function pushStateToURL(state) {
+    manager.pushStateToURL(state);
+    return manager.history.push.mock.calls[0][0].search;
+  }
+
+  function simulateBrowserHistoryEvent(newUrl) {
+    // Since we're not in a real browser, we simulate the change event that
+    // would have ocurred.
+    manager.history.listen.mock.calls[0][0]({
+      search: newUrl
+    });
+  }
+
+  function subject(onURLStateChangeHandler, newUrl) {
+    // Set our handler, which we be called any time url state changes
+    manager.onURLStateChange(onURLStateChangeHandler);
+
+    simulateBrowserHistoryEvent(newUrl);
+  }
+
   it("will call provided callback with updated state when url changes", () => {
-    const manager = createManager();
+    setup();
+    let newState;
+
+    // Provide url state change handler
+    subject(state => {
+      newState = state;
+    }, basicParameterStateAsUrl);
+
+    // Verify it is called when url state changes
+    expect(newState).toEqual(basicParameterState);
+  });
+
+  /*
+    If we triggered the callback every time we pushed state to the url,
+    we would have an infinite loop.
+
+    pushStateToURL -> UpdateUrl -> HistoryEvent -> Handler -> pushstateToURL
+
+    Instead, we just want:
+    pushStateToURL -> UpdateUrl -> HistoryEvent -> Handler
 
+  */
+  it("will not trigger callback as a result of pushStateToURL", () => {
+    setup();
+
+    // Push state to the url
+    // newUrl is the url that the browser changed to
+    const newUrl = pushStateToURL(basicParameterState);
     let newState;
-    manager.onURLStateChange(state => {
+
+    // Provide url state change handler
+    subject(state => {
       newState = state;
-    });
+    }, newUrl);
 
-    // Simulate state change
-    manager.history.listen.mock.calls[0][0]({
-      search:
-        "?fv-dependencies=underscore&fv-keywords=node&q=node&size=20&sort-direction=asc&sort-field=name"
-    });
+    // Verify it is called when url state changes
+    expect(newState).toBe(undefined);
+  });
+
+  it("will trigger callback if navigating back to a url with browser buttons", () => {
+    setup();
+
+    // Push state to the url
+    // newUrl is the url that the browser changed to
+    const newUrl = pushStateToURL(basicParameterState);
+    let newState;
+
+    // Provide url state change handler
+    subject(state => {
+      newState = state;
+    }, newUrl);
+
+    // Verify it is called when url state changes
+    expect(newState).toBe(undefined);
+
+    // Back button to empty state
+    simulateBrowserHistoryEvent("");
+    expect(newState).toEqual({}); // Was called, but empty state
 
-    expect(newState).toMatchSnapshot();
+    // Forward button to double back to the original "newUrl"
+    simulateBrowserHistoryEvent(newUrl);
+    expect(newState).toEqual(basicParameterState);
   });
 });

src/search-lib/__snapshots__/URLManager.test.js.snap

@@ -78,24 +78,3 @@ Object {
   "sortField": "name",
 }
 `;
-
-exports[`#onURLStateChange will call provided callback with updated state when url changes 1`] = `
-Object {
-  "filters": Array [
-    Object {
-      "dependencies": Array [
-        "underscore",
-      ],
-    },
-    Object {
-      "keywords": Array [
-        "node",
-      ],
-    },
-  ],
-  "resultsPerPage": 20,
-  "searchTerm": "node",
-  "sortDirection": "asc",
-  "sortField": "name",
-}
-`;