Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Enable transcripts for video library [FC-0076] #1596

Merged
merged 12 commits into from
Feb 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import { connect } from 'react-redux';
import { connect, useDispatch, useSelector } from 'react-redux';
import PropTypes from 'prop-types';
import {
FormattedMessage,
Expand All @@ -17,7 +17,7 @@ import {
} from '@openedx/paragon';
import { Add, InfoOutline } from '@openedx/paragon/icons';

import { actions, selectors } from '../../../../../../data/redux';
import { thunkActions, actions, selectors } from '../../../../../../data/redux';
import messages from './messages';

import { RequestKeys } from '../../../../../../data/constants/requests';
Expand Down Expand Up @@ -97,6 +97,11 @@ const TranscriptWidget = ({
const [showImportCard, setShowImportCard] = React.useState(true);
const fullTextLanguages = module.hooks.transcriptLanguages(transcripts, intl);
const hasTranscripts = module.hooks.hasTranscripts(transcripts);
const isLibrary = useSelector(selectors.app.isLibrary);
const dispatch = useDispatch();
if (isLibrary) {
dispatch(thunkActions.video.updateTranscriptHandlerUrl());
}

return (
<CollapsibleFormWidget
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,14 @@ jest.mock('../../../../../../data/redux', () => ({
thunkActions: {
video: {
deleteTranscript: jest.fn().mockName('thunkActions.video.deleteTranscript'),
updateTranscriptHandlerUrl: jest.fn().mockName('thunkActions.video.updateTranscriptHandlerUrl'),
},
},

selectors: {
app: {
isLibrary: jest.fn(state => ({ isLibrary: state })),
},
video: {
transcripts: jest.fn(state => ({ transcripts: state })),
selectedVideoTranscriptUrls: jest.fn(state => ({ selectedVideoTranscriptUrls: state })),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ export const VideoPreviewWidget = ({
videoSource,
transcripts,
blockTitle,
isLibrary,
intl,
}) => {
const imgRef = React.useRef();
Expand Down Expand Up @@ -47,10 +46,7 @@ export const VideoPreviewWidget = ({
/>
<Stack gap={1} className="justify-content-center">
<h4 className="text-primary mb-0">{blockTitle}</h4>
{!isLibrary && (
// Since content libraries v2 don't support static assets yet, we can't include transcripts.
<LanguageNamesWidget transcripts={transcripts} />
)}
<LanguageNamesWidget transcripts={transcripts} />
{videoType && (
<Hyperlink
className="text-primary x-small"
Expand All @@ -74,15 +70,13 @@ VideoPreviewWidget.propTypes = {
thumbnail: PropTypes.string.isRequired,
transcripts: PropTypes.arrayOf(PropTypes.string).isRequired,
blockTitle: PropTypes.string.isRequired,
isLibrary: PropTypes.bool.isRequired,
};

export const mapStateToProps = (state) => ({
transcripts: selectors.video.transcripts(state),
videoSource: selectors.video.videoSource(state),
thumbnail: selectors.video.thumbnail(state),
blockTitle: selectors.app.blockTitle(state),
isLibrary: selectors.app.isLibrary(state),
});

export default injectIntl(connect(mapStateToProps)(VideoPreviewWidget));
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('VideoPreviewWidget', () => {
expect(screen.queryByText('No transcripts added')).toBeInTheDocument();
});

test('hides transcripts section in preview for libraries', () => {
test('renders transcripts section in preview for libraries', () => {
render(
<VideoPreviewWidget
videoSource="some-source"
Expand All @@ -41,7 +41,7 @@ describe('VideoPreviewWidget', () => {
thumbnail=""
/>,
);
expect(screen.queryByText('No transcripts added')).not.toBeInTheDocument();
expect(screen.queryByText('No transcripts added')).toBeInTheDocument();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,7 @@ const VideoSettingsModal: React.FC<Props> = ({
<SocialShareWidget />
)}
<ThumbnailWidget />
{!isLibrary && ( // Since content libraries v2 don't support static assets yet, we can't include transcripts.
<TranscriptWidget />
)}
<TranscriptWidget />
Copy link
Contributor

@pomegranited pomegranited Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(out of scope for this issue) The Transcripts part of this VideoPreviewWidget is hard to use 😞

  • Can't type in the language I want in the dropdown; I have to scroll all the way down to the language to select it.
  • No spinners shown while transcript uploads / downloads from Youtube are happening -- it just sits there looking like it's doing nothing.
    It also doesn't prevent me from clicking "Save", and so if an upload / replace / delete for a transcript fails, I don't know about it until I re-open the modal.
  • When I import transcripts from YouTube, only one of the imported transcripts (English) shows up in the preview widget.
    I have to save, and the re-edit to see all languages for the imported transcripts.
  • The "select a language then launch file upload control" behavior is non-intuitive, and may not be accessible either? I think these two components should split in two, so I can select my language / upload a file in whatever order I want to.
  • The "Download" transcript menu item tries to send me to a new page. It should open in a new window (which closes after the download completes).

Do we have budget for a follow-up task to fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pomegranited for listing this issues. It will be very helpful!

Do we have budget for a follow-up task to fix this?

I think so, I have to evaluate it.

<DurationWidget />
<HandoutWidget />
<LicenseWidget />
Expand Down
1 change: 1 addition & 0 deletions src/editors/data/constants/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,5 @@ export const RequestKeys = StrictDict({
uploadAsset: 'uploadAsset',
fetchAdvancedSettings: 'fetchAdvancedSettings',
fetchVideoFeatures: 'fetchVideoFeatures',
getHandlerUrl: 'getHandlerUrl',
} as const);
149 changes: 111 additions & 38 deletions src/editors/data/redux/thunkActions/requests.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { RequestKeys } from '../../constants/requests';
import api, { loadImages } from '../../services/cms/api';
import { actions as requestsActions } from '../requests';
import { selectors as appSelectors } from '../app';
import { selectors as videoSelectors } from '../video';

// This 'module' self-import hack enables mocking during tests.
// See src/editors/decisions/0005-internal-editor-testability-decisions.md. The whole approach to how hooks are tested
Expand All @@ -15,7 +16,7 @@ import { acceptedImgKeys } from '../../../sharedComponents/ImageUploadModal/Sele

// Similar to `import { actions, selectors } from '..';` but avoid circular imports:
const actions = { requests: requestsActions };
const selectors = { app: appSelectors };
const selectors = { app: appSelectors, video: videoSelectors };

/**
* Wrapper around a network request promise, that sends actions to the redux store to
Expand Down Expand Up @@ -239,16 +240,30 @@ export const importTranscript = ({ youTubeId, ...rest }) => (dispatch, getState)
};

export const deleteTranscript = ({ language, videoId, ...rest }) => (dispatch, getState) => {
dispatch(module.networkRequest({
requestKey: RequestKeys.deleteTranscript,
promise: api.deleteTranscript({
blockId: selectors.app.blockId(getState()),
language,
videoId,
studioEndpointUrl: selectors.app.studioEndpointUrl(getState()),
}),
...rest,
}));
const state = getState();
const isLibrary = selectors.app.isLibrary(state);
if (isLibrary) {
dispatch(module.networkRequest({
requestKey: RequestKeys.deleteTranscript,
promise: api.deleteTranscriptV2({
language,
videoId,
handlerUrl: selectors.video.transcriptHandlerUrl(state),
}),
...rest,
}));
} else {
dispatch(module.networkRequest({
requestKey: RequestKeys.deleteTranscript,
promise: api.deleteTranscript({
blockId: selectors.app.blockId(state),
language,
videoId,
studioEndpointUrl: selectors.app.studioEndpointUrl(state),
}),
...rest,
}));
}
};

export const uploadTranscript = ({
Expand All @@ -257,17 +272,32 @@ export const uploadTranscript = ({
language,
...rest
}) => (dispatch, getState) => {
dispatch(module.networkRequest({
requestKey: RequestKeys.uploadTranscript,
promise: api.uploadTranscript({
blockId: selectors.app.blockId(getState()),
transcript,
videoId,
language,
studioEndpointUrl: selectors.app.studioEndpointUrl(getState()),
}),
...rest,
}));
const state = getState();
const isLibrary = selectors.app.isLibrary(state);
if (isLibrary) {
dispatch(module.networkRequest({
requestKey: RequestKeys.uploadTranscript,
promise: api.uploadTranscriptV2({
handlerUrl: selectors.video.transcriptHandlerUrl(state),
transcript,
videoId,
language,
}),
...rest,
}));
} else {
dispatch(module.networkRequest({
requestKey: RequestKeys.uploadTranscript,
promise: api.uploadTranscript({
blockId: selectors.app.blockId(state),
transcript,
videoId,
language,
studioEndpointUrl: selectors.app.studioEndpointUrl(state),
}),
...rest,
}));
}
};

export const updateTranscriptLanguage = ({
Expand All @@ -277,28 +307,70 @@ export const updateTranscriptLanguage = ({
videoId,
...rest
}) => (dispatch, getState) => {
dispatch(module.networkRequest({
requestKey: RequestKeys.updateTranscriptLanguage,
promise: api.uploadTranscript({
blockId: selectors.app.blockId(getState()),
transcript: file,
videoId,
language: languageBeforeChange,
newLanguage: newLanguageCode,
studioEndpointUrl: selectors.app.studioEndpointUrl(getState()),
}),
...rest,
}));
const state = getState();
const isLibrary = selectors.app.isLibrary(state);
if (isLibrary) {
dispatch(module.networkRequest({
requestKey: RequestKeys.updateTranscriptLanguage,
promise: api.uploadTranscriptV2({
handlerUrl: selectors.video.transcriptHandlerUrl(state),
transcript: file,
videoId,
language: languageBeforeChange,
newLanguage: newLanguageCode,
}),
...rest,
}));
} else {
dispatch(module.networkRequest({
requestKey: RequestKeys.updateTranscriptLanguage,
promise: api.uploadTranscript({
blockId: selectors.app.blockId(state),
transcript: file,
videoId,
language: languageBeforeChange,
newLanguage: newLanguageCode,
studioEndpointUrl: selectors.app.studioEndpointUrl(state),
}),
...rest,
}));
}
};

export const getTranscriptFile = ({ language, videoId, ...rest }) => (dispatch, getState) => {
const state = getState();
const isLibrary = selectors.app.isLibrary(state);
if (isLibrary) {
dispatch(module.networkRequest({
requestKey: RequestKeys.getTranscriptFile,
promise: api.getTranscriptV2({
handlerUrl: selectors.video.transcriptHandlerUrl(state),
videoId,
language,
}),
...rest,
}));
} else {
dispatch(module.networkRequest({
requestKey: RequestKeys.getTranscriptFile,
promise: api.getTranscript({
studioEndpointUrl: selectors.app.studioEndpointUrl(state),
blockId: selectors.app.blockId(state),
videoId,
language,
}),
...rest,
}));
}
};

export const getHandlerlUrl = ({ handlerName, ...rest }) => (dispatch, getState) => {
dispatch(module.networkRequest({
requestKey: RequestKeys.getTranscriptFile,
promise: api.getTranscript({
requestKey: RequestKeys.getHandlerUrl,
promise: api.getHandlerUrl({
studioEndpointUrl: selectors.app.studioEndpointUrl(getState()),
blockId: selectors.app.blockId(getState()),
videoId,
language,
handlerName,
}),
...rest,
}));
Expand Down Expand Up @@ -368,4 +440,5 @@ export default StrictDict({
fetchAdvancedSettings,
fetchVideoFeatures,
uploadVideo,
getHandlerlUrl,
});
Loading