Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New: Log preview_error message from Preview #619

Closed
87 changes: 81 additions & 6 deletions src/lib/Preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ import {
getHeaders,
findScriptLocation,
appendQueryParams,
replacePlaceholders
replacePlaceholders,
stripAuthFromString
} from './util';
import {
getURL,
Expand Down Expand Up @@ -48,8 +49,9 @@ import {
X_REP_HINT_VIDEO_MP4,
FILE_OPTION_FILE_VERSION_ID
} from './constants';
import { VIEWER_EVENT } from './events';
import { VIEWER_EVENT, ERROR_CODE } from './events';
import './Preview.scss';
import { getClientLogDetails, createPreviewError, getISOTime } from './logUtils';

const DEFAULT_DISABLED_VIEWERS = ['Office']; // viewers disabled by default
const PREFETCH_COUNT = 4; // number of files to prefetch
Expand Down Expand Up @@ -330,9 +332,13 @@ class Preview extends EventEmitter {
if (checkFileValid(file)) {
cacheFile(this.cache, file);
} else {
const message = '[Preview SDK] Tried to cache invalid file';
/* eslint-disable no-console */
console.error('[Preview SDK] Tried to cache invalid file: ', file);
console.error(`${message}: `, file);
/* eslint-enable no-console */

const err = createPreviewError(ERROR_CODE.invalidCacheAttempt, message, file);
this.logPreviewError(err);
}
});
}
Expand Down Expand Up @@ -536,6 +542,10 @@ class Preview extends EventEmitter {
/* eslint-disable no-console */
console.error(`Error prefetching file ID ${fileId} - ${err}`);
/* eslint-enable no-console */

const error = createPreviewError(ERROR_CODE.prefetchFile, null, err);
this.logPreviewError(error);

return;
}

Expand Down Expand Up @@ -1051,6 +1061,10 @@ class Preview extends EventEmitter {
case VIEWER_EVENT.mediaEndAutoplay:
this.navigateRight();
break;
case VIEWER_EVENT.error:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this case entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot. This prevents a double error message from being emitted, which would double up on firing off preview_error events.

// Do nothing since 'error' event was already caught, and will be emitted
// as a 'preview_error' event
break;
default:
// This includes 'notification', 'preload' and others
this.emit(data.event, data.data);
Expand Down Expand Up @@ -1195,12 +1209,15 @@ class Preview extends EventEmitter {

// Check if hit the retry limit
if (this.retryCount > RETRY_COUNT) {
let errorCode = ERROR_CODE.retriesExceeded;
let errorMessage = __('error_refresh');
if (err.response && err.response.status === 429) {
errorCode = ERROR_CODE.rateLimit;
errorMessage = __('error_rate_limit');
}

this.triggerError(new Error(errorMessage));
const error = createPreviewError(errorCode, errorMessage, this.file.id);
this.triggerError(error);
return;
}

Expand Down Expand Up @@ -1245,6 +1262,9 @@ class Preview extends EventEmitter {
* @return {void}
*/
triggerError(err) {
// Always log preview errors
this.logPreviewError(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a chance of double logging here. Above you bound to the VIEWER_EVENT.error event, which you then call to triggerError. On line 966 of your patch (this is old code), we also bind to the error event coming from the viewer and call triggerError. This would end up calling logPreviewError twice, one from handleViewerEvents and one from triggerError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aww hell. So, as it is, node requires that you bind to the 'error' event (listener #1), then we listen for it in handleViewerEvents (listener #2). The huge issue here is when #1 is bound can be after any setup errors occur.

Does it make sense that we begin listening for errors immediately on viewer creation, and not in handleViewerEvents?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've confirmed that handleViewerEvents() only handles the generic viewerevent, therefore our initial thoughts are invalid. Merging


// If preview is closed don't do anything
if (!this.open) {
return;
Expand All @@ -1269,6 +1289,50 @@ class Preview extends EventEmitter {
this.viewer.load(err);
}

/**
* Create a generic log Object.
*
* @private
* @return {Object} Log details for viewer session and current file.
*/
createLog() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const file = this.file || {};
const log = {
timestamp: getISOTime(),
file_id: getProp(file, 'id', ''),
file_version_id: getProp(file, 'file_version.id', ''),
content_type: getProp(this.viewer, 'options.viewer.NAME', ''),
extension: file.extension || '',
locale: getProp(this.location, 'locale', ''),
...getClientLogDetails()
};

return log;
}

/**
* Message, to any listeners of Preview, that an error has occurred.
*
* @private
* @param {Error} error - The error that occurred.
* @return {void}
*/
logPreviewError(error) {
const err = error;
// If we haven't supplied a code, then it was thrown by the browser
err.code = error.code || ERROR_CODE.browserError;
// Make sure to strip auth, if it's a string.
err.message = typeof error.message === 'string' ? stripAuthFromString(error.message) : error.message;
err.displayMessage = typeof error.displayMessage === 'string' ? stripAuthFromString(error.displayMessage) : '';

const errorLog = {
error: err,
...this.createLog()
};

this.emit('preview_error', errorLog);
}

/**
* Builds a list of required XHR headers.
*
Expand Down Expand Up @@ -1344,16 +1408,27 @@ class Preview extends EventEmitter {
});
})
.catch((err) => {
const message = `Error prefetching file ID ${fileId} - ${err}`;
/* eslint-disable no-console */
console.error(`Error prefetching file ID ${fileId} - ${err}`);
console.error(message);
/* eslint-enable no-console */

const error = createPreviewError(ERROR_CODE.prefetchFile, message, {
fileId,
error: err
});
this.logPreviewError(error);
});
});
})
.catch(() => {
const message = 'Error prefetching files';
/* eslint-disable no-console */
console.error('Error prefetching files');
console.error(message);
/* eslint-enable no-console */

const error = createPreviewError(ERROR_CODE, message, filesToPrefetch);
this.logPreviewError(error);
});
}

Expand Down
18 changes: 11 additions & 7 deletions src/lib/RepStatus.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import EventEmitter from 'events';
import { get, appendAuthParams } from './util';
import { STATUS_SUCCESS, STATUS_VIEWABLE } from './constants';
import { createPreviewError } from './logUtils';

const STATUS_UPDATE_INTERVAL_MS = 2000;

Expand Down Expand Up @@ -106,26 +107,29 @@ class RepStatus extends EventEmitter {
*/
handleResponse() {
const status = RepStatus.getStatus(this.representation);
let errorCode;
const errCode = RepStatus.getErrorCode(this.representation);
let errorMessage;
let error;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section changed to allow readable error codes with translated display messages


switch (status) {
case 'error':
switch (RepStatus.getErrorCode(this.representation)) {
switch (errCode) {
case ERROR_PASSWORD_PROTECTED:
errorCode = __('error_password_protected');
errorMessage = __('error_password_protected');
break;
case ERROR_TRY_AGAIN_LATER:
errorCode = __('error_try_again_later');
errorMessage = __('error_try_again_later');
break;
case ERROR_UNSUPPORTED_FORMAT:
errorCode = __('error_bad_file');
errorMessage = __('error_bad_file');
break;
default:
errorCode = __('error_refresh');
errorMessage = __('error_refresh');
break;
}

this.reject(errorCode);
error = createPreviewError(errCode, errorMessage, this.representation);
this.reject(error);
break;

case STATUS_SUCCESS:
Expand Down
113 changes: 110 additions & 3 deletions src/lib/__tests__/Preview-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import Browser from '../Browser';
import * as file from '../file';
import * as util from '../util';
import { API_HOST, CLASS_NAVIGATION_VISIBILITY } from '../constants';
import { VIEWER_EVENT } from '../events';
import { VIEWER_EVENT, ERROR_CODE } from '../events';
import { createPreviewError } from '../logUtils';

const tokens = require('../tokens');

Expand Down Expand Up @@ -337,6 +338,7 @@ describe('lib/Preview', () => {
stubs.checkFileValid = sandbox.stub(file, 'checkFileValid');
stubs.cacheFile = sandbox.stub(file, 'cacheFile');
stubs.error = sandbox.stub(console, 'error');
stubs.logPreviewError = sandbox.stub(preview, 'logPreviewError');
});

it('should format the metadata into an array', () => {
Expand Down Expand Up @@ -374,6 +376,7 @@ describe('lib/Preview', () => {
preview.updateFileCache(files);
expect(stubs.cacheFile).calledOnce;
expect(stubs.error).calledOnce;
expect(stubs.logPreviewError).calledOnce;
});

it('should not cache a file if it is watermarked', () => {
Expand Down Expand Up @@ -1913,17 +1916,19 @@ describe('lib/Preview', () => {
stubs.checkPermission = sandbox.stub(file, 'checkPermission');
stubs.showDownloadButton = sandbox.stub(preview.ui, 'showDownloadButton');
stubs.emit = sandbox.stub(preview, 'emit');
stubs.logPreviewError = sandbox.stub(preview, 'logPreviewError');
stubs.attachViewerListeners = sandbox.stub(preview, 'attachViewerListeners');

preview.open = true;
});

it('should do nothing if the preview is closed', () => {
it('should only log an error if the preview is closed', () => {
preview.open = false;

preview.triggerError();
preview.triggerError(new Error('fail'));
expect(stubs.uncacheFile).to.not.be.called;
expect(stubs.destroy).to.not.be.called;
expect(stubs.logPreviewError).to.be.called;
});

it('should prevent any other viewers from loading, clear the cache, complete postload tasks, and destroy anything still visible', () => {
Expand All @@ -1943,6 +1948,108 @@ describe('lib/Preview', () => {
});
});

describe('createLog()', () => {
it('should create a log object containing correct file info properties', () => {
const id = '12345';
preview.file = {
id
};

const log = preview.createLog();
expect(log.timestamp).to.exist;
expect(log.file_id).to.equal(id);
expect(log.file_version_id).to.exist;
expect(log.content_type).to.exsit;
expect(log.extension).to.exist;
expect(log.locale).to.exist;
});

it('should use empty string for file_id, if no file', () => {
preview.file = undefined;
const log = preview.createLog();

expect(log.file_id).to.equal('');
});

it('should use empty string for file_version_id, if no file version', () => {
preview.file = {
id: '12345',
file_version: undefined
};
const log = preview.createLog();

expect(log.file_version_id).to.equal('');
});
});

describe('logPreviewError', () => {
it('should emit a "preview_error" message', (done) => {
preview.on('preview_error', () => {
done();
});

preview.logPreviewError({});
});

it('should emit a "preview_error" message with an object describing the error', (done) => {
const code = 'an_error';
const displayMessage = 'Oh no!';
const message = { fileId: '12345' };
const error = createPreviewError(code, displayMessage, message);

preview.on('preview_error', (details) => {
expect(details.error).to.deep.equal(error);
done();
});

preview.logPreviewError(error);
});

it('should emit a "preview_error" message with info about the preview session', (done) => {
const fileId = '1234';
const fileVersionId = '999';

preview.file = {
id: fileId,
file_version: {
id: fileVersionId
}
};

preview.on('preview_error', (details) => {
expect(details.file_id).to.equal(fileId);
expect(details.file_version_id).to.equal(fileVersionId);
done();
});

preview.logPreviewError({});
});

it('should use a default browser error code if none is present', (done) => {
preview.on('preview_error', (details) => {
expect(details.error.code).to.equal(ERROR_CODE.browserError);
done();
});

preview.logPreviewError({});
});

it('should strip any auth from the message and displayMessage if it is present', (done) => {
const message = 'A message';
const displayMessage = 'A display message';
const auth = 'access_token="1234abcd"';
const filtered = 'access_token=[FILTERED]';
preview.on('preview_error', (details) => {
expect(details.error.message).to.equal(`${message}?${filtered}`)
expect(details.error.displayMessage).to.equal(`${displayMessage}?${filtered}`)
done();
});

const error = createPreviewError('bad_thing', `${displayMessage}?${auth}`, `${message}?${auth}`);
preview.logPreviewError(error);
});
});

describe('getRequestHeaders()', () => {
beforeEach(() => {
stubs.canPlayDash = sandbox.stub(Browser, 'canPlayDash').returns(false);
Expand Down
Loading