fix: ensure transcript button appears in video player (#1135)

* fix: ensure transcript button appears in video player * chore: unmute video by default * chore: fix * chore: lint * chore: lint * fix: only include vjstranscribe plugin if showTranscripts is true * chore: tests * fix: added test coverage and some nits --------- Co-authored-by: Maham Akif <[email protected]>
openedx · Jul 30, 2024 · 7981e83 · 7981e83
1 parent 5efbadd
commit 7981e83
Show file tree

Hide file tree

Showing 9 changed files with 289 additions and 138 deletions.
diff --git a/src/components/microlearning/VideoDetailPage.jsx b/src/components/microlearning/VideoDetailPage.jsx
@@ -91,11 +91,11 @@ const VideoDetailPage = () => {
               <h2 data-testid="video-title" className="mb-0">
                 {videoData?.courseTitle}
               </h2>
-              <span className="small ml-2 mt-2">
+              <span className="small ml-2 mt-2 text-nowrap">
                 {videoData?.videoDuration && `(${videoData?.videoDuration} minutes)`}
               </span>
             </div>
-            <p className="small align-self-stretch text-justify p-0 mb-0">
+            <p className="small align-self-stretch p-0 mb-0">
               {videoData?.videoSummary}
             </p>
             {videoData?.videoSkills?.length > 0 && (
@@ -123,9 +123,11 @@ const VideoDetailPage = () => {
               </div>
             )}
           </div>
-          <div className="video-player-wrapper position-relative mw-100 overflow-hidden my-4 mt-0">
-            <VideoPlayer videoURL={videoData?.videoUrl} customOptions={customOptions} />
-          </div>
+          { videoData?.videoUrl && (
+            <div className="video-player-wrapper position-relative mw-100 overflow-hidden my-4 mt-2">
+              <VideoPlayer videoURL={videoData?.videoUrl} customOptions={customOptions} />
+            </div>
+          )}
         </article>
         {isDefinedAndNotNull(courseMetadata.activeCourseRun) && (
           <article className="col-12 col-lg-3 pr-0">

diff --git a/src/components/video/VideoJS.jsx b/src/components/video/VideoJS.jsx
@@ -1,11 +1,11 @@
-import React, { useEffect, useRef } from 'react';
+import { useCallback, useEffect, useRef } from 'react';
 import PropTypes from 'prop-types';
 
 import 'videojs-youtube';
 import videojs from 'video.js';
 import 'video.js/dist/video-js.css';
 import { PLAYBACK_RATES } from './data/constants';
-import { fetchAndAddTranscripts } from './data/service';
+import { usePlayerOptions, useTranscripts } from './data';
 
 window.videojs = videojs;
 // eslint-disable-next-line import/no-extraneous-dependencies
@@ -15,7 +15,46 @@ const VideoJS = ({ options, onReady, customOptions }) => {
   const videoRef = useRef(null);
   const playerRef = useRef(null);
 
+  const transcripts = useTranscripts({
+    player: playerRef.current,
+    customOptions,
+  });
+
+  const playerOptions = usePlayerOptions({
+    transcripts,
+    options,
+    customOptions,
+  });
+
+  const handlePlayerReady = useCallback(() => {
+    // Add remote text tracks
+    const textTracks = Object.entries(transcripts.textTracks);
+    textTracks.forEach(([lang, webVttFileUrl]) => {
+      playerRef.current.addRemoteTextTrack({
+        kind: 'subtitles',
+        src: webVttFileUrl,
+        srclang: lang,
+        label: lang,
+      }, false);
+    });
+
+    // Set playback rates
+    if (customOptions?.showPlaybackMenu) {
+      playerRef.current.playbackRates(PLAYBACK_RATES);
+    }
+
+    // Callback to parent component, if provided
+    if (onReady) {
+      onReady(playerRef.current);
+    }
+  }, [customOptions?.showPlaybackMenu, onReady, transcripts.textTracks]);
+
   useEffect(() => {
+    if (transcripts.isLoading) {
+      // While async transcripts data is loading, don't initialize the player
+      return;
+    }
+
     // Make sure Video.js player is only initialized once
     if (!playerRef.current) {
       // The Video.js player needs to be _inside_ the component el for React 18 Strict Mode.
@@ -24,46 +63,30 @@ const VideoJS = ({ options, onReady, customOptions }) => {
       videoElement.classList.add('vjs-big-play-centered');
       videoRef.current.appendChild(videoElement);
 
-      // eslint-disable-next-line no-multi-assign
-      const player = playerRef.current = videojs(videoElement, options, () => {
-        if (onReady) {
-          onReady(player);
-        }
-      });
-
-      if (customOptions?.showPlaybackMenu) {
-        player.playbackRates(PLAYBACK_RATES);
-      }
-
-      if (customOptions?.showTranscripts && customOptions?.transcriptUrls) {
-        fetchAndAddTranscripts(customOptions?.transcriptUrls, player);
-      }
+      playerRef.current = videojs(videoElement, playerOptions, handlePlayerReady);
     } else {
-      const player = playerRef.current;
-
-      player.autoplay(options.autoplay);
-      player.src(options.sources);
+      playerRef.current.autoplay(playerOptions.autoplay);
+      playerRef.current.src(playerOptions.sources);
     }
-  }, [onReady, options, videoRef, customOptions]);
+  }, [transcripts.isLoading, playerOptions, handlePlayerReady]);
 
   // Dispose the Video.js player when the functional component unmounts
   useEffect(() => {
-    const player = playerRef.current;
-
-    return () => {
-      if (player && !player.isDisposed()) {
-        player.dispose();
+    const cleanup = () => {
+      if (playerRef.current && !playerRef.current.isDisposed()) {
+        playerRef.current.dispose();
         playerRef.current = null;
       }
     };
-  }, [playerRef]);
+    return cleanup;
+  }, []);
 
   return (
     <>
       <div data-vjs-player className="video-js-wrapper">
         <div ref={videoRef} />
       </div>
-      { customOptions?.showTranscripts && <div id="vjs-transcribe" className="transcript-container" />}
+      {customOptions?.showTranscripts && <div id="vjs-transcribe" className="transcript-container" />}
     </>
   );
 };

diff --git a/src/components/video/data/hooks.js b/src/components/video/data/hooks.js
@@ -0,0 +1,58 @@
+import { useEffect, useState } from 'react';
+import { logError } from '@edx/frontend-platform/logging';
+
+import { fetchAndAddTranscripts } from './service';
+
+export function useTranscripts({ player, customOptions }) {
+  const shouldUseTranscripts = !!(customOptions?.showTranscripts && customOptions?.transcriptUrls);
+  const [isLoading, setIsLoading] = useState(shouldUseTranscripts);
+  const [textTracks, setTextTracks] = useState([]);
+  const [transcriptUrl, setTranscriptUrl] = useState(null);
+
+  useEffect(() => {
+    const fetchFn = async () => {
+      setIsLoading(true);
+      if (shouldUseTranscripts) {
+        try {
+          const result = await fetchAndAddTranscripts(customOptions.transcriptUrls, player);
+          setTextTracks(result);
+          // We are only catering to English transcripts for now as we don't have the option to change
+          // the transcript language yet.
+          if (result.en) {
+            setTranscriptUrl(result.en);
+          }
+        } catch (error) {
+          logError(`Error fetching transcripts for player: ${error}`);
+        } finally {
+          setIsLoading(false);
+        }
+      }
+    };
+    fetchFn();
+  }, [customOptions?.transcriptUrls, player, shouldUseTranscripts]);
+
+  return {
+    textTracks,
+    transcriptUrl,
+    isLoading,
+  };
+}
+
+export function usePlayerOptions({
+  transcripts,
+  options,
+  customOptions,
+}) {
+  const plugins = { ...options.plugins };
+  if (customOptions?.showTranscripts && transcripts.transcriptUrl) {
+    const existingTranscribeUrls = plugins.vjstranscribe?.urls || [];
+    plugins.vjstranscribe = {
+      ...plugins.vjstranscribe,
+      urls: [transcripts.transcriptUrl, ...existingTranscribeUrls],
+    };
+  }
+  return {
+    ...options,
+    plugins,
+  };
+}
diff --git a/src/components/video/data/index.js b/src/components/video/data/index.js
@@ -0,0 +1,4 @@
+export * from './constants';
+export * from './hooks';
+export * from './service';
+export * from './utils';
diff --git a/src/components/video/data/service.js b/src/components/video/data/service.js
@@ -1,41 +1,25 @@
 import { logError } from '@edx/frontend-platform/logging';
 import { convertToWebVtt, createWebVttFile } from './utils';
 
-const fetchAndAddTranscripts = async (transcriptUrls, player) => {
+const fetchAndAddTranscripts = async (transcriptUrls) => {
+  const data = {};
   const transcriptPromises = Object.entries(transcriptUrls).map(([lang, url]) => fetch(url)
-    .then(response => {
-      if (!response.ok) {
-        logError(`Failed to fetch transcript for ${lang}`);
-      }
-      return response.json();
-    })
-    .then(transcriptData => {
+    .then(response => response.json())
+    .then((transcriptData) => {
       const webVttData = convertToWebVtt(transcriptData);
       const webVttFileUrl = createWebVttFile(webVttData);
-
-      player.addRemoteTextTrack({
-        kind: 'subtitles',
-        src: webVttFileUrl,
-        srclang: lang,
-        label: lang,
-      }, false);
-
-      // We are only catering to English transcripts for now as we don't have the option to change
-      // the transcript language yet.
-      if (lang === 'en') {
-        player.vjstranscribe({
-          urls: [webVttFileUrl],
-        });
-      }
+      data[lang] = webVttFileUrl;
     })
     .catch(error => {
-      logError(`Error fetching or processing transcript for ${lang}:`, error);
+      logError(`Error fetching or processing transcript for ${lang}: ${error}`);
     }));
 
   try {
     await Promise.all(transcriptPromises);
+    return data;
   } catch (error) {
-    logError('Error fetching or processing transcripts:', error);
+    logError(`Error fetching or processing transcripts: ${error}`);
+    return data;
   }
 };
 

diff --git a/src/components/video/data/tests/hooks.test.js b/src/components/video/data/tests/hooks.test.js
@@ -0,0 +1,70 @@
+import { renderHook } from '@testing-library/react-hooks';
+import { logError } from '@edx/frontend-platform/logging';
+import { useTranscripts } from '../hooks';
+import { fetchAndAddTranscripts } from '../service';
+
+// Mocking dependencies
+jest.mock('../service', () => ({
+  fetchAndAddTranscripts: jest.fn(),
+}));
+
+jest.mock('@edx/frontend-platform/logging', () => ({
+  logError: jest.fn(),
+}));
+
+describe('useTranscripts', () => {
+  const customOptions = {
+    showTranscripts: true,
+    transcriptUrls: {
+      en: 'https://example.com/transcript-en.txt',
+    },
+  };
+  const mockPlayer = {};
+
+  it('should set isLoading to true initially if showTranscripts is true', () => {
+    const { result } = renderHook(() => useTranscripts({ player: mockPlayer, customOptions }));
+    expect(result.current.isLoading).toBe(true);
+  });
+
+  it('should fetch and set textTracks and transcriptUrl correctly', async () => {
+    const textTracks = { en: 'https://example.com/transcript-en.txt' };
+    fetchAndAddTranscripts.mockResolvedValueOnce(textTracks);
+
+    const { result, waitForNextUpdate } = renderHook(() => useTranscripts({ player: mockPlayer, customOptions }));
+
+    await waitForNextUpdate();
+
+    expect(result.current.isLoading).toBe(false);
+    expect(result.current.textTracks).toEqual(textTracks);
+    expect(result.current.transcriptUrl).toBe(textTracks.en);
+  });
+
+  it('should log error and set isLoading to false if fetching transcripts fails', async () => {
+    const errorMessage = 'Error fetching transcripts';
+    fetchAndAddTranscripts.mockRejectedValueOnce(new Error(errorMessage));
+
+    const { result, waitForNextUpdate } = renderHook(() => useTranscripts({ player: mockPlayer, customOptions }));
+
+    await waitForNextUpdate();
+
+    expect(logError).toHaveBeenCalledWith(`Error fetching transcripts for player: Error: ${errorMessage}`);
+    expect(result.current.isLoading).toBe(false);
+    expect(result.current.textTracks).toEqual([]);
+    expect(result.current.transcriptUrl).toBeNull();
+  });
+
+  it('should not fetch transcripts if showTranscripts is false', async () => {
+    const customOptionsWithoutTranscripts = {
+      showTranscripts: false,
+      transcriptUrls: undefined,
+    };
+
+    const { result } = renderHook(() => useTranscripts({
+      player: mockPlayer,
+      customOptions: customOptionsWithoutTranscripts,
+    }));
+
+    expect(result.current.textTracks).toEqual([]);
+    expect(result.current.transcriptUrl).toBeNull();
+  });
+});