download filename, error handling and cache busting (openedx#98)

* feat: handle download error and display them

* chore: update test environment for easier single file test

* feat: add cache bursting to the download

Author: leangseu-edx

jest.config.js

@@ -14,4 +14,5 @@ module.exports = createConfig('jest', {
     'src/postcss.config.js',
   ],
   testTimeout: 120000,
+  testEnvironment: 'jsdom',
 });

src/containers/ReviewModal/ReviewErrors/DownloadErrors.jsx

@@ -28,27 +28,44 @@ export class DownloadErrors extends React.Component {
     if (!this.props.isFailed) { return null; }
     return (
       <ReviewError
-        key="lockFailed"
+        key="downloadFailed"
         headingMessage={messages.downloadFailedHeading}
         actions={{
           cancel: { onClick: this.cancelAction, message: messages.dismiss },
           confirm: { onClick: this.props.downloadFiles, message: messages.retryDownload },
         }}
       >
         <FormattedMessage {...messages.downloadFailedContent} />
+        <br />
+        <FormattedMessage {...messages.failedFiles} />
+        <ul>
+          {this.props.error.files.map(filename => (
+            <li key={filename}>{filename}</li>
+          ))}
+        </ul>
       </ReviewError>
     );
   }
 }
+
+DownloadErrors.defaultProps = {
+  error: {
+    files: [],
+  },
+};
 DownloadErrors.propTypes = {
   // redux
   clearState: PropTypes.func.isRequired,
   isFailed: PropTypes.bool.isRequired,
+  error: PropTypes.shape({
+    files: PropTypes.arrayOf(PropTypes.string),
+  }),
   downloadFiles: PropTypes.func.isRequired,
 };
 
 export const mapStateToProps = (state) => ({
   isFailed: selectors.requests.isFailed(state, { requestKey: RequestKeys.downloadFiles }),
+  error: selectors.requests.error(state, { requestKey: RequestKeys.downloadFiles }),
 });
 
 export const mapDispatchToProps = {

src/containers/ReviewModal/ReviewErrors/DownloadErrors.test.jsx

@@ -14,7 +14,10 @@ let el;
 
 jest.mock('data/redux', () => ({
   selectors: {
-    requests: { isFailed: (...args) => ({ isFailed: args }) },
+    requests: {
+      isFailed: (...args) => ({ isFailed: args }),
+      error: (...args) => ({ error: args }),
+    },
   },
   actions: {
     requests: { clearRequest: jest.fn() },
@@ -28,6 +31,9 @@ jest.mock('./ReviewError', () => 'ReviewError');
 describe('DownloadErrors component', () => {
   const props = {
     isFailed: false,
+    error: {
+      files: [],
+    },
   };
   describe('component', () => {
     beforeEach(() => {
@@ -40,7 +46,12 @@ describe('DownloadErrors component', () => {
         el.instance().cancelAction = jest.fn().mockName('this.cancelAction');
       });
       test('failed: show error', () => {
-        el.setProps({ isFailed: true });
+        el.setProps({
+          isFailed: true,
+          error: {
+            files: ['file-1-failed.error', 'file-2.failed'],
+          },
+        });
         expect(el.instance().render()).toMatchSnapshot();
         expect(el.isEmptyRender()).toEqual(false);
       });
@@ -68,6 +79,10 @@ describe('DownloadErrors component', () => {
       const requestKey = RequestKeys.downloadFiles;
       expect(mapped.isFailed).toEqual(selectors.requests.isFailed(testState, { requestKey }));
     });
+    test('error loads from requests.error(downloadFiles)', () => {
+      const requestKey = RequestKeys.downloadFiles;
+      expect(mapped.error).toEqual(selectors.requests.error(testState, { requestKey }));
+    });
   });
   describe('mapDispatchToProps', () => {
     it('loads clearState from actions.requests.clearRequest', () => {

src/containers/ReviewModal/ReviewErrors/__snapshots__/DownloadErrors.test.jsx.snap

@@ -34,6 +34,20 @@ exports[`DownloadErrors component component snapshots failed: show error 1`] = `
     description="Failed download error content"
     id="ora-grading.ReviewModal.errorDownloadFailedContent"
   />
+  <br />
+  <FormattedMessage
+    defaultMessage="Failed files:"
+    description="List header for file download failure alert"
+    id="ora-grading.ReviewModal.errorDownloadFailedFiles"
+  />
+  <ul>
+    <li>
+      file-1-failed.error
+    </li>
+    <li>
+      file-2.failed
+    </li>
+  </ul>
 </ReviewError>
 `;
 

src/containers/ReviewModal/ReviewErrors/messages.js

@@ -82,6 +82,11 @@ const messages = defineMessages({
     defaultMessage: 'Retry download',
     description: 'Failed download retry button text',
   },
+  failedFiles: {
+    id: 'ora-grading.ReviewModal.errorDownloadFailedFiles',
+    defaultMessage: 'Failed files:',
+    description: 'List header for file download failure alert',
+  },
 });
 
 export default StrictDict(messages);

src/data/redux/thunkActions/download.js

@@ -1,15 +1,17 @@
 import * as zip from '@zip.js/zip.js';
 import FileSaver from 'file-saver';
 
-import { StrictDict } from 'utils';
 import { RequestKeys } from 'data/constants/requests';
 import { selectors } from 'data/redux';
+import { locationId } from 'data/constants/app';
+import { stringifyUrl } from 'data/services/lms/utils';
 
 import { networkRequest } from './requests';
 import * as module from './download';
 
-export const ERRORS = StrictDict({
-  fetchFailed: 'Fetch failed',
+export const DownloadException = (files) => ({
+  files,
+  name: 'DownloadException',
 });
 
 /**
@@ -21,22 +23,13 @@ export const genManifest = (files) => files.map(
   (file) => `Filename: ${file.name}\nDescription: ${file.description}\nSize: ${file.size}`,
 ).join('\n\n');
 
-/**
- * Returns the zip filename
- * @return {string} - zip download file name
- */
-export const zipFileName = () => {
-  const currentDate = new Date().getTime();
-  return `ora-files-download-${currentDate}.zip`;
-};
-
 /**
  * Zip the blob output of a set of files with a manifest file.
  * @param {obj[]} files - list of file entries with downloadUrl, name, and description
  * @param {blob[]} blobs - file content blobs
  * @return {Promise} - zip async process promise.
  */
-export const zipFiles = async (files, blobs) => {
+export const zipFiles = async (files, blobs, username) => {
   const zipWriter = new zip.ZipWriter(new zip.BlobWriter('application/zip'));
   await zipWriter.add('manifest.txt', new zip.TextReader(module.genManifest(files)));
 
@@ -50,39 +43,71 @@ export const zipFiles = async (files, blobs) => {
   }
 
   const zipFile = await zipWriter.close();
-  FileSaver.saveAs(zipFile, module.zipFileName());
+  const zipName = `${username}-${locationId}.zip`;
+  FileSaver.saveAs(zipFile, zipName);
 };
 
+/**
+ * generate url with additional timestamp for cache busting.
+ * This is implemented for fixing issue with the browser not
+ * allowing the user to fetch the same url as the image tag.
+ * @param {string} url
+ * @returns {string}
+ */
+export const getTimeStampUrl = (url) => stringifyUrl(url, {
+  ora_grading_download_timestamp: new Date().getTime(),
+});
+
 /**
  * Download a file and return its blob is successful, or null if not.
  * @param {obj} file - file entry with downloadUrl
- * @return {blob} - file blob or null
+ * @return {Promise} - file blob or null
  */
-export const downloadFile = (file) => fetch(file.downloadUrl).then(resp => (
-  resp.ok ? resp.blob() : null
-));
+export const downloadFile = (file) => fetch(
+  module.getTimeStampUrl(file.downloadUrl),
+).then((response) => {
+  if (!response.ok) {
+    // This is necessary because some of the error such as 404 does not throw.
+    // Due to that inconsistency, I have decide to share catch statement like this.
+    throw new Error(response.statusText);
+  }
+  return response.blob();
+});
 
 /**
  * Download blobs given file objects.  Returns a promise map.
  * @param {obj[]} files - list of file entries with downloadUrl, name, and description
  * @return {Promise[]} - Promise map of download attempts (null for failed fetches)
  */
-export const downloadBlobs = (files) => Promise.all(files.map(module.downloadFile));
+export const downloadBlobs = async (files) => {
+  const blobs = [];
+  const errors = [];
+
+  // eslint-disable-next-line no-restricted-syntax
+  for (const file of files) {
+    try {
+      // eslint-disable-next-line no-await-in-loop
+      blobs.push(await module.downloadFile(file));
+    } catch (error) {
+      errors.push(file.name);
+    }
+  }
+  if (errors.length) {
+    throw DownloadException(errors);
+  }
+  return blobs;
+};
 
 /**
  * Download all files for the selected submission as a zip file.
  * Throw error and do not download zip if any of the files fail to fetch.
  */
 export const downloadFiles = () => (dispatch, getState) => {
   const { files } = selectors.grading.selected.response(getState());
+  const username = selectors.grading.selected.username(getState());
   dispatch(networkRequest({
     requestKey: RequestKeys.downloadFiles,
-    promise: module.downloadBlobs(files).then(blobs => {
-      if (blobs.some(blob => blob === null)) {
-        throw Error(ERRORS.fetchFailed);
-      }
-      return module.zipFiles(files, blobs);
-    }),
+    promise: module.downloadBlobs(files).then(blobs => module.zipFiles(files, blobs, username)),
   }));
 };