From d3df36bb70b16c2dec83bab8757a7e90678ef4d3 Mon Sep 17 00:00:00 2001 From: Sid Verma Date: Tue, 24 Oct 2023 20:41:50 +0530 Subject: [PATCH] chore: update endpoint used to confirm filename conflicts --- .../AssetsDropZone/AssetsDropZone.test.jsx | 10 ++-- src/components/AssetsDropZone/container.jsx | 4 +- src/components/AssetsDropZone/index.jsx | 7 ++- .../AssetsUploadConfirm.test.jsx | 26 ++++----- .../AssetsUploadConfirm/container.jsx | 6 +- src/components/AssetsUploadConfirm/index.jsx | 26 ++++----- src/data/actions/assets.js | 39 +++++++------ src/data/actions/assets.test.js | 56 +++++++------------ src/data/api/client.js | 10 ++-- src/data/constants/actionTypes.js | 6 +- src/data/reducers/assets.js | 10 ++-- src/data/reducers/assets.test.js | 12 ++-- src/utils/constants.jsx | 2 + 13 files changed, 104 insertions(+), 110 deletions(-) diff --git a/src/components/AssetsDropZone/AssetsDropZone.test.jsx b/src/components/AssetsDropZone/AssetsDropZone.test.jsx index 82a403fc..a15d19d3 100644 --- a/src/components/AssetsDropZone/AssetsDropZone.test.jsx +++ b/src/components/AssetsDropZone/AssetsDropZone.test.jsx @@ -5,7 +5,7 @@ import { courseDetails } from '../../utils/testConstants'; import { mountWithIntl } from '../../utils/i18n/enzymeHelper'; const defaultProps = { - preUploadCheck: () => {}, + validateAssetsAndUpload: () => {}, uploadAssets: () => {}, uploadExceedMaxCount: () => {}, uploadExceedMaxSize: () => {}, @@ -71,13 +71,13 @@ describe('', () => { wrapper.instance().onDrop([{}, {}], [{}]); expect(mockUploadInvalidFileType).toBeCalled(); }); - it('call preUploadCheck() for approved files', () => { - const mockPreUploadCheck = jest.fn(); + it('call validateAssetsAndUpload() for approved files', () => { + const mockvalidateAssetsAndUpload = jest.fn(); wrapper.setProps({ - preUploadCheck: mockPreUploadCheck, + validateAssetsAndUpload: mockvalidateAssetsAndUpload, }); wrapper.instance().onDrop([{}, {}], []); - expect(mockPreUploadCheck).toBeCalled(); + expect(mockvalidateAssetsAndUpload).toBeCalled(); }); }); diff --git a/src/components/AssetsDropZone/container.jsx b/src/components/AssetsDropZone/container.jsx index d740257a..85b752a1 100644 --- a/src/components/AssetsDropZone/container.jsx +++ b/src/components/AssetsDropZone/container.jsx @@ -2,7 +2,7 @@ import { connect } from 'react-redux'; import AssetsDropZone from '.'; import { - preUploadCheck, uploadAssets, uploadExceedMaxSize, uploadExceedMaxCount, uploadInvalidFileType, + validateAssetsAndUpload, uploadAssets, uploadExceedMaxSize, uploadExceedMaxCount, uploadInvalidFileType, } from '../../data/actions/assets'; const mapStateToProps = state => ({ @@ -10,7 +10,7 @@ const mapStateToProps = state => ({ }); const mapDispatchToProps = dispatch => ({ - preUploadCheck: (files, courseDetails) => dispatch(preUploadCheck(files, courseDetails)), + validateAssetsAndUpload: (files, courseDetails) => dispatch(validateAssetsAndUpload(files, courseDetails)), uploadAssets: (files, courseDetails) => dispatch(uploadAssets(files, courseDetails)), uploadExceedMaxCount: maxFileCount => dispatch(uploadExceedMaxCount(maxFileCount)), uploadExceedMaxSize: maxFileSizeMB => dispatch(uploadExceedMaxSize(maxFileSizeMB)), diff --git a/src/components/AssetsDropZone/index.jsx b/src/components/AssetsDropZone/index.jsx index 3b9059d9..c1653f31 100644 --- a/src/components/AssetsDropZone/index.jsx +++ b/src/components/AssetsDropZone/index.jsx @@ -6,6 +6,7 @@ import PropTypes from 'prop-types'; import { Button } from '@edx/paragon'; import FontAwesomeStyles from 'font-awesome/css/font-awesome.min.css'; import WrappedMessage from '../../utils/i18n/formattedMessageWrapper'; +import MAX_FILE_UPLOAD_COUNT from '../../utils/constants'; import messages from './displayMessages'; import styles from './AssetsDropZone.scss'; @@ -28,7 +29,7 @@ export default class AssetsDropZone extends React.Component { this.props.uploadInvalidFileType(); } } else { - this.props.preUploadCheck(acceptedFiles, this.props.courseDetails); + this.props.validateAssetsAndUpload(acceptedFiles, this.props.courseDetails); } }; @@ -136,13 +137,13 @@ AssetsDropZone.propTypes = { uploadExceedMaxCount: PropTypes.func.isRequired, uploadExceedMaxSize: PropTypes.func.isRequired, uploadInvalidFileType: PropTypes.func.isRequired, - preUploadCheck: PropTypes.func.isRequired, + validateAssetsAndUpload: PropTypes.func.isRequired, }; AssetsDropZone.defaultProps = { acceptedFileTypes: undefined, buttonRef: () => {}, compactStyle: false, - maxFileCount: 1000, + maxFileCount: MAX_FILE_UPLOAD_COUNT, maxFileSizeMB: 10, }; diff --git a/src/components/AssetsUploadConfirm/AssetsUploadConfirm.test.jsx b/src/components/AssetsUploadConfirm/AssetsUploadConfirm.test.jsx index 55538905..8bd9c20f 100644 --- a/src/components/AssetsUploadConfirm/AssetsUploadConfirm.test.jsx +++ b/src/components/AssetsUploadConfirm/AssetsUploadConfirm.test.jsx @@ -6,29 +6,29 @@ import { mountWithIntl } from '../../utils/i18n/enzymeHelper'; import mockQuerySelector from '../../utils/mockQuerySelector'; const defaultProps = { - files: [], + filesToUpload: [], uploadAssets: () => { }, - clearPreUploadProps: () => {}, + clearUploadConfirmProps: () => {}, courseDetails: {}, - preUploadError: [], + filenameConflicts: [], }; const modalIsClosed = (wrapper) => { - expect(wrapper.prop('preUploadError')).toEqual([]); + expect(wrapper.prop('filenameConflicts')).toEqual([]); expect(wrapper.state('modalOpen')).toEqual(false); expect(wrapper.find(Modal).prop('open')).toEqual(false); }; const modalIsOpen = (wrapper) => { - expect(wrapper.prop('preUploadError')).toBeTruthy(); + expect(wrapper.prop('filenameConflicts')).toBeTruthy(); expect(wrapper.state('modalOpen')).toEqual(true); expect(wrapper.find(Modal).prop('open')).toEqual(true); }; const errorMessageHasCorrectFiles = (wrapper, files) => { - const preUploadError = wrapper.prop('preUploadError'); + const filenameConflicts = wrapper.prop('filenameConflicts'); files.forEach((file) => { - expect(preUploadError).toContain(file); + expect(filenameConflicts).toContain(file); }); }; @@ -57,7 +57,7 @@ describe('AssetsUploadConfirm', () => { it('open if there is an error message', () => { wrapper.setProps({ - preUploadError: ['asset.jpg'], + filenameConflicts: ['asset.jpg'], }); modalIsOpen(wrapper); @@ -67,24 +67,24 @@ describe('AssetsUploadConfirm', () => { describe('behaves', () => { it('Overwrite calls uploadAssets', () => { const mockUploadAssets = jest.fn(); - const files = [new File([''], 'file1')]; + const filesToUpload = [new File([''], 'file1')]; const courseDetails = { id: 'course-v1:edX+DemoX+Demo_Course', }; wrapper.setProps({ - files, + filesToUpload, courseDetails, uploadAssets: mockUploadAssets, }); wrapper.find(Button).filterWhere(button => button.text() === 'Overwrite').simulate('click'); - expect(mockUploadAssets).toBeCalledWith(files, courseDetails); + expect(mockUploadAssets).toBeCalledWith(filesToUpload, courseDetails); }); it('clicking cancel button closes the status alert', () => { wrapper.setProps({ - preUploadError: ['asset.jpg'], - clearPreUploadProps: () => { + filenameConflicts: ['asset.jpg'], + clearUploadConfirmProps: () => { wrapper.setProps({ ...defaultProps, }); diff --git a/src/components/AssetsUploadConfirm/container.jsx b/src/components/AssetsUploadConfirm/container.jsx index ec2417e1..09e848f2 100644 --- a/src/components/AssetsUploadConfirm/container.jsx +++ b/src/components/AssetsUploadConfirm/container.jsx @@ -1,17 +1,17 @@ import { connect } from 'react-redux'; -import { uploadAssets, clearPreUploadProps } from '../../data/actions/assets'; +import { uploadAssets, clearUploadConfirmProps } from '../../data/actions/assets'; import AssetsUploadConfirm from '.'; const mapStateToProps = state => ({ files: state.metadata.files, - preUploadError: state.metadata.preUploadError, + filenameConflicts: state.metadata.filenameConflicts, courseDetails: state.studioDetails.course, }); const mapDispatchToProps = dispatch => ({ uploadAssets: (assets, courseDetails) => dispatch(uploadAssets(assets, courseDetails)), - clearPreUploadProps: () => dispatch(clearPreUploadProps()), + clearUploadConfirmProps: () => dispatch(clearUploadConfirmProps()), }); const WrappedAssetsUploadConfirm = connect( diff --git a/src/components/AssetsUploadConfirm/index.jsx b/src/components/AssetsUploadConfirm/index.jsx index a42da5cf..d32de3fa 100644 --- a/src/components/AssetsUploadConfirm/index.jsx +++ b/src/components/AssetsUploadConfirm/index.jsx @@ -17,32 +17,32 @@ export default class AssetsUploadConfirm extends React.Component { } componentWillReceiveProps(nextProps) { - const { preUploadError } = nextProps; - this.updateAlertOpenState(preUploadError); + const { filenameConflicts } = nextProps; + this.updateAlertOpenState(filenameConflicts); } - updateAlertOpenState = (preUploadError) => { + updateAlertOpenState = (filenameConflicts) => { this.setState({ - modalOpen: preUploadError.length !== 0, + modalOpen: filenameConflicts.length !== 0, }); }; uploadFiles = () => { - this.props.uploadAssets(this.props.files, this.props.courseDetails); + this.props.uploadAssets(this.props.filesToUpload, this.props.courseDetails); }; onClose = () => { this.setState(defaultState); - this.props.clearPreUploadProps(); + this.props.clearUploadConfirmProps(); }; render() { const { uploadFiles } = this; const { modalOpen } = this.state; - const { preUploadError } = this.props; + const { filenameConflicts } = this.props; const listOfFiles = ( ); const content = ( @@ -81,9 +81,9 @@ export default class AssetsUploadConfirm extends React.Component { AssetsUploadConfirm.propTypes = { // eslint-disable-next-line react/forbid-prop-types - files: PropTypes.arrayOf(PropTypes.object), + filesToUpload: PropTypes.arrayOf(PropTypes.object), uploadAssets: PropTypes.func.isRequired, - clearPreUploadProps: PropTypes.func.isRequired, + clearUploadConfirmProps: PropTypes.func.isRequired, courseDetails: PropTypes.shape({ lang: PropTypes.string, url_name: PropTypes.string, @@ -94,10 +94,10 @@ AssetsUploadConfirm.propTypes = { id: PropTypes.string, revision: PropTypes.string, }).isRequired, - preUploadError: PropTypes.arrayOf(PropTypes.string), + filenameConflicts: PropTypes.arrayOf(PropTypes.string), }; AssetsUploadConfirm.defaultProps = { - files: [], - preUploadError: [], + filesToUpload: [], + filenameConflicts: [], }; diff --git a/src/data/actions/assets.js b/src/data/actions/assets.js index a9dc02a4..361f23a7 100644 --- a/src/data/actions/assets.js +++ b/src/data/actions/assets.js @@ -21,6 +21,12 @@ const requestFailed = responseAction => ( responseAction && 'type' in responseAction && responseAction.type === assetActions.request.REQUEST_ASSETS_FAILURE ); +const getUploadConflicts = (filesToUpload, response) => response.json().then((json) => { + const filesFound = json.assets.map(item => item.display_name); + const conflicts = filesToUpload.filter(item => filesFound.includes(item)); + return conflicts; +}); + export const updateRequest = newRequest => ({ type: assetActions.request.UPDATE_REQUEST, newRequest, @@ -273,22 +279,22 @@ export const uploadAssetFailure = (asset, response) => ({ }); export const setFilesToUpload = (files) => ({ - type: assetActions.files.FILES_UPDATE, + type: assetActions.uploadConfirm.FILES_TO_UPLOAD, files, }); -export const setPreUploadError = (preUploadError) => ({ - type: assetActions.files.FILES_PRE_UPLOAD_ERROR, - preUploadError, +export const setFilenameConflicts = (filenameConflicts) => ({ + type: assetActions.uploadConfirm.FILENAME_CONFLICTS, + filenameConflicts, }); -export const clearPreUploadProps = () => (dispatch) => { +export const clearUploadConfirmProps = () => (dispatch) => { dispatch(setFilesToUpload([])); - dispatch(setPreUploadError([])); + dispatch(setFilenameConflicts([])); }; export const uploadAssets = (assets, courseDetails) => (dispatch) => { - dispatch(clearPreUploadProps()); + dispatch(clearUploadConfirmProps()); dispatch(uploadingAssets(assets.length)); // gather all the promises into a single promise that can be returned return Promise.all(assets.map(asset => ( @@ -307,18 +313,17 @@ export const uploadAssets = (assets, courseDetails) => (dispatch) => { ))); }; -export const preUploadCheck = (assets, courseDetails) => (dispatch) => { +export const validateAssetsAndUpload = (assets, courseDetails) => (dispatch) => { const filenames = assets.map(asset => asset.name); - return clientApi.preUploadCheck(courseDetails.id, filenames) - .then((response) => { - if (response.ok) { - return dispatch(uploadAssets(assets, courseDetails)); - } - return response.json().then((resp) => { + return clientApi.getAssetDetails(courseDetails.id, filenames) + .then((response) => getUploadConflicts(filenames, response) + .then((conflicts) => { + if (conflicts.length === 0) { + return dispatch(uploadAssets(assets, courseDetails)); + } dispatch(setFilesToUpload(assets)); - return dispatch(setPreUploadError(resp.files)); - }); - }); + return dispatch(setFilenameConflicts(conflicts)); + })); }; export const uploadExceedMaxCount = maxFileCount => ({ diff --git a/src/data/actions/assets.test.js b/src/data/actions/assets.test.js index 9c75069d..604d22d5 100644 --- a/src/data/actions/assets.test.js +++ b/src/data/actions/assets.test.js @@ -55,19 +55,6 @@ const getUploadResponse = isSuccess => ( }) ); -// currying a function that can be passed to fetch-mock to determine what -// response should be based on whether or not pre-upload check succeeds or fails -const getPreUploadCheckResponse = (isSuccess, errorFiles = []) => ( - () => ({ - status: isSuccess ? 200 : 409, - body: { - allowed: isSuccess, - reason: isSuccess ? '' : 'error', - files: isSuccess ? '' : errorFiles, - }, - }) -); - describe('Assets Action Creators', () => { beforeEach(() => { store = mockStore(initialState); @@ -725,49 +712,46 @@ describe('Assets Action Creators', () => { expect(store.dispatch(actionCreators.uploadingAssets(99, 'response'))).toEqual(expectedAction); }); - it('returns expected state from preUploadCheck success', () => { + it('returns expected state from validateAssetsAndUpload success', () => { + const assets = [new File([''], 'file.txt')]; const assetsResponse = { status: 200, body: { - assets: ['a.txt'], + assets: [], }, }; - fetchMock.mock(`begin:${assetsEndpoint}`, getPreUploadCheckResponse(true), { method: 'post' }); fetchMock.mock(`begin:${assetsEndpoint}`, assetsResponse, { method: 'get' }); - - const assets = ['a.txt', 'b.txt', 'c.txt']; - + fetchMock.mock(`begin:${assetsEndpoint}`, getUploadResponse(true), { method: 'post' }); const expectedActions = [ - { files: [], type: assetActions.files.FILES_UPDATE }, - { preUploadError: [], type: assetActions.files.FILES_PRE_UPLOAD_ERROR }, + { files: [], type: assetActions.uploadConfirm.FILES_TO_UPLOAD }, + { filenameConflicts: [], type: assetActions.uploadConfirm.FILENAME_CONFLICTS }, ]; - return store.dispatch(actionCreators.preUploadCheck(assets, courseDetails)).then(() => { + return store.dispatch(actionCreators.validateAssetsAndUpload(assets, courseDetails)).then(() => { expect(store.getActions().slice(0, 2)).toEqual(expectedActions); // the rest of the actions are similar to what executes in uploadAssets, and tested there }); }); - it('returns expected state from preUploadCheck failure', () => { + it('returns expected state from validateAssetsAndUpload failure', () => { + const assets = [new File([''], 'file.txt'), new File([''], 'file_2.txt')]; const assetsResponse = { status: 200, body: { - assets: ['a.txt', 'b.txt', 'c.txt'], + assets: [{ + display_name: 'file.txt', + }], }, }; - - fetchMock.mock(`begin:${assetsEndpoint}`, getPreUploadCheckResponse(false, ['a.txt']), { method: 'post' }); - fetchMock.mock(`begin:${assetsEndpoint}`, assetsResponse, { method: 'get' }); - - const assets = ['a.txt', 'b.txt', 'c.txt']; + fetchMock.once(`begin:${assetsEndpoint}`, assetsResponse, { method: 'get' }); const expectedActions = [ - { files: assets, type: assetActions.files.FILES_UPDATE }, - { preUploadError: ['a.txt'], type: assetActions.files.FILES_PRE_UPLOAD_ERROR }, + { files: assets, type: assetActions.uploadConfirm.FILES_TO_UPLOAD }, + { filenameConflicts: ['file.txt'], type: assetActions.uploadConfirm.FILENAME_CONFLICTS }, ]; - return store.dispatch(actionCreators.preUploadCheck(assets, courseDetails)).then(() => { + return store.dispatch(actionCreators.validateAssetsAndUpload(assets, courseDetails)).then(() => { expect(store.getActions()).toEqual(expectedActions); }); }); @@ -786,8 +770,8 @@ describe('Assets Action Creators', () => { const assets = ['a.txt', 'b.txt', 'c.txt']; const expectedActions = [ - { files: [], type: assetActions.files.FILES_UPDATE }, - { preUploadError: [], type: assetActions.files.FILES_PRE_UPLOAD_ERROR }, + { files: [], type: assetActions.uploadConfirm.FILES_TO_UPLOAD }, + { filenameConflicts: [], type: assetActions.uploadConfirm.FILENAME_CONFLICTS }, { count: assets.length, type: assetActions.upload.UPLOADING_ASSETS }, ]; @@ -823,8 +807,8 @@ describe('Assets Action Creators', () => { const assets = ['a.txt', 'b.txt', 'c.txt']; const expectedActions = [ - { files: [], type: assetActions.files.FILES_UPDATE }, - { preUploadError: [], type: assetActions.files.FILES_PRE_UPLOAD_ERROR }, + { files: [], type: assetActions.uploadConfirm.FILES_TO_UPLOAD }, + { filenameConflicts: [], type: assetActions.uploadConfirm.FILENAME_CONFLICTS }, { count: assets.length, type: assetActions.upload.UPLOADING_ASSETS }, ]; diff --git a/src/data/api/client.js b/src/data/api/client.js index 80397d75..42488764 100644 --- a/src/data/api/client.js +++ b/src/data/api/client.js @@ -4,6 +4,7 @@ import 'whatwg-fetch'; // fetch polyfill import endpoints from './endpoints'; import { getDatabaseAttributesFromAssetAttributes } from '../../utils/getAssetsAttributes'; +import { MAX_FILE_UPLOAD_COUNT } from '../../utils/constants'; export function pingStudioHome() { return fetch(endpoints.home, { @@ -65,11 +66,12 @@ export function requestToggleLockAsset(courseId, asset) { }); } -export function preUploadCheck(courseId, filenames) { - return fetch(`${endpoints.assets}/${courseId}/pre_upload_check`, { +export function getAssetDetails(courseId, filenames) { + const params = new URLSearchParams(filenames.map(filename => ['display_name', filename])); + params.append('page_size', MAX_FILE_UPLOAD_COUNT); + return fetch(`${endpoints.assets}/${courseId}?${params}`, { credentials: 'same-origin', - method: 'post', - body: JSON.stringify({ filenames }), + method: 'get', headers: { 'Accept': 'application/json', 'X-CSRFToken': Cookies.get('csrftoken'), diff --git a/src/data/constants/actionTypes.js b/src/data/constants/actionTypes.js index f290bd82..d355e155 100644 --- a/src/data/constants/actionTypes.js +++ b/src/data/constants/actionTypes.js @@ -55,9 +55,9 @@ export const assetActions = { imagePreview: { IMAGE_PREVIEW_UPDATE: 'IMAGE_PREVIEW_UPDATE', }, - files: { - FILES_UPDATE: 'FILES_UPDATE', - FILES_PRE_UPLOAD_ERROR: 'FILES_PRE_UPLOAD_ERROR', + uploadConfirm: { + FILES_TO_UPLOAD: 'FILES_TO_UPLOAD', + FILENAME_CONFLICTS: 'FILENAME_CONFLICTS', }, }; diff --git a/src/data/reducers/assets.js b/src/data/reducers/assets.js index 376b0085..1bdb6258 100644 --- a/src/data/reducers/assets.js +++ b/src/data/reducers/assets.js @@ -135,17 +135,17 @@ export const search = (state = searchInitial, action) => { export const files = (state = [], action) => { switch (action.type) { - case assetActions.files.FILES_UPDATE: + case assetActions.uploadConfirm.FILES_TO_UPLOAD: return action.files; default: return state; } }; -export const preUploadError = (state = [], action) => { +export const filenameConflicts = (state = [], action) => { switch (action.type) { - case assetActions.files.FILES_PRE_UPLOAD_ERROR: - return action.preUploadError; + case assetActions.uploadConfirm.FILENAME_CONFLICTS: + return action.filenameConflicts; default: return state; } @@ -304,5 +304,5 @@ export const metadata = combineReducers({ sort, status, files, - preUploadError, + filenameConflicts, }); diff --git a/src/data/reducers/assets.test.js b/src/data/reducers/assets.test.js index fffeba52..a754854f 100644 --- a/src/data/reducers/assets.test.js +++ b/src/data/reducers/assets.test.js @@ -592,27 +592,27 @@ describe('Assets Reducers', () => { }); }); describe('files reducer', () => { - it('returns correct files state on FILES_UPDATE action', () => { + it('returns correct files state on FILES_TO_UPLOAD action', () => { defaultState = []; action = { files: ['file1', 'file2'], - type: assetActions.files.FILES_UPDATE, + type: assetActions.uploadConfirm.FILES_TO_UPLOAD, }; state = reducers.files(defaultState, action); expect(state).toEqual(['file1', 'file2']); }); - it('returns correct preUploadError state on FILES_PRE_UPLOAD_ERROR action', () => { + it('returns correct filenameConflicts state on FILENAME_CONFLICTS action', () => { defaultState = ''; action = { - preUploadError: ['file.txt'], - type: assetActions.files.FILES_PRE_UPLOAD_ERROR, + filenameConflicts: ['file.txt'], + type: assetActions.uploadConfirm.FILENAME_CONFLICTS, }; - state = reducers.preUploadError(defaultState, action); + state = reducers.filenameConflicts(defaultState, action); expect(state).toEqual(['file.txt']); }); diff --git a/src/utils/constants.jsx b/src/utils/constants.jsx index 31c318e4..dc718ef2 100644 --- a/src/utils/constants.jsx +++ b/src/utils/constants.jsx @@ -107,3 +107,5 @@ export const COURSE_BEST_PRACTICES_DATA_SHAPE = { videos: PropTypes.shape(UNITS_SHAPE), is_self_paced: PropTypes.bool, }; + +export const MAX_FILE_UPLOAD_COUNT = 100;