From 0f41df2cf343e264a54029ad130df2ec28f64821 Mon Sep 17 00:00:00 2001 From: leangseu-edx <83240113+leangseu-edx@users.noreply.github.com> Date: Thu, 12 May 2022 09:45:46 -0400 Subject: [PATCH] feat: add fetch submission files (#110) chore: remove cache busting --- src/data/redux/thunkActions/download.js | 40 +++++++----- src/data/redux/thunkActions/download.test.js | 65 +++++++++++--------- src/data/services/lms/api.js | 22 +++++-- src/data/services/lms/api.test.js | 13 ++++ src/data/services/lms/urls.js | 2 + 5 files changed, 94 insertions(+), 48 deletions(-) diff --git a/src/data/redux/thunkActions/download.js b/src/data/redux/thunkActions/download.js index 943d01d..1daf720 100644 --- a/src/data/redux/thunkActions/download.js +++ b/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,7 +88,20 @@ 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(); + } }; /** @@ -103,11 +109,13 @@ export const downloadBlobs = async (files) => { * 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)), })); }; diff --git a/src/data/redux/thunkActions/download.test.js b/src/data/redux/thunkActions/download.test.js index f5fa2d9..04523a8 100644 --- a/src/data/redux/thunkActions/download.test.js +++ b/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; diff --git a/src/data/services/lms/api.js b/src/data/services/lms/api.js index 5621527..994095d 100644 --- a/src/data/services/lms/api.js +++ b/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: }, + * } + */ +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, diff --git a/src/data/services/lms/api.test.js b/src/data/services/lms/api.test.js index 2cd4165..89f8562 100644 --- a/src/data/services/lms/api.test.js +++ b/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), diff --git a/src/data/services/lms/urls.js b/src/data/services/lms/urls.js index 313ee5b..f4ec138 100644 --- a/src/data/services/lms/urls.js +++ b/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,