chore: iframe rendering optimization

refactor: corrected scroll to target xblock refactor: fixed scroll behavior to xblock element refactor: after rebase refactor: fixed tests refactor: updated tests
openedx · Jan 16, 2025 · 4168e54 · 4168e54
1 parent 98fbcff
commit 4168e54
Show file tree

Hide file tree

Showing 17 changed files with 108 additions and 106 deletions.
diff --git a/.gitignore b/.gitignore
@@ -1,6 +1,7 @@
 .DS_Store
 .eslintcache
 .idea
+.run
 node_modules
 npm-debug.log
 coverage

diff --git a/src/course-unit/CourseUnit.test.jsx b/src/course-unit/CourseUnit.test.jsx
@@ -243,7 +243,7 @@ describe('<CourseUnit />', () => {
       );
 
       simulatePostMessageEvent(messageTypes.deleteXBlock, {
-        id: courseVerticalChildrenMock.children[0].block_id,
+        usageId: courseVerticalChildrenMock.children[0].block_id,
       });
 
       expect(getByText(/Delete this component?/i)).toBeInTheDocument();
@@ -257,10 +257,10 @@ describe('<CourseUnit />', () => {
       const deleteButton = getAllByRole('button', { name: /Delete/i })
         .find(({ classList }) => classList.contains('btn-primary'));
 
-      userEvent.click(cancelButton);
+      expect(cancelButton).toBeInTheDocument();
 
       simulatePostMessageEvent(messageTypes.deleteXBlock, {
-        id: courseVerticalChildrenMock.children[0].block_id,
+        usageId: courseVerticalChildrenMock.children[0].block_id,
       });
 
       expect(getByRole('dialog')).toBeInTheDocument();
@@ -296,8 +296,12 @@ describe('<CourseUnit />', () => {
 
     axiosMock
       .onDelete(getXBlockBaseApiUrl(courseVerticalChildrenMock.children[0].block_id))
-      .replyOnce(200, { dummy: 'value' });
-    await executeThunk(deleteUnitItemQuery(courseId, blockId), store.dispatch);
+      .reply(200, { dummy: 'value' });
+    await executeThunk(deleteUnitItemQuery(
+      courseId,
+      courseVerticalChildrenMock.children[0].block_id,
+      simulatePostMessageEvent,
+    ), store.dispatch);
 
     const updatedCourseVerticalChildren = courseVerticalChildrenMock.children.filter(
       child => child.block_id !== courseVerticalChildrenMock.children[0].block_id,
@@ -1617,6 +1621,8 @@ describe('<CourseUnit />', () => {
         callbackFn: requestData.callbackFn,
       }), store.dispatch);
 
+      simulatePostMessageEvent(messageTypes.rollbackMovedXBlock, { locator: requestData.sourceLocator });
+
       const dismissButton = queryByRole('button', {
         name: /dismiss/i, hidden: true,
       });

diff --git a/src/course-unit/add-component/AddComponent.jsx b/src/course-unit/add-component/AddComponent.jsx
@@ -61,6 +61,7 @@ const AddComponent = ({ blockId, handleCreateNewCourseXBlock }) => {
       case COMPONENT_TYPES.problem:
       case COMPONENT_TYPES.video:
         handleCreateNewCourseXBlock({ type, parentLocator: blockId }, ({ courseKey, locator }) => {
+          localStorage.setItem('modalEditLastYPosition', window.scrollY);
           navigate(`/course/${courseKey}/editor/${type}/${locator}`);
         });
         break;

diff --git a/src/course-unit/constants.js b/src/course-unit/constants.js
@@ -52,14 +52,20 @@ export const messageTypes = {
   videoFullScreen: 'plugin.videoFullScreen',
   refreshXBlock: 'refreshXBlock',
   showMoveXBlockModal: 'showMoveXBlockModal',
+  completeXBlockMoving: 'completeXBlockMoving',
+  rollbackMovedXBlock: 'rollbackMovedXBlock',
   showMultipleComponentPicker: 'showMultipleComponentPicker',
   addSelectedComponentsToBank: 'addSelectedComponentsToBank',
   showXBlockLibraryChangesPreview: 'showXBlockLibraryChangesPreview',
   copyXBlock: 'copyXBlock',
   manageXBlockAccess: 'manageXBlockAccess',
+  completeManageXBlockAccess: 'completeManageXBlockAccess',
   deleteXBlock: 'deleteXBlock',
+  completeXBlockDeleting: 'completeXBlockDeleting',
   duplicateXBlock: 'duplicateXBlock',
-  refreshXBlockPositions: 'refreshPositions',
+  completeXBlockDuplicating: 'completeXBlockDuplicating',
   newXBlockEditor: 'newXBlockEditor',
   toggleCourseXBlockDropdown: 'toggleCourseXBlockDropdown',
+  addXBlock: 'addXBlock',
+  scrollToXBlock: 'scrollToXBlock',
 };
diff --git a/src/course-unit/data/thunk.js b/src/course-unit/data/thunk.js
@@ -9,6 +9,7 @@ import { RequestStatus } from '../../data/constants';
 import { NOTIFICATION_MESSAGES } from '../../constants';
 import { updateModel, updateModels } from '../../generic/model-store';
 import { updateClipboardData } from '../../generic/data/slice';
+import { messageTypes } from '../constants';
 import {
   getCourseUnitData,
   editUnitDisplayName,
@@ -126,6 +127,7 @@ export function editCourseUnitVisibilityAndData(
   isVisible,
   groupAccess,
   isDiscussionEnabled,
+  callback,
   blockId = itemId,
 ) {
   return async (dispatch) => {
@@ -143,6 +145,9 @@ export function editCourseUnitVisibilityAndData(
         isDiscussionEnabled,
       ).then(async (result) => {
         if (result) {
+          if (callback) {
+            callback();
+          }
           const courseUnit = await getCourseUnitData(blockId);
           dispatch(fetchCourseItemSuccess(courseUnit));
           const courseVerticalChildrenData = await getCourseVerticalChildren(blockId);
@@ -158,11 +163,8 @@ export function editCourseUnitVisibilityAndData(
   };
 }
 
-export function createNewCourseXBlock(body, callback, blockId) {
+export function createNewCourseXBlock(body, callback, blockId, sendMessageToIframe) {
   return async (dispatch) => {
-    dispatch(updateLoadingCourseXblockStatus({ status: RequestStatus.IN_PROGRESS }));
-    dispatch(updateSavingStatus({ status: RequestStatus.PENDING }));
-
     if (body.stagedContent) {
       dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.pasting));
     } else {
@@ -188,10 +190,10 @@ export function createNewCourseXBlock(body, callback, blockId) {
           const courseVerticalChildrenData = await getCourseVerticalChildren(blockId);
           dispatch(updateCourseVerticalChildren(courseVerticalChildrenData));
           dispatch(hideProcessingNotification());
-          dispatch(updateLoadingCourseXblockStatus({ status: RequestStatus.SUCCESSFUL }));
-          dispatch(updateSavingStatus({ status: RequestStatus.SUCCESSFUL }));
           if (callback) {
             callback(result);
+          } else {
+            sendMessageToIframe(messageTypes.addXBlock, { data: result });
           }
           const currentBlockId = body.category === 'vertical' ? formattedResult.locator : blockId;
           const courseUnit = await getCourseUnitData(currentBlockId);
@@ -220,13 +222,14 @@ export function fetchCourseVerticalChildrenData(itemId) {
   };
 }
 
-export function deleteUnitItemQuery(itemId, xblockId) {
+export function deleteUnitItemQuery(itemId, xblockId, sendMessageToIframe) {
   return async (dispatch) => {
     dispatch(updateSavingStatus({ status: RequestStatus.PENDING }));
     dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.deleting));
 
     try {
       await deleteUnitItem(xblockId);
+      sendMessageToIframe(messageTypes.completeXBlockDeleting, null);
       const { userClipboard } = await getCourseSectionVerticalData(itemId);
       dispatch(updateClipboardData(userClipboard));
       const courseUnit = await getCourseUnitData(itemId);
@@ -240,13 +243,14 @@ export function deleteUnitItemQuery(itemId, xblockId) {
   };
 }
 
-export function duplicateUnitItemQuery(itemId, xblockId) {
+export function duplicateUnitItemQuery(itemId, xblockId, callback) {
   return async (dispatch) => {
     dispatch(updateSavingStatus({ status: RequestStatus.PENDING }));
     dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.duplicating));
 
     try {
-      await duplicateUnitItem(itemId, xblockId);
+      const { courseKey, locator } = await duplicateUnitItem(itemId, xblockId);
+      callback(courseKey, locator);
       const courseUnit = await getCourseUnitData(itemId);
       dispatch(fetchCourseItemSuccess(courseUnit));
       dispatch(hideProcessingNotification());
@@ -300,9 +304,13 @@ export function patchUnitItemQuery({
       dispatch(updateMovedXBlockParams(xBlockParams));
       dispatch(updateCourseOutlineInfo({}));
       dispatch(updateCourseOutlineInfoLoadingStatus({ status: RequestStatus.IN_PROGRESS }));
-      const courseUnit = await getCourseUnitData(currentParentLocator);
-      dispatch(fetchCourseItemSuccess(courseUnit));
-      callbackFn();
+      try {
+        const courseUnit = await getCourseUnitData(currentParentLocator);
+        dispatch(fetchCourseItemSuccess(courseUnit));
+      } catch (error) {
+        handleResponseErrors(error, dispatch, updateSavingStatus);
+      }
+      callbackFn(sourceLocator);
     } catch (error) {
       handleResponseErrors(error, dispatch, updateSavingStatus);
     } finally {

diff --git a/src/course-unit/header-title/HeaderTitle.jsx b/src/course-unit/header-title/HeaderTitle.jsx
@@ -11,8 +11,6 @@ import {
 import ConfigureModal from '../../generic/configure-modal/ConfigureModal';
 import { getCourseUnitData } from '../data/selectors';
 import { updateQueryPendingStatus } from '../data/slice';
-import { messageTypes } from '../constants';
-import { useIframe } from '../context/hooks';
 import messages from './messages';
 
 const HeaderTitle = ({
@@ -28,15 +26,9 @@ const HeaderTitle = ({
   const currentItemData = useSelector(getCourseUnitData);
   const [isConfigureModalOpen, openConfigureModal, closeConfigureModal] = useToggle(false);
   const { selectedPartitionIndex, selectedGroupsLabel } = currentItemData.userPartitionInfo;
-  const { sendMessageToIframe } = useIframe();
 
   const onConfigureSubmit = (...arg) => {
     handleConfigureSubmit(currentItemData.id, ...arg, closeConfigureModal);
-    // TODO: this artificial delay is a temporary solution
-    // to ensure the iframe content is properly refreshed.
-    setTimeout(() => {
-      sendMessageToIframe(messageTypes.refreshXBlock, null);
-    }, 1000);
   };
 
   const getVisibilityMessage = () => {

diff --git a/src/course-unit/header-title/HeaderTitle.test.jsx b/src/course-unit/header-title/HeaderTitle.test.jsx
@@ -60,53 +60,45 @@ describe('<HeaderTitle />', () => {
   it('render HeaderTitle component correctly', () => {
     const { getByText, getByRole } = renderComponent();
 
-    waitFor(() => {
-      expect(getByText(unitTitle)).toBeInTheDocument();
-      expect(getByRole('button', { name: messages.altButtonEdit.defaultMessage })).toBeInTheDocument();
-      expect(getByRole('button', { name: messages.altButtonSettings.defaultMessage })).toBeInTheDocument();
-    });
+    expect(getByText(unitTitle)).toBeInTheDocument();
+    expect(getByRole('button', { name: messages.altButtonEdit.defaultMessage })).toBeInTheDocument();
+    expect(getByRole('button', { name: messages.altButtonSettings.defaultMessage })).toBeInTheDocument();
   });
 
   it('render HeaderTitle with open edit form', () => {
     const { getByRole } = renderComponent({
       isTitleEditFormOpen: true,
     });
 
-    waitFor(() => {
-      expect(getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage })).toBeInTheDocument();
-      expect(getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage })).toHaveValue(unitTitle);
-      expect(getByRole('button', { name: messages.altButtonEdit.defaultMessage })).toBeInTheDocument();
-      expect(getByRole('button', { name: messages.altButtonSettings.defaultMessage })).toBeInTheDocument();
-    });
+    expect(getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage })).toBeInTheDocument();
+    expect(getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage })).toHaveValue(unitTitle);
+    expect(getByRole('button', { name: messages.altButtonEdit.defaultMessage })).toBeInTheDocument();
+    expect(getByRole('button', { name: messages.altButtonSettings.defaultMessage })).toBeInTheDocument();
   });
 
   it('calls toggle edit title form by clicking on Edit button', () => {
     const { getByRole } = renderComponent();
 
-    waitFor(() => {
-      const editTitleButton = getByRole('button', { name: messages.altButtonEdit.defaultMessage });
-      userEvent.click(editTitleButton);
-      expect(handleTitleEdit).toHaveBeenCalledTimes(1);
-    });
+    const editTitleButton = getByRole('button', { name: messages.altButtonEdit.defaultMessage });
+    userEvent.click(editTitleButton);
+    expect(handleTitleEdit).toHaveBeenCalledTimes(1);
   });
 
-  it('calls saving title by clicking outside or press Enter key', async () => {
+  it('calls saving title by clicking outside or press Enter key', () => {
     const { getByRole } = renderComponent({
       isTitleEditFormOpen: true,
     });
 
-    waitFor(() => {
-      const titleField = getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage });
-      userEvent.type(titleField, ' 1');
-      expect(titleField).toHaveValue(`${unitTitle} 1`);
-      userEvent.click(document.body);
-      expect(handleTitleEditSubmit).toHaveBeenCalledTimes(1);
-
-      userEvent.click(titleField);
-      userEvent.type(titleField, ' 2[Enter]');
-      expect(titleField).toHaveValue(`${unitTitle} 1 2`);
-      expect(handleTitleEditSubmit).toHaveBeenCalledTimes(2);
-    });
+    const titleField = getByRole('textbox', { name: messages.ariaLabelButtonEdit.defaultMessage });
+    userEvent.type(titleField, ' 1');
+    expect(titleField).toHaveValue(`${unitTitle} 1`);
+    userEvent.click(document.body);
+    expect(handleTitleEditSubmit).toHaveBeenCalledTimes(1);
+
+    userEvent.click(titleField);
+    userEvent.type(titleField, ' 2[Enter]');
+    expect(titleField).toHaveValue(`${unitTitle} 1 2`);
+    expect(handleTitleEditSubmit).toHaveBeenCalledTimes(2);
   });
 
   it('displays a visibility message with the selected groups for the unit', async () => {
@@ -125,7 +117,7 @@ describe('<HeaderTitle />', () => {
     const visibilityMessage = messages.definedVisibilityMessage.defaultMessage
       .replace('{selectedGroupsLabel}', 'Visibility group 1');
 
-    waitFor(() => {
+    await waitFor(() => {
       expect(getByText(visibilityMessage)).toBeInTheDocument();
     });
   });
@@ -140,8 +132,8 @@ describe('<HeaderTitle />', () => {
     await executeThunk(fetchCourseUnitQuery(blockId), store.dispatch);
     const { getByText } = renderComponent();
 
-    waitFor(() => {
-      expect(getByText(messages.someVisibilityMessage.defaultMessage)).toBeInTheDocument();
+    await waitFor(() => {
+      expect(getByText(messages.commonVisibilityMessage.defaultMessage)).toBeInTheDocument();
     });
   });
 });
diff --git a/src/course-unit/hooks.jsx b/src/course-unit/hooks.jsx
@@ -84,6 +84,7 @@ export const useCourseUnit = ({ courseId, blockId }) => {
       isVisible,
       groupAccess,
       isDiscussionEnabled,
+      () => sendMessageToIframe(messageTypes.completeManageXBlockAccess, null),
       blockId,
     ));
     closeModalFn();
@@ -113,15 +114,19 @@ export const useCourseUnit = ({ courseId, blockId }) => {
   };
 
   const handleCreateNewCourseXBlock = (body, callback) => (
-    dispatch(createNewCourseXBlock(body, callback, blockId))
+    dispatch(createNewCourseXBlock(body, callback, blockId, sendMessageToIframe))
   );
 
   const unitXBlockActions = {
     handleDelete: (XBlockId) => {
-      dispatch(deleteUnitItemQuery(blockId, XBlockId));
+      dispatch(deleteUnitItemQuery(blockId, XBlockId, sendMessageToIframe));
     },
     handleDuplicate: (XBlockId) => {
-      dispatch(duplicateUnitItemQuery(blockId, XBlockId));
+      dispatch(duplicateUnitItemQuery(
+        blockId,
+        XBlockId,
+        (courseKey, locator) => sendMessageToIframe(messageTypes.completeXBlockDuplicating, { courseKey, locator }),
+      ));
     },
   };
 
@@ -136,7 +141,7 @@ export const useCourseUnit = ({ courseId, blockId }) => {
       currentParentLocator,
       isMoving: false,
       callbackFn: () => {
-        sendMessageToIframe(messageTypes.refreshXBlock, null);
+        sendMessageToIframe(messageTypes.rollbackMovedXBlock, { locator: sourceLocator });
         window.scrollTo({ top: 0, behavior: 'smooth' });
       },
     }));

diff --git a/src/course-unit/move-modal/hooks.tsx b/src/course-unit/move-modal/hooks.tsx
@@ -184,8 +184,8 @@ export const useMoveModal = ({
       title: state.sourceXBlockInfo.current.displayName,
       currentParentLocator: blockId,
       isMoving: true,
-      callbackFn: () => {
-        sendMessageToIframe(messageTypes.refreshXBlock, null);
+      callbackFn: (sourceLocator: string) => {
+        sendMessageToIframe(messageTypes.completeXBlockMoving, { locator: sourceLocator });
         closeModal();
         window.scrollTo({ top: 0, behavior: 'smooth' });
       },

diff --git a/src/course-unit/sidebar/PublishControls.jsx b/src/course-unit/sidebar/PublishControls.jsx
@@ -35,12 +35,14 @@ const PublishControls = ({ blockId }) => {
 
   const handleCourseUnitDiscardChanges = () => {
     closeDiscardModal();
-    dispatch(editCourseUnitVisibilityAndData(blockId, PUBLISH_TYPES.discardChanges));
-    // TODO: this artificial delay is a temporary solution
-    // to ensure the iframe content is properly refreshed.
-    setTimeout(() => {
-      sendMessageToIframe(messageTypes.refreshXBlock, null);
-    }, 1000);
+    dispatch(editCourseUnitVisibilityAndData(
+      blockId,
+      PUBLISH_TYPES.discardChanges,
+      null,
+      null,
+      null,
+      () => sendMessageToIframe(messageTypes.refreshXBlock, null),
+    ));
   };
 
   const handleCourseUnitPublish = () => {

diff --git a/src/course-unit/xblock-container-iframe/hooks/types.ts b/src/course-unit/xblock-container-iframe/hooks/types.ts
@@ -4,7 +4,7 @@ export type UseMessageHandlersTypes = {
   dispatch: (action: any) => void;
   setIframeOffset: (height: number) => void;
   handleDeleteXBlock: (usageId: string) => void;
-  handleRefetchXBlocks: () => void;
+  handleScrollToXBlock: (scrollOffset: number) => void;
   handleDuplicateXBlock: (blockType: string, usageId: string) => void;
   handleManageXBlockAccess: (usageId: string) => void;
 };