feat: add fetch submission files (openedx#110)

chore: remove cache busting

Author: leangseu-edx

src/data/redux/thunkActions/download.js

@@ -4,7 +4,7 @@ import FileSaver from 'file-saver';
 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 api from 'data/services/lms/api';
 
 import { networkRequest } from './requests';
 import * as module from './download';
@@ -14,6 +14,10 @@ export const DownloadException = (files) => ({
   name: 'DownloadException',
 });
 
+export const FetchSubmissionFilesException = () => ({
+  name: 'FetchSubmissionFilesException',
+});
+
 /**
  * Generate a manifest file content based on files object
  * @param {obj[]} files - list of file entries with downloadUrl, name, description, and size
@@ -47,24 +51,13 @@ export const zipFiles = async (files, blobs, username) => {
   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 {Promise} - file blob or null
  */
 export const downloadFile = (file) => fetch(
-  module.getTimeStampUrl(file.downloadUrl),
+  file.downloadUrl,
 ).then((response) => {
   if (!response.ok) {
     // This is necessary because some of the error such as 404 does not throw.
@@ -95,19 +88,34 @@ export const downloadBlobs = async (files) => {
   if (errors.length) {
     throw DownloadException(errors);
   }
-  return blobs;
+  return ({ blobs, files });
+};
+
+/**
+ * @param {string} submissionUUID
+ * @returns Promise
+ */
+export const getSubmissionFiles = async (submissionUUID) => {
+  try {
+    const { files } = await api.fetchSubmissionFiles(submissionUUID);
+    return files;
+  } catch {
+    throw FetchSubmissionFilesException();
+  }
 };
 
 /**
  * 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 submissionUUID = selectors.grading.selected.submissionUUID(getState());
   const username = selectors.grading.selected.username(getState());
   dispatch(networkRequest({
     requestKey: RequestKeys.downloadFiles,
-    promise: module.downloadBlobs(files).then(blobs => module.zipFiles(files, blobs, username)),
+    promise: module.getSubmissionFiles(submissionUUID)
+      .then(module.downloadBlobs)
+      .then(({ blobs, files }) => module.zipFiles(files, blobs, username)),
   }));
 };
 

src/data/redux/thunkActions/download.test.js

@@ -3,6 +3,7 @@ import FileSaver from 'file-saver';
 
 import { selectors } from 'data/redux';
 import { RequestKeys } from 'data/constants/requests';
+import api from 'data/services/lms/api';
 import * as download from './download';
 
 const mockBlobWriter = jest.fn().mockName('BlobWriter');
@@ -38,7 +39,7 @@ jest.mock('data/redux', () => ({
   selectors: {
     grading: {
       selected: {
-        response: jest.fn(),
+        submissionUUID: jest.fn(),
         username: jest.fn(),
       },
     },
@@ -57,13 +58,13 @@ describe('download thunkActions', () => {
   });
   const files = [mockFile('test-file1.jpg'), mockFile('test-file2.pdf')];
   const blobs = ['blob1', 'blob2'];
-  const response = { files };
+  const submissionUUID = 'submission-uuid';
   const username = 'student-name';
   let dispatch;
   const getState = () => testState;
   describe('genManifest', () => {
     test('returns a list of strings with filename and description for each file', () => {
-      expect(download.genManifest(response.files)).toEqual(
+      expect(download.genManifest(files)).toEqual(
         [
           `Filename: ${files[0].name}\nDescription: ${files[0].description}\nSize: ${files[0].size}`,
           `Filename: ${files[1].name}\nDescription: ${files[1].description}\nSize: ${files[0].size}`,
@@ -86,43 +87,25 @@ describe('download thunkActions', () => {
       });
     });
   });
-
-  describe('getTimeStampUrl', () => {
-    it('generate different url every milisecond for cache busting', () => {
-      const testUrl = 'test/url?param1=true';
-      const firstGen = download.getTimeStampUrl(testUrl);
-      // fast forward for 1 milisecond
-      jest.advanceTimersByTime(1);
-      const secondGen = download.getTimeStampUrl(testUrl);
-      expect(firstGen).not.toEqual(secondGen);
-    });
-  });
-
   describe('downloadFile', () => {
     let fetch;
-    let getTimeStampUrl;
     const blob = 'test-blob';
     const file = files[0];
     beforeEach(() => {
       fetch = window.fetch;
       window.fetch = jest.fn();
-      getTimeStampUrl = download.getTimeStampUrl;
-      download.getTimeStampUrl = jest.fn();
     });
     afterEach(() => {
       window.fetch = fetch;
-      download.getTimeStampUrl = getTimeStampUrl;
     });
     it('returns blob output if successful', () => {
       window.fetch.mockReturnValue(Promise.resolve({ ok: true, blob: () => blob }));
       expect(download.downloadFile(file)).resolves.toEqual(blob);
-      expect(download.getTimeStampUrl).toBeCalledWith(file.downloadUrl);
     });
     it('throw if not successful', () => {
       const failFetchStatusText = 'failed to fetch';
       window.fetch.mockReturnValue(Promise.resolve({ ok: false, statusText: failFetchStatusText }));
       expect(() => download.downloadFile(file)).rejects.toThrow(failFetchStatusText);
-      expect(download.getTimeStampUrl).toBeCalledWith(file.downloadUrl);
     });
   });
 
@@ -135,10 +118,10 @@ describe('download thunkActions', () => {
     afterEach(() => { download.downloadFile = downloadFile; });
 
     it('returns a mapping of all files to download action', async () => {
-      const downloadedBlobs = await download.downloadBlobs(files);
+      const downloadBlobs = await download.downloadBlobs(files);
       expect(download.downloadFile).toHaveBeenCalledTimes(files.length);
-      expect(downloadedBlobs.length).toEqual(files.length);
-      expect(downloadedBlobs).toEqual(files.map(file => file.name));
+      expect(downloadBlobs.blobs.length).toEqual(files.length);
+      expect(downloadBlobs.blobs).toEqual(files.map(file => file.name));
     });
 
     it('returns a mapping of errors from download action', () => {
@@ -148,25 +131,51 @@ describe('download thunkActions', () => {
     });
   });
 
+  describe('getSubmissionFiles', () => {
+    let fetchSubmissionFiles;
+    beforeEach(() => {
+      fetchSubmissionFiles = api.fetchSubmissionFiles;
+      api.fetchSubmissionFiles = jest.fn();
+    });
+    afterEach(() => { api.fetchSubmissionFiles = fetchSubmissionFiles; });
+    it('return api.fetchSubmissionFiles on success', async () => {
+      api.fetchSubmissionFiles = () => Promise.resolve({ files });
+      const data = await download.getSubmissionFiles();
+      expect(data).toEqual(files);
+    });
+
+    it('throw FetchSubmissionFilesException on fetch failure', () => {
+      api.fetchSubmissionFiles = () => Promise.reject();
+      expect(() => download.getSubmissionFiles()).rejects.toEqual(download.FetchSubmissionFilesException());
+    });
+  });
+
   describe('downloadFiles', () => {
     let downloadBlobs;
+    let getSubmissionFiles;
     beforeEach(() => {
       dispatch = jest.fn();
-      selectors.grading.selected.response = () => ({ files });
+      selectors.grading.selected.submissionUUID = () => submissionUUID;
       selectors.grading.selected.username = () => username;
       download.zipFiles = jest.fn();
 
       downloadBlobs = download.downloadBlobs;
-      download.downloadBlobs = () => Promise.resolve(blobs);
+      download.downloadBlobs = () => Promise.resolve({ blobs, files });
+
+      getSubmissionFiles = download.getSubmissionFiles;
+      download.getSubmissionFiles = () => Promise.resolve(files);
+    });
+    afterEach(() => {
+      download.downloadBlobs = downloadBlobs;
+      download.getSubmissionFiles = getSubmissionFiles;
     });
-    afterEach(() => { download.downloadBlobs = downloadBlobs; });
     it('dispatches network request with downloadFiles key', () => {
       download.downloadFiles()(dispatch, getState);
       const { networkRequest } = dispatch.mock.calls[0][0];
       expect(networkRequest.requestKey).toEqual(RequestKeys.downloadFiles);
     });
     it('dispatches network request for downloadFiles, zipping output of downloadBlobs', async () => {
-      download.downloadBlobs = () => Promise.resolve(blobs);
+      download.downloadBlobs = () => Promise.resolve({ blobs, files });
       download.downloadFiles()(dispatch, getState);
       const { networkRequest } = dispatch.mock.calls[0][0];
       await networkRequest.promise;

src/data/services/lms/api.js

@@ -14,7 +14,7 @@ import {
  *********************************************************************************/
 
 /**
- * get('/api/initialize', { ora_location, course_id? })
+ * get('/api/initialize', { oraLocation })
  * @return {
  *   oraMetadata: { name, prompt, type ('individual' vs 'team'), rubricConfig, fileUploadResponseConfig },
  *   courseMetadata: { courseOrg, courseName, courseNumber, courseId },
@@ -38,7 +38,7 @@ const initializeApp = () => get(
 ).then(response => response.data);
 
 /**
- * get('/api/submission', { submissionUUID })
+ * get('/api/submission', { oraLocation, submissionUUID })
  * @return {
  *   submision: {
  *     gradeData,
@@ -54,9 +54,22 @@ const fetchSubmission = (submissionUUID) => get(
   }),
 ).then(response => response.data);
 
+/**
+ * get('/api/submission/files', { oraLocation, submissionUUID })
+ * @return {
+ *     response: { files: [{}], text: <html> },
+ * }
+ */
+const fetchSubmissionFiles = (submissionUUID) => get(
+  stringifyUrl(urls.fetchSubmissionFilesUrl, {
+    [paramKeys.oraLocation]: locationId,
+    [paramKeys.submissionUUID]: submissionUUID,
+  }),
+).then(response => response.data);
+
 /**
  * fetches the current grade, gradeStatus, and rubricResponse data for the given submission
- * get('/api/submissionStatus', { submissionUUID })
+ * get('/api/submissionStatus', { oraLocation, submissionUUID })
  *   @return {obj} submissionStatus object
  *   {
  *     gradeData,
@@ -72,7 +85,7 @@ const fetchSubmissionStatus = (submissionUUID) => get(
 ).then(response => response.data);
 
 /**
- * post('api/lock', { ora_location, submissionUUID });
+ * post('api/lock', { oraLocation, submissionUUID });
  * @param {string} submissionUUID
  */
 const lockSubmission = (submissionUUID) => post(
@@ -120,6 +133,7 @@ const updateGrade = (submissionUUID, gradeData) => post(
 export default StrictDict({
   initializeApp,
   fetchSubmission,
+  fetchSubmissionFiles,
   fetchSubmissionStatus,
   lockSubmission,
   updateGrade,

src/data/services/lms/api.test.js

@@ -71,6 +71,19 @@ describe('lms service api methods', () => {
       },
     });
   });
+  describe('fetchSubmissionFiles', () => {
+    testAPI({
+      promise: api.fetchSubmissionFiles(submissionUUID),
+      method: methodKeys.get,
+      expected: {
+        urlKey: urlKeys.fetchSubmissionFilesUrl,
+        urlParams: {
+          [paramKeys.oraLocation]: locationId,
+          [paramKeys.submissionUUID]: submissionUUID,
+        },
+      },
+    });
+  });
   describe('fetchSubmissionStatus', () => {
     testAPI({
       promise: api.fetchSubmissionStatus(submissionUUID),

src/data/services/lms/urls.js

@@ -8,6 +8,7 @@ const baseEsgUrl = `${api}ora_staff_grader/`;
 
 const oraInitializeUrl = `${baseEsgUrl}initialize`;
 const fetchSubmissionUrl = `${baseEsgUrl}submission`;
+const fetchSubmissionFilesUrl = `${baseEsgUrl}submission/files`;
 const fetchSubmissionStatusUrl = `${baseEsgUrl}submission/status`;
 const fetchSubmissionLockUrl = `${baseEsgUrl}submission/lock`;
 const batchUnlockSubmissionsUrl = `${baseEsgUrl}submission/batch/unlock`;
@@ -24,6 +25,7 @@ export default StrictDict({
   api,
   oraInitializeUrl,
   fetchSubmissionUrl,
+  fetchSubmissionFilesUrl,
   fetchSubmissionStatusUrl,
   fetchSubmissionLockUrl,
   batchUnlockSubmissionsUrl,