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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b7fbac9
feat: Enable transcripts for video library
ChrisChV Jan 17, 2025
627b4af
test: Enable transcript in libraries
ChrisChV Jan 20, 2025
a82e796
test: Adding tests for updateTranscriptHandlerUrl
ChrisChV Jan 21, 2025
9c4a930
feat: Suppor delete and replace transcripts in library
ChrisChV Jan 27, 2025
73c4bda
feat: enable download, replace and update transcripts in library
ChrisChV Jan 29, 2025
729780e
Merge branch 'master' into chris/FAL-3989-video-transcripts
ChrisChV Jan 29, 2025
9f74c07
refactor: use getState once in requests.js functions
ChrisChV Jan 29, 2025
4961e73
refactor: trascriptXblockV2 to transcriptXblockV2 in urls
ChrisChV Jan 29, 2025
91b4a9d
refactor: delete getTranscriptDownloadUrlV2 and add it to getTranscri…
ChrisChV Jan 29, 2025
e23367e
style: Fix coverage
ChrisChV Jan 30, 2025
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
13 changes: 12 additions & 1 deletion ...Editor/components/VideoSettingsModal/components/TranscriptWidget/TranscriptActionMenu.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,17 @@ const TranscriptActionMenu = ({
launchDeleteConfirmation,
// redux
getTranscriptDownloadUrl,
getTranscriptDownloadUrlV2,
buildTranscriptUrl,
isLibrary,
}) => {
const input = fileInput({ onAddFile: module.hooks.replaceFileCallback({ language, dispatch: useDispatch() }) });
const downloadLink = transcriptUrl ? buildTranscriptUrl({ transcriptUrl }) : getTranscriptDownloadUrl({ language });
let downloadLink;
if (isLibrary) {
downloadLink = getTranscriptDownloadUrlV2({ language });
} else {
downloadLink = transcriptUrl ? buildTranscriptUrl({ transcriptUrl }) : getTranscriptDownloadUrl({ language });
}
return (
<Dropdown>
<Dropdown.Toggle
Expand Down Expand Up @@ -77,12 +84,16 @@ TranscriptActionMenu.propTypes = {
launchDeleteConfirmation: PropTypes.func.isRequired,
// redux
getTranscriptDownloadUrl: PropTypes.func.isRequired,
getTranscriptDownloadUrlV2: PropTypes.func.isRequired,
buildTranscriptUrl: PropTypes.func.isRequired,
isLibrary: PropTypes.bool.isRequired,
};

export const mapStateToProps = (state) => ({
getTranscriptDownloadUrl: selectors.video.getTranscriptDownloadUrl(state),
getTranscriptDownloadUrlV2: selectors.video.getTranscriptDownloadUrlV2(state),
buildTranscriptUrl: selectors.video.buildTranscriptUrl(state),
isLibrary: selectors.app.isLibrary(state),
});

export const mapDispatchToProps = {
Expand Down
12 changes: 12 additions & 0 deletions ...r/components/VideoSettingsModal/components/TranscriptWidget/TranscriptActionMenu.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,12 @@ jest.mock('../../../../../../data/redux', () => ({
},
},
selectors: {
app: {
isLibrary: jest.fn(args => ({ isLibrary: args })).mockName('selectors.app.isLibrary'),
},
video: {
getTranscriptDownloadUrl: jest.fn(args => ({ getTranscriptDownloadUrl: args })).mockName('selectors.video.getTranscriptDownloadUrl'),
getTranscriptDownloadUrlV2: jest.fn(args => ({ getTranscriptDownloadUrlV2: args })).mockName('selectors.video.getTranscriptDownloadUrlV2'),
buildTranscriptUrl: jest.fn(args => ({ buildTranscriptUrl: args })).mockName('selectors.video.buildTranscriptUrl'),
},
},
Expand Down Expand Up @@ -66,7 +70,9 @@ describe('TranscriptActionMenu', () => {
launchDeleteConfirmation: jest.fn().mockName('launchDeleteConfirmation'),
// redux
getTranscriptDownloadUrl: jest.fn().mockName('selectors.video.getTranscriptDownloadUrl'),
getTranscriptDownloadUrlV2: jest.fn().mockName('selectors.video.getTranscriptDownloadUrlV2'),
buildTranscriptUrl: jest.fn().mockName('selectors.video.buildTranscriptUrl'),
isLibrary: false,
};
afterAll(() => {
jest.clearAllMocks();
Expand All @@ -83,6 +89,12 @@ describe('TranscriptActionMenu', () => {
shallow(<TranscriptActionMenu {...props} transcriptUrl="url" />).snapshot,
).toMatchSnapshot();
});
test('snapshots: renders as expected with isLibrary props: dont show confirm delete', () => {
jest.spyOn(module.hooks, 'replaceFileCallback').mockImplementationOnce(() => jest.fn().mockName('module.hooks.replaceFileCallback'));
expect(
shallow(<TranscriptActionMenu {...props} isLibrary />).snapshot,
).toMatchSnapshot();
});
});
describe('mapStateToProps', () => {
const testState = { A: 'pple', B: 'anana', C: 'ucumber' };
Expand Down
54 changes: 54 additions & 0 deletions ...ettingsModal/components/TranscriptWidget/__snapshots__/TranscriptActionMenu.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,60 @@ exports[`TranscriptActionMenu Snapshots snapshots: renders as expected with defa
</Dropdown>
`;

exports[`TranscriptActionMenu Snapshots snapshots: renders as expected with isLibrary props: dont show confirm delete 1`] = `
<Dropdown>
<Dropdown.Toggle
alt="Actions dropdown"
as="IconButton"
iconAs="Icon"
id="dropdown-toggle-with-iconbutton-video-transcript-widget"
variant="primary"
/>
<Dropdown.Menu
className="video_transcript Action Menu"
>
<Dropdown.Item
key="transcript-actions-sOmenUmBer-replace"
onClick={[MockFunction click input]}
>
<FormattedMessage
defaultMessage="Replace"
description="Message Presented To user for action to replace transcript"
id="authoring.videoeditor.transcript.replaceTranscript"
/>
</Dropdown.Item>
<Dropdown.Item
key="transcript-actions-sOmenUmBer-download"
>
<FormattedMessage
defaultMessage="Download"
description="Message Presented To user for action to download transcript"
id="authoring.videoeditor.transcript.downloadTranscript"
/>
</Dropdown.Item>
<Dropdown.Item
key="transcript-actions-sOmenUmBer-delete"
onClick={[MockFunction launchDeleteConfirmation]}
>
<FormattedMessage
defaultMessage="Delete"
description="Message Presented To user for action to delete transcript"
id="authoring.videoeditor.transcript.deleteTranscript"
/>
</Dropdown.Item>
</Dropdown.Menu>
<FileInput
acceptedFiles=".srt"
fileInput={
{
"click": [MockFunction click input],
"onAddFile": [MockFunction module.hooks.replaceFileCallback],
}
}
/>
</Dropdown>
`;

exports[`TranscriptActionMenu Snapshots snapshots: renders as expected with transcriptUrl props: dont show confirm delete 1`] = `
<Dropdown>
<Dropdown.Toggle
Expand Down
2 changes: 2 additions & 0 deletions ...mponents/VideoSettingsModal/components/TranscriptWidget/__snapshots__/index.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,8 @@ exports[`TranscriptWidget component snapshots snapshot: renders ErrorAlert with
</CollapsibleFormWidget>
`;

exports[`TranscriptWidget component snapshots snapshot: renders when isLibrary is true 1`] = `undefined`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this test? Isn't the TranscriptWidget rendered when isLibrary is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here 91b4a9d


exports[`TranscriptWidget component snapshots snapshots: renders as expected with allowTranscriptDownloads true 1`] = `
<CollapsibleFormWidget
fontSize="x-small"
Expand Down
11 changes: 9 additions & 2 deletions ...ontainers/VideoEditor/components/VideoSettingsModal/components/TranscriptWidget/index.jsx
View file
Open in desktop
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 } 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 @@ -90,13 +90,18 @@ const TranscriptWidget = ({
updateField,
isUploadError,
isDeleteError,
isLibrary,
// injected
intl,
}) => {
const [error] = React.useContext(ErrorContext).transcripts;
const [showImportCard, setShowImportCard] = React.useState(true);
const fullTextLanguages = module.hooks.transcriptLanguages(transcripts, intl);
const hasTranscripts = module.hooks.hasTranscripts(transcripts);
const dispatch = useDispatch();
if (isLibrary) {
dispatch(thunkActions.video.updateTranscriptHandlerUrl());
}

return (
<CollapsibleFormWidget
Expand Down Expand Up @@ -197,6 +202,7 @@ TranscriptWidget.propTypes = {
updateField: PropTypes.func.isRequired,
isUploadError: PropTypes.bool.isRequired,
isDeleteError: PropTypes.bool.isRequired,
isLibrary: PropTypes.bool.isRequired,
intl: PropTypes.shape(intlShape).isRequired,
};
export const mapStateToProps = (state) => ({
Expand All @@ -207,6 +213,7 @@ export const mapStateToProps = (state) => ({
allowTranscriptImport: selectors.video.allowTranscriptImport(state),
isUploadError: selectors.requests.isFailed(state, { requestKey: RequestKeys.uploadTranscript }),
isDeleteError: selectors.requests.isFailed(state, { requestKey: RequestKeys.deleteTranscript }),
isLibrary: selectors.app.isLibrary(state),
Copy link
Contributor

Choose a reason for hiding this comment

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

If you need to access the redux store, please use a hook instead of a prop:

const isLibrary = useSelector(selectors.app.isLibrary);

});

export const mapDispatchToProps = (dispatch) => ({
Expand Down
9 changes: 9 additions & 0 deletions ...ners/VideoEditor/components/VideoSettingsModal/components/TranscriptWidget/index.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ jest.mock('../../../../../../data/redux', () => ({
},

selectors: {
app: {
isLibrary: jest.fn(state => ({ isLibrary: state })),
},
video: {
transcripts: jest.fn(state => ({ transcripts: state })),
selectedVideoTranscriptUrls: jest.fn(state => ({ selectedVideoTranscriptUrls: state })),
Expand Down Expand Up @@ -103,6 +106,7 @@ describe('TranscriptWidget', () => {
updateField: jest.fn().mockName('args.updateField'),
isUploadError: false,
isDeleteError: false,
isLibrary: false,
};

describe('snapshots', () => {
Expand Down Expand Up @@ -151,6 +155,11 @@ describe('TranscriptWidget', () => {
shallow(<TranscriptWidget {...props} isDeleteError transcripts={['en']} />).snapshot,
).toMatchSnapshot();
});
test('snapshot: renders when isLibrary is true', () => {
expect(
shallow(<TranscriptWidget {...props} isDeleteError transcripts={['en']} isLibrary />).snapshot,
).toMatchSnapshot();
});
});
describe('mapStateToProps', () => {
const testState = { A: 'pple', B: 'anana', C: 'ucumber' };
Expand Down
8 changes: 1 addition & 7 deletions ...tainers/VideoEditor/components/VideoSettingsModal/components/VideoPreviewWidget/index.jsx
View file
Open in desktop
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));
4 changes: 2 additions & 2 deletions ...rs/VideoEditor/components/VideoSettingsModal/components/VideoPreviewWidget/index.test.jsx
View file
Open in desktop
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();
});
});
});
4 changes: 1 addition & 3 deletions src/editors/containers/VideoEditor/components/VideoSettingsModal/index.tsx
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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);
Loading