From 906d47abaad2b7c2a3ee2d2cc444fde676395299 Mon Sep 17 00:00:00 2001
From: Tony Jin <github@tonyjin.com>
Date: Tue, 19 Jun 2018 16:18:31 -0700
Subject: [PATCH] Fix: Retry logic (#808)

- Remove incorrect short-circuit-on-retry logic. This short circuit was being called any time you called preview on the same file twice and subsequent loads would always fail because the short circuit happened before an access token was assigned
- Set up fake clock in beforeEach of tests for handleFetchError() - not resetting the timer was causing weird issues
---
 src/lib/Preview.js                | 19 ++++++++-----------
 src/lib/__tests__/Preview-test.js | 17 +++++++----------
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/src/lib/Preview.js b/src/lib/Preview.js
index 4cb1bebfe..98c8fad15 100644
--- a/src/lib/Preview.js
+++ b/src/lib/Preview.js
@@ -754,9 +754,7 @@ class Preview extends EventEmitter {
             );
         }
 
-        // Retry up to RETRY_COUNT if we are reloading same file. If load is called during a preview when file version
-        // ID has been specified, count as a retry only if the current file verison ID matches that specified file
-        // version ID
+        // If file version ID is specified, increment retry count if it matches current file version ID
         if (fileVersionId) {
             if (fileVersionId === currentFileVersionId) {
                 this.retryCount += 1;
@@ -764,13 +762,18 @@ class Preview extends EventEmitter {
                 this.retryCount = 0;
             }
 
-            // Otherwise, count this as a retry if the file ID we are trying to load matches the current file ID
+            // Otherwise, increment retry count if file ID to load matches current file ID
         } else if (this.file.id === currentFileId) {
             this.retryCount += 1;
+
+            // Otherwise, reset retry count
         } else {
             this.retryCount = 0;
         }
 
+        // @TODO: This may not be the best way to detect if we are offline. Make sure this works well if we decided to
+        // combine Box Elements + Preview. This could potentially break if we have Box Elements fetch the file object
+        // and pass the well-formed file object directly to the preview library to render.
         const isPreviewOffline = typeof fileIdOrFile === 'object' && this.options.skipServerUpdate;
         if (isPreviewOffline) {
             this.handleTokenResponse({});
@@ -790,12 +793,6 @@ class Preview extends EventEmitter {
      * @return {void}
      */
     handleTokenResponse(tokenMap) {
-        // If this is a retry, short-circuit and load from server
-        if (this.retryCount > 0) {
-            this.loadFromServer();
-            return;
-        }
-
         // Set the authorization token
         this.options.token = tokenMap[this.file.id];
 
@@ -1379,7 +1376,7 @@ class Preview extends EventEmitter {
         // Nuke the cache
         uncacheFile(this.cache, this.file);
 
-        // Check if hit the retry limit
+        // Check if we hit the retry limit for fetching file info
         if (this.retryCount > RETRY_COUNT) {
             let errorCode = ERROR_CODE.EXCEEDED_RETRY_LIMIT;
             let errorMessage = __('error_refresh');
diff --git a/src/lib/__tests__/Preview-test.js b/src/lib/__tests__/Preview-test.js
index e7f5d970d..b5f865408 100644
--- a/src/lib/__tests__/Preview-test.js
+++ b/src/lib/__tests__/Preview-test.js
@@ -1092,13 +1092,6 @@ describe('lib/Preview', () => {
             stubs.cacheFile = sandbox.stub(file, 'cacheFile');
         });
 
-        it('should short circuit and load from server if it is a retry', () => {
-            preview.retryCount = 1;
-
-            preview.handleTokenResponse({});
-            expect(stubs.loadFromServer).to.be.called;
-        });
-
         it('should set the token option', () => {
             preview.retryCount = 0;
             const TOKEN = 'bar';
@@ -1999,7 +1992,10 @@ describe('lib/Preview', () => {
     });
 
     describe('handleFetchError()', () => {
+        let clock;
+
         beforeEach(() => {
+            clock = sandbox.useFakeTimers();
             stubs.uncacheFile = sandbox.stub(file, 'uncacheFile');
             stubs.triggerError = sandbox.stub(preview, 'triggerError');
             stubs.load = sandbox.stub(preview, 'load');
@@ -2010,6 +2006,10 @@ describe('lib/Preview', () => {
             };
         });
 
+        afterEach(() => {
+            clock.restore();
+        });
+
         it('should do nothing if the preview is closed', () => {
             preview.file = {
                 id: '0'
@@ -2061,7 +2061,6 @@ describe('lib/Preview', () => {
             preview.file = {
                 id: '0'
             };
-            const clock = sinon.useFakeTimers();
             preview.open = true;
             preview.retryCount = 1;
             preview.file.id = 1;
@@ -2077,7 +2076,6 @@ describe('lib/Preview', () => {
             preview.file = {
                 id: '0'
             };
-            const clock = sinon.useFakeTimers();
             preview.open = true;
             preview.retryCount = 3;
 
@@ -2097,7 +2095,6 @@ describe('lib/Preview', () => {
                     .withArgs('Retry-After')
                     .returns(5)
             };
-            const clock = sinon.useFakeTimers();
             preview.open = true;
             preview.retryCount = 1;