Skip to content

Commit

Permalink
Fix video upload failures (#952)
Browse files Browse the repository at this point in the history
  • Loading branch information
jesperhodge authored Apr 15, 2024
1 parent 2fda48f commit 0f440c6
Show file tree
Hide file tree
Showing 11 changed files with 557 additions and 135 deletions.
3 changes: 2 additions & 1 deletion .env.development
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ ENABLE_TEAM_TYPE_SETTING=false
ENABLE_NEW_EDITOR_PAGES=true
ENABLE_UNIT_PAGE=false
ENABLE_ASSETS_PAGE=false
ENABLE_VIDEO_UPLOAD_PAGE_LINK_IN_CONTENT_DROPDOWN=false
ENABLE_VIDEO_UPLOAD_PAGE_LINK_IN_CONTENT_DROPDOWN=true
ENABLE_NEW_VIDEO_UPLOAD_PAGE=true
ENABLE_TAGGING_TAXONOMY_PAGES=true
BBB_LEARN_MORE_URL=''
HOTJAR_APP_ID=''
Expand Down
4 changes: 4 additions & 0 deletions src/files-and-videos/generic/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ const messages = defineMessages({
defaultMessage: 'Apply',
description: 'Label for apply sort button in sort and filter modal',
},
failedLabel: {
id: 'course-authoring.files-and-uploads.filter.failed.label',
defaultMessage: 'Failed',
},
});

export default messages;
Original file line number Diff line number Diff line change
@@ -1,18 +1,24 @@
import React from 'react';
import { PropTypes } from 'prop-types';
import { useIntl } from '@edx/frontend-platform/i18n';
import { Badge } from '@openedx/paragon';
import { VIDEO_FAILURE_STATUSES } from '../../../videos-page/data/constants';
import messages from '../../messages';

const StatusColumn = ({ row }) => {
const { status } = row.original;
const isUploaded = status === 'Success';
const isFailed = VIDEO_FAILURE_STATUSES.includes(status);
const intl = useIntl();
const failedText = intl.formatMessage(messages.failedLabel);

if (isUploaded) {
return null;
}

return (
<Badge variant="light">
{status}
{isFailed ? failedText : status}
</Badge>
);
};
Expand Down
9 changes: 9 additions & 0 deletions src/files-and-videos/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,12 @@
gap: 24px 16px;
grid-template-columns: repeat(3, 33%);
}

.video-upload-spinner {
width: 1.3rem;
height: 1.3rem;
}

.video-upload-warning-text {
font-size: 18px;
}
6 changes: 4 additions & 2 deletions src/files-and-videos/videos-page/VideoThumbnail.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from '@openedx/paragon';
import { FileInput, useFileInput } from '../generic';
import messages from './messages';
import { VIDEO_SUCCESS_STATUSES } from './data/constants';
import { VIDEO_SUCCESS_STATUSES, VIDEO_FAILURE_STATUSES } from './data/constants';
import { RequestStatus } from '../../data/constants';

const VideoThumbnail = ({
Expand Down Expand Up @@ -45,6 +45,8 @@ const VideoThumbnail = ({
const supportedFiles = videoImageSettings?.supportedFileFormats
? Object.values(videoImageSettings.supportedFileFormats) : null;
const isUploaded = VIDEO_SUCCESS_STATUSES.includes(status);
const isFailed = VIDEO_FAILURE_STATUSES.includes(status);
const failedMessage = intl.formatMessage(messages.failedCheckboxLabel);

const showThumbnail = allowThumbnailUpload && thumbnail && isUploaded;

Expand Down Expand Up @@ -84,7 +86,7 @@ const VideoThumbnail = ({
<div className="status-badge">
{!isUploaded && (
<Badge variant="light">
{status}
{!isFailed ? status : failedMessage}
</Badge>
)}
</div>
Expand Down
48 changes: 40 additions & 8 deletions src/files-and-videos/videos-page/VideosPage.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useEffect } from 'react';
import React, { useEffect, useRef } from 'react';
import PropTypes from 'prop-types';
import { useDispatch, useSelector } from 'react-redux';
import {
Expand All @@ -12,6 +12,8 @@ import {
Button,
CheckboxFilter,
Container,
Alert,
Spinner,
} from '@openedx/paragon';
import Placeholder from '@edx/frontend-lib-content-components';

Expand All @@ -24,6 +26,7 @@ import {
fetchVideoDownload,
fetchVideos,
getUsagePaths,
markVideoUploadsInProgressAsFailed,
resetErrors,
updateVideoOrder,
} from './data/thunks';
Expand All @@ -50,9 +53,16 @@ const VideosPage = ({
intl,
}) => {
const dispatch = useDispatch();
const [isTranscriptSettingsOpen, openTranscriptSettings, closeTranscriptSettings] = useToggle(false);
const [
isTranscriptSettingsOpen,
openTranscriptSettings,
closeTranscriptSettings,
] = useToggle(false);
const courseDetails = useModel('courseDetails', courseId);
document.title = getPageHeadTitle(courseDetails?.name, intl.formatMessage(messages.heading));
document.title = getPageHeadTitle(
courseDetails?.name,
intl.formatMessage(messages.heading),
);

useEffect(() => {
dispatch(fetchVideos(courseId));
Expand All @@ -68,7 +78,16 @@ const VideosPage = ({
usageStatus: usagePathStatus,
errors: errorMessages,
pageSettings,
} = useSelector(state => state.videos);
} = useSelector((state) => state.videos);

const uploadingIdsRef = useRef([]);

useEffect(() => {
window.onbeforeunload = () => {
dispatch(markVideoUploadsInProgressAsFailed({ uploadingIdsRef, courseId }));
return undefined;
};
}, []);

const {
isVideoTranscriptEnabled,
Expand All @@ -78,12 +97,14 @@ const VideosPage = ({
videoImageSettings,
} = pageSettings;

const supportedFileFormats = { 'video/*': videoSupportedFileFormats || FILES_AND_UPLOAD_TYPE_FILTERS.video };
const supportedFileFormats = {
'video/*': videoSupportedFileFormats || FILES_AND_UPLOAD_TYPE_FILTERS.video,
};

const handleErrorReset = (error) => dispatch(resetErrors(error));
const handleAddFile = (files) => {
handleErrorReset({ errorType: 'add' });
files.forEach(file => dispatch(addVideoFile(courseId, file, videoIds)));
files.forEach((file) => dispatch(addVideoFile(courseId, file, videoIds, uploadingIdsRef)));
};
const handleDeleteFile = (id) => dispatch(deleteVideoFile(courseId, id));
const handleDownloadFile = (selectedRows) => dispatch(fetchVideoDownload({ selectedRows, courseId }));
Expand Down Expand Up @@ -128,8 +149,14 @@ const VideosPage = ({
Filter: CheckboxFilter,
filter: 'exactTextCase',
filterChoices: [
{ name: intl.formatMessage(messages.transcribedCheckboxLabel), value: 'transcribed' },
{ name: intl.formatMessage(messages.notTranscribedCheckboxLabel), value: 'notTranscribed' },
{
name: intl.formatMessage(messages.transcribedCheckboxLabel),
value: 'transcribed',
},
{
name: intl.formatMessage(messages.notTranscribedCheckboxLabel),
value: 'notTranscribed',
},
],
};
const activeColumn = {
Expand Down Expand Up @@ -201,6 +228,11 @@ const VideosPage = ({
updateFileStatus={updateVideoStatus}
loadingStatus={loadingStatus}
/>
<Alert variant="warning" show={addVideoStatus === RequestStatus.IN_PROGRESS}>
<div className="video-upload-warning-text"><Spinner animation="border" variant="warning" className="video-upload-spinner mr-3" screenReaderText="loading" />
<p className="d-inline"><FormattedMessage {...messages.videoUploadAlertLabel} /></p>
</div>
</Alert>
<ActionRow>
<div className="h2">
<FormattedMessage {...messages.heading} />
Expand Down
74 changes: 60 additions & 14 deletions src/files-and-videos/videos-page/VideosPage.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ import {
addVideoThumbnail,
fetchVideoDownload,
} from './data/thunks';
import { getVideosUrl, getCourseVideosApiUrl, getApiBaseUrl } from './data/api';
import * as api from './data/api';
import videoMessages from './messages';
import messages from '../generic/messages';

const { getVideosUrl, getCourseVideosApiUrl, getApiBaseUrl } = api;

let axiosMock;
let store;
let file;
Expand Down Expand Up @@ -136,7 +138,7 @@ describe('Videos page', () => {
value: [file],
});
fireEvent.drop(dropzone);
await executeThunk(addVideoFile(courseId, file, []), store.dispatch);
await executeThunk(addVideoFile(courseId, file, [], { current: [] }), store.dispatch);
});
const addStatus = store.getState().videos.addingStatus;
expect(addStatus).toEqual(RequestStatus.SUCCESSFUL);
Expand All @@ -157,7 +159,7 @@ describe('Videos page', () => {
roles: [],
},
});
store = initializeStore(initialState);
store = initializeStore({ ...initialState });
axiosMock = new MockAdapter(getAuthenticatedHttpClient());
file = new File(['(⌐□_□)'], 'download.png', { type: 'image/png' });
});
Expand Down Expand Up @@ -224,6 +226,13 @@ describe('Videos page', () => {

expect(addThumbnailButton).toBeNull();
});
describe('with videos with backend status in_progress', () => {
it('should render video with in progress status', async () => {
await mockStore(RequestStatus.IN_PROGRESS);
expect(screen.getByText('Failed')).toBeVisible();
expect(screen.queryByText('In Progress')).not.toBeInTheDocument();
});
});
});

describe('table actions', () => {
Expand All @@ -240,12 +249,46 @@ describe('Videos page', () => {
const { videoIds } = store.getState().videos;
await act(async () => {
userEvent.upload(addFilesButton, file);
await executeThunk(addVideoFile(courseId, file, videoIds), store.dispatch);
await executeThunk(addVideoFile(courseId, file, videoIds, { current: [] }), store.dispatch);
});
const addStatus = store.getState().videos.addingStatus;
expect(addStatus).toEqual(RequestStatus.SUCCESSFUL);
});

it('when uploads are in progress, should show alert and set them to failed on page leave', async () => {
await mockStore(RequestStatus.SUCCESSFUL);

const mockResponseData = { status: '200', ok: true, blob: () => 'Data' };
const mockFetchResponse = Promise.resolve(mockResponseData);
global.fetch = jest.fn().mockImplementation(() => mockFetchResponse);

axiosMock.onPost(getCourseVideosApiUrl(courseId)).reply(204, generateNewVideoApiResponse());
axiosMock.onGet(getCourseVideosApiUrl(courseId)).reply(200, generateAddVideoApiResponse());

const uploadSpy = jest.spyOn(api, 'uploadVideo');
const setFailedSpy = jest.spyOn(api, 'sendVideoUploadStatus').mockImplementation(() => {});
uploadSpy.mockResolvedValue(new Promise(() => {}));

const addFilesButton = screen.getAllByLabelText('file-input')[3];
act(async () => {
userEvent.upload(addFilesButton, file);
});
await waitFor(() => {
const addStatus = store.getState().videos.addingStatus;
expect(addStatus).toEqual(RequestStatus.IN_PROGRESS);
expect(uploadSpy).toHaveBeenCalled();
expect(screen.getByText(videoMessages.videoUploadAlertLabel.defaultMessage)).toBeVisible();
});
act(() => {
window.dispatchEvent(new Event('beforeunload'));
});
await waitFor(() => {
expect(setFailedSpy).toHaveBeenCalledWith(courseId, expect.any(String), expect.any(String), 'upload_failed');
});
uploadSpy.mockRestore();
setFailedSpy.mockRestore();
});

it('should have disabled action buttons', async () => {
await mockStore(RequestStatus.SUCCESSFUL);
const actionsButton = screen.getByText(messages.actionsButtonLabel.defaultMessage);
Expand Down Expand Up @@ -573,7 +616,7 @@ describe('Videos page', () => {
const addFilesButton = screen.getAllByLabelText('file-input')[3];
await act(async () => {
userEvent.upload(addFilesButton, file);
await executeThunk(addVideoFile(courseId, file), store.dispatch);
await executeThunk(addVideoFile(courseId, file, undefined, { current: [] }), store.dispatch);
});
const addStatus = store.getState().videos.addingStatus;
expect(addStatus).toEqual(RequestStatus.FAILED);
Expand All @@ -588,7 +631,7 @@ describe('Videos page', () => {
const addFilesButton = screen.getAllByLabelText('file-input')[3];
await act(async () => {
userEvent.upload(addFilesButton, file);
await executeThunk(addVideoFile(courseId, file), store.dispatch);
await executeThunk(addVideoFile(courseId, file, undefined, { current: [] }), store.dispatch);
});
const addStatus = store.getState().videos.addingStatus;
expect(addStatus).toEqual(RequestStatus.FAILED);
Expand Down Expand Up @@ -623,7 +666,7 @@ describe('Videos page', () => {
const addFilesButton = screen.getAllByLabelText('file-input')[3];
await act(async () => {
userEvent.upload(addFilesButton, file);
await executeThunk(addVideoFile(courseId, file), store.dispatch);
await executeThunk(addVideoFile(courseId, file, undefined, { current: [] }), store.dispatch);
});
const addStatus = store.getState().videos.addingStatus;
expect(addStatus).toEqual(RequestStatus.FAILED);
Expand All @@ -636,19 +679,20 @@ describe('Videos page', () => {

const videoMenuButton = screen.getByTestId('file-menu-dropdown-mOckID1');

await waitFor(() => {
await waitFor(async () => {
axiosMock.onDelete(`${getCourseVideosApiUrl(courseId)}/mOckID1`).reply(404);
fireEvent.click(within(videoMenuButton).getByLabelText('file-menu-toggle'));
fireEvent.click(screen.getByTestId('open-delete-confirmation-button'));
expect(screen.getByText('Delete mOckID1.mp4')).toBeVisible();

fireEvent.click(screen.getByText(messages.deleteFileButtonLabel.defaultMessage));
expect(screen.queryByText('Delete mOckID1.mp4')).toBeNull();

executeThunk(deleteVideoFile(courseId, 'mOckID1', 5), store.dispatch);
});
const deleteStatus = store.getState().videos.deletingStatus;
expect(deleteStatus).toEqual(RequestStatus.FAILED);
executeThunk(deleteVideoFile(courseId, 'mOckID1', 5), store.dispatch);
await waitFor(() => {
const deleteStatus = store.getState().videos.deletingStatus;
expect(deleteStatus).toEqual(RequestStatus.FAILED);
});

expect(screen.getByTestId('grid-card-mOckID1')).toBeVisible();

Expand All @@ -669,8 +713,10 @@ describe('Videos page', () => {
video: { id: 'mOckID3', displayName: 'mOckID3' },
}), store.dispatch);
});
const { usageStatus } = store.getState().videos;
expect(usageStatus).toEqual(RequestStatus.FAILED);
await waitFor(() => {
const { usageStatus } = store.getState().videos;
expect(usageStatus).toEqual(RequestStatus.FAILED);
});
});

it('multiple video files fetch failure should show error', async () => {
Expand Down
3 changes: 2 additions & 1 deletion src/files-and-videos/videos-page/data/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ export const MIN_HEIGHT = 360;
export const ASPECT_RATIO = 16 / 9;
export const ASPECT_RATIO_ERROR_MARGIN = 0.1;
export const TRANSCRIPT_FAILURE_STATUSES = ['Transcript Failed', 'Partial Failure'];
export const VIDEO_PROCESSING_STATUSES = ['Uploading', 'In Progress', 'Uploaded'];
export const VIDEO_PROCESSING_STATUSES = ['In Progress', 'Uploaded']; // Don't add "Uploading" here. Otherwise interrupted uploads will be considered as processing.
export const VIDEO_SUCCESS_STATUSES = ['Ready', 'Imported'];
export const VIDEO_FAILURE_STATUSES = ['Failed', 'Partial Failure', 'Uploading']; // 'Uploading' is added here to handle interrupted uploads.
Loading

0 comments on commit 0f440c6

Please sign in to comment.