fix: unit preview in course outline sidebar (#2940)

Author: navinkarkera

src/course-unit/unit-sidebar/UnitSidebarContext.tsx

@@ -94,9 +94,11 @@ export const UnitSidebarProvider = ({
   );
 };
 
-export function useUnitSidebarContext(): UnitSidebarContextData {
+export function useUnitSidebarContext(raiseError?: true): UnitSidebarContextData;
+export function useUnitSidebarContext(raiseError?: boolean): UnitSidebarContextData | undefined;
+export function useUnitSidebarContext(raiseError: boolean = true): UnitSidebarContextData | undefined {
   const ctx = useContext(UnitSidebarContext);
-  if (ctx === undefined) {
+  if (ctx === undefined && raiseError) {
     /* istanbul ignore next */
     throw new Error('useUnitSidebarContext() was used in a component without a <UnitSidebarProvider> ancestor.');
   }

src/course-unit/xblock-container-iframe/index.tsx

@@ -58,7 +58,7 @@ const XBlockContainerIframe: FC<XBlockContainerIframeProps> = ({
   const {
     setCurrentPageKey,
     setSelectedComponentId,
-  } = useUnitSidebarContext();
+  } = useUnitSidebarContext(!readonly) || {};
 
   // Useful to reload iframe
   const [iframeKey, setIframeKey] = useState(0);
@@ -138,7 +138,7 @@ const XBlockContainerIframe: FC<XBlockContainerIframeProps> = ({
   const onDeleteSubmit = async () => {
     if (deleteXBlockId) {
       await unitXBlockActions.handleDelete(deleteXBlockId);
-      setSelectedComponentId(undefined);
+      setSelectedComponentId?.(undefined);
       closeDeleteModal();
     }
   };
@@ -192,7 +192,7 @@ const XBlockContainerIframe: FC<XBlockContainerIframeProps> = ({
 
   const handleOpenManageTagsModal = (id: string) => {
     if (isUnitPageNewDesignEnabled()) {
-      setCurrentPageKey('align', id);
+      setCurrentPageKey?.('align', id);
       sendMessageToIframe(messageTypes.selectXblock, { locator: id });
     } else {
       // Legacy manage tags modal
@@ -219,7 +219,7 @@ const XBlockContainerIframe: FC<XBlockContainerIframeProps> = ({
   };
 
   const handleXBlockSelected = (id) => {
-    setCurrentPageKey('info', id);
+    setCurrentPageKey?.('info', id);
   };
 
   const messageHandlers = useMessageHandlers({

src/data/apiHooks.test.tsx

@@ -29,20 +29,20 @@ describe('useWaffleFlags', () => {
     axiosMock.onGet(getApiWaffleFlagsUrl()).reply(() => promise);
 
     render(<FlagComponent />);
-    expect(screen.getByLabelText('isLoading')).toHaveTextContent('loading');
-    expect(screen.getByLabelText('isError')).toHaveTextContent('false');
+    expect(await screen.findByLabelText('isLoading')).toHaveTextContent('loading');
+    expect(await screen.findByLabelText('isError')).toHaveTextContent('false');
     // The default should be enabled, even before we hear back from the server:
-    expect(screen.getByLabelText('useNewCourseOutlinePage')).toHaveTextContent('enabled');
+    expect(await screen.findByLabelText('useNewCourseOutlinePage')).toHaveTextContent('enabled');
 
     // Then, the server responds with a new value:
     resolveResponse([200, { useNewCourseOutlinePage: false }]);
 
     // Now, we're no longer loading and we have the new value:
-    await waitFor(() => {
-      expect(screen.getByLabelText('isLoading')).toHaveTextContent('false');
+    await waitFor(async () => {
+      expect(await screen.findByLabelText('isLoading')).toHaveTextContent('false');
     });
-    expect(screen.getByLabelText('isError')).toHaveTextContent('false');
-    expect(screen.getByLabelText('useNewCourseOutlinePage')).toHaveTextContent('disabled');
+    expect(await screen.findByLabelText('isError')).toHaveTextContent('false');
+    expect(await screen.findByLabelText('useNewCourseOutlinePage')).toHaveTextContent('disabled');
   });
 
   it('uses the default values if there\'s an error', async () => {
@@ -53,20 +53,20 @@ describe('useWaffleFlags', () => {
     axiosMock.onGet(getApiWaffleFlagsUrl()).reply(() => promise);
 
     render(<FlagComponent />);
-    expect(screen.getByLabelText('isLoading')).toHaveTextContent('loading');
-    expect(screen.getByLabelText('isError')).toHaveTextContent('false');
+    expect(await screen.findByLabelText('isLoading')).toHaveTextContent('loading');
+    expect(await screen.findByLabelText('isError')).toHaveTextContent('false');
     // The default should be enabled, even before we hear back from the server:
-    expect(screen.getByLabelText('useNewCourseOutlinePage')).toHaveTextContent('enabled');
+    expect(await screen.findByLabelText('useNewCourseOutlinePage')).toHaveTextContent('enabled');
 
     // Then, the server responds with an error
     resolveResponse([500, {}]);
 
     // Now, we're no longer loading, we have an error state, and we still have the default value:
-    await waitFor(() => {
-      expect(screen.getByLabelText('isLoading')).toHaveTextContent('false');
+    await waitFor(async () => {
+      expect(await screen.findByLabelText('isLoading')).toHaveTextContent('false');
     });
-    expect(screen.getByLabelText('isError')).toHaveTextContent('error');
-    expect(screen.getByLabelText('useNewCourseOutlinePage')).toHaveTextContent('enabled');
+    expect(await screen.findByLabelText('isError')).toHaveTextContent('error');
+    expect(await screen.findByLabelText('useNewCourseOutlinePage')).toHaveTextContent('enabled');
   });
 
   it('uses the global flag values while loading the course-specific flags', async () => {
@@ -81,26 +81,26 @@ describe('useWaffleFlags', () => {
 
     // Check the global flag:
     render(<FlagComponent />);
-    await waitFor(() => {
+    await waitFor(async () => {
       // Once it loads the flags from the server, the global 'false' value will override the default 'true':
-      expect(screen.getByLabelText('useNewCourseOutlinePage')).toHaveTextContent('disabled');
+      expect(await screen.findByLabelText('useNewCourseOutlinePage')).toHaveTextContent('disabled');
     });
 
     // Now check the course-specific flag:
     cleanup();
     render(<FlagComponent courseId={courseId} />);
 
     // Now, the course-specific value is loading but in the meantime we use the global default:
-    expect(screen.getByLabelText('isLoading')).toHaveTextContent('loading');
-    expect(screen.getByLabelText('isError')).toHaveTextContent('false');
-    expect(screen.getByLabelText('useNewCourseOutlinePage')).toHaveTextContent('disabled');
+    expect(await screen.findByLabelText('isLoading')).toHaveTextContent('loading');
+    expect(await screen.findByLabelText('isError')).toHaveTextContent('false');
+    expect(await screen.findByLabelText('useNewCourseOutlinePage')).toHaveTextContent('disabled');
 
     // Now the server responds: the course-specific flag is ON:
     resolveResponse([200, { useNewCourseOutlinePage: true }]);
-    await waitFor(() => {
-      expect(screen.getByLabelText('isLoading')).toHaveTextContent('false');
+    await waitFor(async () => {
+      expect(await screen.findByLabelText('isLoading')).toHaveTextContent('false');
     });
-    expect(screen.getByLabelText('isError')).toHaveTextContent('false');
-    expect(screen.getByLabelText('useNewCourseOutlinePage')).toHaveTextContent('enabled');
+    expect(await screen.findByLabelText('isError')).toHaveTextContent('false');
+    expect(await screen.findByLabelText('useNewCourseOutlinePage')).toHaveTextContent('enabled');
   });
 });