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.

Sign up for GitHub

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

Fix: Fix shared links previewed in other subdomains #173

Merged
merged 3 commits into from
Jun 20, 2017
Merged

Fix: Fix shared links previewed in other subdomains #173

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e181997
Fix: Fix shared links previewed in other subdomains
Jun 16, 2017
7c7f043
Fix: Responding to comments
Jun 16, 2017
6a1762d
Merge branch 'master' into bug-fixes
Jun 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/lib/viewers/office/OfficeViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,25 +184,28 @@ const MESSAGE_HOST_READY = 'Host_PostmessageReady';
*/
setupRunmodeURL(appHost, fileId, sharedLink) {
// @TODO(jpress): Combine with setupWopiSrc Logic when removing the platform fork
let src = `${appHost}/integrations/officeonline/openExcelOnlinePreviewer`;
let runmodeSrc = '/integrations/officeonline/openExcelOnlinePreviewer';
Copy link

Choose a reason for hiding this comment

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

"route" might be a better name for this, just because a runmode isn't actually included in the src URL we build

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed - we also shouldn't have Box-specific language in our code if possible.


const domain = document.createElement('a');
Copy link

Choose a reason for hiding this comment

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

Just checking - will this cause issues on IE like that bug you found a few weeks ago?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. Is there a reason why we created an tag an didn't just use window.location to begin with?

Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a tag lets us leverage the browser when parsing out different parts of the domain

domain.href = sharedLink;

if (sharedLink) {
// Use the domain in case previewer has a different subdomain
runmodeSrc = `${domain.origin}${runmodeSrc}`;
// Find the shared or vanity name
const sharedName = sharedLink.split('/s/')[1];
if (sharedName) {
src = `${src}?s=${sharedName}&fileId=${fileId}`;
runmodeSrc = `${runmodeSrc}?s=${sharedName}&fileId=${fileId}`;
} else {
const tempAnchor = document.createElement('a');
tempAnchor.href = sharedLink;
const vanitySubdomain = tempAnchor.hostname.split('.')[0];
const vanityName = tempAnchor.href.split('/v/')[1];
src = `${src}?v=${vanityName}&vanity_subdomain=${vanitySubdomain}&fileId=${fileId}`;
const vanitySubdomain = domain.hostname.split('.')[0];
const vanityName = domain.href.split('/v/')[1];
runmodeSrc = `${runmodeSrc}?v=${vanityName}&vanity_subdomain=${vanitySubdomain}&fileId=${fileId}`;
}
} else {
src = `${src}?fileId=${fileId}`;
runmodeSrc = `${appHost}${runmodeSrc}?fileId=${fileId}`;
}

return src;
return runmodeSrc;
}

/**
Expand Down
18 changes: 10 additions & 8 deletions src/lib/viewers/office/__tests__/OfficeViewer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ describe('lib/viewers/office/OfficeViewer', () => {
location: {
locale: 'en-US'
},
appHost: 'app.box.com',
apiHost: 'app.box.com',
appHost: 'https://app.box.com',
apiHost: 'https://app.box.com',
token: 'token'
});
stubs = {
Expand Down Expand Up @@ -185,7 +185,7 @@ describe('lib/viewers/office/OfficeViewer', () => {
office.options.file.id,
office.options.sharedLink
);
expect(src).to.equal('app.box.com/integrations/officeonline/openExcelOnlinePreviewer?fileId=123');
expect(src).to.equal('https://app.box.com/integrations/officeonline/openExcelOnlinePreviewer?fileId=123');
});

it('should load a xlsx file and set the shared name in src url on load event when the file is a shared link', () => {
Expand All @@ -195,7 +195,9 @@ describe('lib/viewers/office/OfficeViewer', () => {
office.options.file.id,
office.options.sharedLink
);
expect(src).to.equal('app.box.com/integrations/officeonline/openExcelOnlinePreviewer?s=abcd&fileId=123');
expect(src).to.equal(
'https://app.box.com/integrations/officeonline/openExcelOnlinePreviewer?s=abcd&fileId=123'
);
});

it('should load a xlsx file and set the vanity name in src url on load event when the file is a vanity url without a subdomain', () => {
Expand All @@ -206,7 +208,7 @@ describe('lib/viewers/office/OfficeViewer', () => {
office.options.sharedLink
);
expect(src).to.equal(
'app.box.com/integrations/officeonline/openExcelOnlinePreviewer?v=test&vanity_subdomain=app&fileId=123'
'https://app.box.com/integrations/officeonline/openExcelOnlinePreviewer?v=test&vanity_subdomain=app&fileId=123'
);
});

Expand All @@ -218,21 +220,21 @@ describe('lib/viewers/office/OfficeViewer', () => {
office.options.sharedLink
);
expect(src).to.equal(
'app.box.com/integrations/officeonline/openExcelOnlinePreviewer?v=test&vanity_subdomain=cloud&fileId=123'
'https://cloud.app.box.com/integrations/officeonline/openExcelOnlinePreviewer?v=test&vanity_subdomain=cloud&fileId=123'
);
});
});

describe('setupWOPISrc()', () => {
it('should append the file ID if there is no shared link', () => {
const src = office.setupWOPISrc(office.options.apiHost, office.options.file.id, office.options.sharedLink);
expect(src).to.equal('app.box.com/wopi/files/123');
expect(src).to.equal('https://app.box.com/wopi/files/123');
});

it('should append the shared name and file ID if there is a shared link', () => {
office.options.sharedLink = 'https://app.box.com/s/abcd';
Copy link

Choose a reason for hiding this comment

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

Can you add a few tests for when the appHost name and the shared domain don't match? i.e. appHost = "app" and shared domain = "cloud", appHost = "ibm" and shared domain = "cloud", appHost = "cloud" and shared domain = "app"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do!

const src = office.setupWOPISrc(office.options.apiHost, office.options.file.id, office.options.sharedLink);
expect(src).to.equal('app.box.com/wopi/files/s_abcd_f_123');
expect(src).to.equal('https://app.box.com/wopi/files/s_abcd_f_123');
});

it('should not append the shared name if there is a vanity link', () => {
Expand Down