Chore: Adding download reachability checks to Downloads, Document and Image Viewers #669

jeremypress · 2018-02-16T22:47:18Z

No description provided.

boxcla · 2018-02-16T22:47:21Z

Verified that @jeremypress has signed the CLA. Thanks for the pull request!

jeremypress · 2018-02-16T22:48:26Z

src/lib/downloadReachability.js

+ * @param {string} downloadHost - Download URL host
+ * @return {void}
+ */
+export function setDownloadHostNotificationShown(downloadHost) {


Need to clean up params for these various methods to make them all consistent

JustinHoldstock · 2018-02-16T22:54:48Z

src/lib/downloadReachability.js

+ * @return {boolean} - HTTP response
+ */
+export function isCustomDownloadHost(url) {
+    return url && !url.startsWith(DEFAULT_DOWNLOAD_HOST_PREFIX) && !!url.match(/^https:\/\/dl\d+\./);


Move the capture statement to a constant, then you can use it here and in places like replaceDownloadHostWithDefaultUrl()

JustinHoldstock · 2018-02-16T22:58:10Z

src/lib/viewers/BaseViewer.js

@@ -97,6 +107,9 @@ class BaseViewer extends EventEmitter {
    /** @property {boolean} - Whether viewer is being used on a touch device */
    hasTouch;

+    /** @property {boolean} - Has the viewer retried downloading the content */
+    haveRetried = false;


Could you be a little more descriptive with this property name? haveRetried is pretty vague

haveRetriedContentDownload?

JustinHoldstock · 2018-02-16T22:59:08Z

src/lib/viewers/BaseViewer.js

+    handleDownloadError(err, downloadURL) {
+        if (this.haveRetried) {
+            /* eslint-disable no-console */
+            console.error(err);


If you want to put console.error(err) inside of the code that emits preview_error events, we can throttle back the number of console statements that exist in our codebase! Up to you :)

I do! But we would need to remove them all in one to avoid double printing, so I'm leaving this for now and then we can go back and do a commit where we grab them all.

tonyjin · 2018-02-21T00:33:24Z

Add your diagram to the PR description please!

tonyjin · 2018-02-21T00:35:02Z

src/lib/Preview.js

@@ -745,6 +767,9 @@ class Preview extends EventEmitter {
            this.throttledMousemoveHandler
        );

+        // Set up the notification
+        this.ui.setupNotification();


Please work with @pramodsum to make sure this doesn't muck with annotations. I remember we've had a few issues with the location of setupNotification() before.

Were you able to resolve this @jeremypress @pramodsum?

Yep, it wasn't actually annotations related but CSS related. #583

I checked that the draw annotations notification was still good after my change.

jeremypress · 2018-02-21T00:45:42Z

Diagram:

tonyjin · 2018-02-21T00:51:31Z

src/lib/downloadReachability.js

+/**
+ * Replaces the hostname of a download URL with the default hostname, https://dl.
+ *
+ * @private


Not private if you're exporting it :) Don't really care if you add @public or not to all of these, but keep it consistent.

tonyjin · 2018-02-21T00:54:26Z

src/lib/viewers/image/ImageBaseViewer.js

-    errorHandler(err) {
-        /* eslint-disable no-console */
-        console.error(err);
-        /* eslint-enable no-console */


Did you mean to take these out? They're stripped out in the production build but are occasionally useful on dev.

I'm moving them all to the handleDownloadError call in base viewer so we only need to console.error once for any potential download error.

tonyjin · 2018-02-21T00:55:01Z

src/lib/viewers/image/ImageViewer.js

@@ -52,13 +53,14 @@ class ImageViewer extends ImageBaseViewer {

        const { representation, viewer } = this.options;
        const template = representation.content.url_template;
+        const downloadURL = this.createContentUrlWithAuthParams(template, viewer.ASSET);


downloadUrl* to keep consistent with other Url in the code

tonyjin · 2018-02-21T00:57:43Z

src/lib/Preview.js

+                setDownloadReachability(data.download_url).then(() => {
+                    if (isDownloadHostBlocked()) {
+                        // If download is unreachable, try again with default
+                        openUrlInsideIframe(defaultDownloadUrl);


Is there any opportunity for a double download here? e.g. can the first download succeed but then this reachability check fail?

I don't believe so. The only way the reachability check fails is if the network request itself fails. This is different from any bad status 500, 404 etc. which will be treated as "successes" because we get a valid HTTP status.

Unfortunately we have no way of checking the success of the download due to it coming from an iframe. @DanDeMicco mentioned other methods that we might want to move to that would give us more control over download status.

tonyjin · 2018-02-21T00:59:38Z

src/lib/downloadReachability.js

+    return (
+        downloadUrl && !downloadUrl.startsWith(DEFAULT_DOWNLOAD_HOST_PREFIX) && !!downloadUrl.match(DEFAULT_HOST_REGEX)
+    );
+}


Does your code work properly on dev with dl.jpress.inside-box.net and dl.app.jpress.inside-box.net? I don't think it's super important for the fallback logic to work on dev since there's only one download host, but please make sure we can't get stuck into some edge case.

Yes this works on dev since I only check the DL, but this will have to change due to your comment below.

tonyjin · 2018-02-21T01:04:11Z

src/lib/downloadReachability.js

+ * @return {string} - The updated download URL
+ */
+export function replaceDownloadHostWithDefault(downloadUrl) {
+    return downloadUrl.replace(DEFAULT_HOST_REGEX, DEFAULT_DOWNLOAD_HOST_PREFIX);


Some AWS or accelerated hosts have weird patterns like https://ec2apne1.boxcloud.com, I'm not sure your regex would capture that

womp, that is good information to know. I'll need to rethink my whole custom host/default host checking strategy. Good catch.

tonyjin · 2018-02-21T01:05:59Z

src/lib/downloadReachability.js

+ * @param {string} contentTemplate - Content download URL template
+ * @return {string|undefined} Should the notification be shown
+ */
+export function downloadNotificationToShow(contentTemplate) {


This function signature is a bit confusing to me. The JSDoc for returns implies it should return a {boolean} and JSDoc description implies this should return a notification ID of some kind.

This used to return a boolean, I'll update the return description.

JustinHoldstock · 2018-02-27T21:04:10Z

src/lib/events.js

@@ -53,3 +54,9 @@ export const LOAD_METRIC = {
    downloadResponseTime: 'download_response_time', // Time it took for TTFB when requesting a rep.
    fullDocumentLoadTime: 'full_document_load_time' // How long it took to load the document so it could be previewed.
 };
+
+// Events around download reachability
+export const DOWNLOAD_REACHABILITY_METRICS = {


JustinHoldstock · 2018-02-27T21:06:25Z

src/lib/viewers/BaseViewer.js

+     */
+    handleDownloadError(err, downloadURL) {
+        if (this.hasRetriedContentDownload) {
+            this.triggerError(err);


You may want to make sure that this error complies with our new PreviewError

JustinHoldstock · 2018-02-27T21:12:35Z

src/lib/viewers/image/ImageBaseViewer.js

@@ -82,7 +81,9 @@ class ImageBaseViewer extends BaseViewer {
                this.loaded = true;
                this.emit(VIEWER_EVENT.load);
            })
-            .catch(this.errorHandler);
+            .catch(() => {
+                // No-op, this prmise should always resolve


Seems funky to not track an error if it occurs. Can we remove this, and allow for an uncaught error, then (if it's possible for this to error)?

DanDeMicco · 2018-02-27T22:01:19Z

src/lib/__tests__/downloadReachability-test.js

+
+    describe('setDownloadHostFallback()', () => {
+        it('should set the download host fallback key to be true', () => {
+            expect(sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY) === 'true').to.be.false;


This looks a bit weird

expect(sessionStorage.getItem(DOWNLOAD_HOST_FALLBACK_KEY)).to.equal('true')

jeremypress · 2018-02-27T00:16:03Z

src/lib/Preview.js

@@ -163,6 +169,7 @@ class Preview extends EventEmitter {
        this.handleFileInfoResponse = this.handleFileInfoResponse.bind(this);
        this.handleFetchError = this.handleFetchError.bind(this);
        this.handleViewerEvents = this.handleViewerEvents.bind(this);
+        this.handleViewerMetrics = this.handleViewerMetrics.bind(this);


Forgot this in my last PR :)

jeremypress · 2018-02-28T00:24:07Z

src/lib/Preview.js

@@ -745,6 +767,9 @@ class Preview extends EventEmitter {
            this.throttledMousemoveHandler
        );

+        // Set up the notification
+        this.ui.setupNotification();


Yep, it wasn't actually annotations related but CSS related. #583

I checked that the draw annotations notification was still good after my change.

jeremypress commented Feb 16, 2018

View reviewed changes

JustinHoldstock reviewed Feb 16, 2018

View reviewed changes

tonyjin reviewed Feb 21, 2018

View reviewed changes

JustinHoldstock reviewed Feb 27, 2018

View reviewed changes

DanDeMicco approved these changes Feb 27, 2018

View reviewed changes

jeremypress commented Mar 1, 2018

View reviewed changes

Jeremy Press added 5 commits March 2, 2018 09:11

Chore: Adding download reachability checks to Document and Image Viewers

a3a26e7

Chore: Responding to comments

85f16c1

Chore: Refactor and adding logging

fa1f2cb

Chore: Responding to comments

02ad6d6

Chore: Adding startsWith polyfill

f46a1f9

jeremypress merged commit 6468d63 into box:master Mar 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Adding download reachability checks to Downloads, Document and Image Viewers #669

Chore: Adding download reachability checks to Downloads, Document and Image Viewers #669

jeremypress commented Feb 16, 2018

boxcla commented Feb 16, 2018

jeremypress Feb 16, 2018

JustinHoldstock Feb 16, 2018

JustinHoldstock Feb 16, 2018

jeremypress Feb 20, 2018

JustinHoldstock Feb 16, 2018

jeremypress Feb 20, 2018

tonyjin commented Feb 21, 2018

tonyjin Feb 21, 2018

tonyjin Feb 28, 2018

jeremypress Feb 28, 2018

jeremypress commented Feb 21, 2018

tonyjin Feb 21, 2018 •

edited

Loading

tonyjin Feb 21, 2018 •

edited

Loading

jeremypress Feb 21, 2018 •

edited

Loading

tonyjin Feb 21, 2018

tonyjin Feb 21, 2018

jeremypress Feb 21, 2018

jeremypress Feb 22, 2018

tonyjin Feb 21, 2018

jeremypress Feb 21, 2018

tonyjin Feb 21, 2018

jeremypress Feb 21, 2018 •

edited

Loading

tonyjin Feb 21, 2018

jeremypress Feb 21, 2018

JustinHoldstock Feb 27, 2018

JustinHoldstock Feb 27, 2018

JustinHoldstock Feb 27, 2018

DanDeMicco Feb 27, 2018

jeremypress Feb 27, 2018

jeremypress Feb 28, 2018

Chore: Adding download reachability checks to Downloads, Document and Image Viewers #669

Chore: Adding download reachability checks to Downloads, Document and Image Viewers #669

Conversation

jeremypress commented Feb 16, 2018

boxcla commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyjin commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremypress commented Feb 21, 2018

tonyjin Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

tonyjin Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

jeremypress Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremypress Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyjin Feb 21, 2018 •

edited

Loading

tonyjin Feb 21, 2018 •

edited

Loading

jeremypress Feb 21, 2018 •

edited

Loading

jeremypress Feb 21, 2018 •

edited

Loading