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

feat(viewer): use unsupported file type error message for .boxdicom #1437

Merged
merged 4 commits into from
Nov 19, 2021
+29 −15
Merged

feat(viewer): use unsupported file type error message for .boxdicom #1437

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d71d12
feat(viewer): use unsupported file type error message for boxdicom
JChan106 Nov 19, 2021
207fd78
feat(viewer): modify tests and simplify viewer logic
JChan106 Nov 19, 2021
4863d56
feat(viewer): remove unneeded conditional
JChan106 Nov 19, 2021
838b831
feat(viewer): update comment
JChan106 Nov 19, 2021
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
15 changes: 10 additions & 5 deletions src/lib/viewers/iframe/IFrameLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@ const VIEWERS = [
NAME: 'IFrame',
CONSTRUCTOR: IFrameViewer,
REP: ORIGINAL_REP_NAME,
EXT: ['boxnote', 'boxdicom'],
EXT: ['boxdicom'],
},
{
NAME: 'IFrame',
CONSTRUCTOR: IFrameViewer,
REP: ORIGINAL_REP_NAME,
EXT: ['boxnote'],
},
];

Expand All @@ -28,11 +34,10 @@ class IFrameLoader extends AssetLoader {
* @inheritdoc
*/
determineViewer(file, disabledViewers = [], viewerOptions = {}) {
const isDicomFile = file.extension === 'boxdicom';
const disableDicom = getProp(viewerOptions, 'IFrame.disableDicom');
// The IFrame viewer is disabled when the file is a Boxdicom file and the disableDicom viewer option is enabled
if (disableDicom && isDicomFile) {
disabledViewers.push('IFrame');
// Removes boxdicom as a supported extension when the file is a Boxdicom file and the disableDicom viewer option is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we even need to check if isDicomFile is true anymore? If disableDicom option is passed in, would it work if we just remove it from the this.viewers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right, it doesn't really matter about the current file, we should just remove the dicom viewer from the supported viewers if the disableDicom is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

May want to update the comment as well to be consistent with the code

if (disableDicom) {
this.viewers = this.viewers.filter(viewer => !viewer.EXT.includes('boxdicom'));
}

return super.determineViewer(file, disabledViewers);
Expand Down
29 changes: 19 additions & 10 deletions src/lib/viewers/iframe/__tests__/IFrameLoader-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,29 @@ describe('lib/viewers/iframe/IFrameLoader', () => {
},
});

test('should have the correct viewer', () => {
const iframeViewer = IFrameLoader.viewers[0];
expect(iframeViewer).toEqual({
NAME: 'IFrame',
CONSTRUCTOR: IFrameViewer,
REP: 'ORIGINAL',
EXT: ['boxnote', 'boxdicom'],
});
test('should have the correct viewers', () => {
const iframeViewers = IFrameLoader.viewers;
expect(iframeViewers).toEqual([
{
NAME: 'IFrame',
CONSTRUCTOR: IFrameViewer,
REP: 'ORIGINAL',
EXT: ['boxdicom'],
},
{
NAME: 'IFrame',
CONSTRUCTOR: IFrameViewer,
REP: 'ORIGINAL',
EXT: ['boxnote'],
},
]);
});

test.each`
disableDicom | fileType | viewerInstance
${false} | ${'boxdicom'} | ${IFrameLoader.viewers.find(v => v.EXT.includes('boxdicom'))}
${true} | ${'boxdicom'} | ${undefined}
${true} | ${'boxnote'} | ${IFrameLoader.viewers[0]}
${false} | ${'boxdicom'} | ${IFrameLoader.viewers[0]}
${true} | ${'boxnote'} | ${IFrameLoader.viewers.find(v => v.EXT.includes('boxnote'))}
`(
'should return correct result depending on the disableDicom viewer option and file type',
({ disableDicom, fileType, viewerInstance }) => {
Expand All @@ -43,6 +51,7 @@ describe('lib/viewers/iframe/IFrameLoader', () => {
};
const viewer = IFrameLoader.determineViewer(iframe.options.file, [], viewerOptions);
expect(viewer).toEqual(viewerInstance);
expect(IFrameLoader.getViewers().some(v => v.EXT.includes('boxdicom'))).toEqual(!disableDicom);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this assertion if we already have the one above it?

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 could, the assertion checks if the list of viewers is correct based on the disableDicom viewer option. While the above assertion checks if the viewer instance is correct.

},
);
});