refactor: address feedback

Author: bra-i-am

src/editors/sharedComponents/TinyMceWidget/hooks.test.js

@@ -1,5 +1,7 @@
 import 'CourseAuthoring/editors/setupEditorTest';
 import { getConfig } from '@edx/frontend-platform';
+import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
+import * as keyUtils from '../../../generic/key-utils';
 import { MockUseState } from '../../testUtils';
 
 import * as tinyMCE from '../../data/constants/tinyMCE';
@@ -19,6 +21,10 @@ jest.mock('react', () => ({
   useCallback: (cb, prereqs) => ({ cb, prereqs }),
 }));
 
+jest.mock('@edx/frontend-platform/auth', () => ({
+  getAuthenticatedHttpClient: jest.fn(),
+}));
+
 const state = new MockUseState(module);
 const moduleKeys = keyStore(module);
 
@@ -192,30 +198,34 @@ describe('TinyMceEditor hooks', () => {
       const initialContent = `<img src="/static/soMEImagEURl1.jpeg"/><a href="/assets/v1/${baseAssetUrl}/test.pdf">test</a><img src="/${baseAssetUrl}@correct.png" /><img src="/${baseAssetUrl}/correct.png" />`;
       const learningContextId = 'course-v1:org+test+run';
       const lmsEndpointUrl = getConfig().LMS_BASE_URL;
-      it('returns updated src for text editor to update content', () => {
+
+      beforeEach(() => {
+        jest.clearAllMocks();
+      });
+
+      it('returns updated src for text editor to update content', async () => {
         const expected = `<img src="/${baseAssetUrl}@soMEImagEURl1.jpeg"/><a href="/${baseAssetUrl}@test.pdf">test</a><img src="/${baseAssetUrl}@correct.png" /><img src="/${baseAssetUrl}@correct.png" />`;
-        const actual = module.replaceStaticWithAsset({
+        const actual = await module.replaceStaticWithAsset({
           initialContent,
           learningContextId,
           validateAssetUrl: false,
         });
         expect(actual).toEqual(expected);
       });
-      it('returns updated src with absolute url for expandable editor to update content', () => {
-        const editorType = 'expandable';
+      it('returns updated src with absolute url for expandable editor to update content', async () => {
         const expected = `<img src="${lmsEndpointUrl}/${baseAssetUrl}@soMEImagEURl1.jpeg"/><a href="${lmsEndpointUrl}/${baseAssetUrl}@test.pdf">test</a><img src="${lmsEndpointUrl}/${baseAssetUrl}@correct.png" /><img src="${lmsEndpointUrl}/${baseAssetUrl}@correct.png" />`;
-        const actual = module.replaceStaticWithAsset({
+        const actual = await module.replaceStaticWithAsset({
           initialContent,
-          editorType,
+          editorType: 'expandable',
           lmsEndpointUrl,
           learningContextId,
           validateAssetUrl: false,
         });
         expect(actual).toEqual(expected);
       });
-      it('returns false when there are no srcs to update', () => {
+      it('returns false when there are no srcs to update', async () => {
         const content = '<div>Hello world!</div>';
-        const actual = module.replaceStaticWithAsset({ initialContent: content, learningContextId });
+        const actual = await module.replaceStaticWithAsset({ initialContent: content, learningContextId });
         expect(actual).toBeFalsy();
       });
       it('does not convert static URLs with subdirectories but converts direct static files', () => {
@@ -227,6 +237,79 @@ describe('TinyMceEditor hooks', () => {
         });
         expect(actual).toEqual(expected);
       });
+
+      it('replaces multiple static assets in one content string', async () => {
+        const content = `
+      <img src="/static/a.png"/>
+      <img src="/static/b.png"/>
+    `;
+
+        const result = await module.replaceStaticWithAsset({
+          initialContent: content,
+          learningContextId,
+          validateAssetUrl: false,
+        });
+
+        expect(result).toBeTruthy();
+      });
+
+      it('validateAssetUrl success path replaces url', async () => {
+        getAuthenticatedHttpClient.mockReturnValue({
+          get: jest.fn(() => Promise.resolve({})),
+        });
+
+        const content = '<img src="/static/test.png"/>';
+
+        const result = await module.replaceStaticWithAsset({
+          initialContent: content,
+          learningContextId,
+          validateAssetUrl: true,
+        });
+
+        expect(result).toBeTruthy();
+      });
+
+      it('validateAssetUrl failure path keeps original content', async () => {
+        getAuthenticatedHttpClient.mockReturnValue({
+          get: jest.fn(() => Promise.reject(new Error('404'))),
+        });
+
+        const content = '<img src="/static/test.png"/>';
+
+        const result = await module.replaceStaticWithAsset({
+          initialContent: content,
+          learningContextId,
+          validateAssetUrl: true,
+        });
+
+        expect(result).toBeFalsy();
+      });
+
+      it('handles library keys correctly', async () => {
+        jest.spyOn(keyUtils, 'isLibraryKey').mockReturnValue(true);
+
+        const content = '<img src="/static/test.png"/>';
+
+        const result = await module.replaceStaticWithAsset({
+          initialContent: content,
+          learningContextId: 'lib:test',
+          validateAssetUrl: false,
+        });
+
+        expect(result).toContain('static/test.png');
+      });
+
+      it('returns false when asset already valid and no replacement needed', async () => {
+        const content = '<img src="/asset-v1:test+org+run+type@[email protected]"/>';
+
+        const result = await module.replaceStaticWithAsset({
+          initialContent: content,
+          learningContextId,
+          validateAssetUrl: false,
+        });
+
+        expect(result).toBe(false);
+      });
     });
     describe('setAssetToStaticUrl', () => {
       it('returns content with updated img links', () => {

src/editors/sharedComponents/TinyMceWidget/hooks.ts

@@ -105,7 +105,7 @@ export const parseContentForLabels = ({ editor, updateContent }) => {
   }
 };
 
-export const replaceStaticWithAsset = ({
+export const replaceStaticWithAsset = async ({
   initialContent,
   learningContextId,
   editorType,
@@ -118,7 +118,7 @@ export const replaceStaticWithAsset = ({
     src => src.startsWith('/static') || src.startsWith('/asset'),
   );
   if (!isEmpty(srcs)) {
-    srcs.forEach(async src => {
+    for (const src of srcs) {
       const currentContent = content;
       let staticFullUrl;
       const isStatic = src.startsWith('/static/');
@@ -160,6 +160,11 @@ export const replaceStaticWithAsset = ({
         // check if the asset exists on the server before replacing
         try {
           if (validateAssetUrl) {
+            // We intentionally await inside this loop because each replacement
+            // depends on the progressively updated `content` value.
+            // Executing the requests in parallel could introduce race conditions
+            // and produce inconsistent string replacements.
+            // eslint-disable-next-line no-await-in-loop
             await getAuthenticatedHttpClient()
               .get(staticFullUrl);
           }
@@ -169,7 +174,7 @@ export const replaceStaticWithAsset = ({
           content = currentContent;
         }
       }
-    });
+    }
     if (hasChanges) { return content; }
   }
   return false;
@@ -353,9 +358,9 @@ export const setupCustomBehavior = ({
     onAction: toggleLabelFormatting,
   });
   if (editorType === 'expandable') {
-    editor.on('init', () => {
+    editor.on('init', async () => {
       const initialContent = editor.getContent();
-      const newContent = replaceStaticWithAsset({
+      const newContent = await replaceStaticWithAsset({
         initialContent,
         editorType,
         lmsEndpointUrl,
@@ -381,10 +386,11 @@ export const setupCustomBehavior = ({
     }
   });
 
-  editor.on('ExecCommand', /* istanbul ignore next */ (e) => {
+  editor.on('ExecCommand', /* istanbul ignore next */ async (e) => {
     if (editorType === 'text' && e.command === 'mceFocus') {
       const initialContent = editor.getContent();
-      const newContent = replaceStaticWithAsset({
+      // @ts-ignore Some parameters like 'lmsEndpointUrl' were missing here. Fix me?
+      const newContent = await replaceStaticWithAsset({
         initialContent,
         editorType,
         lmsEndpointUrl,

src/schedule-and-details/data/apiHooks.ts

@@ -1,32 +1,24 @@
-import { useQuery, useQueryClient } from '@tanstack/react-query';
-
+import { useQuery } from '@tanstack/react-query';
 import { getCourseDetails } from './api';
 
 /**
  * Get the details of a course.
  */
 export const useGetCourseDetails = (courseId?: string) => {
-  const queryClient = useQueryClient();
-
   const {
-    data, isLoading, isError, refetch,
+    data, isLoading, isError, refetch, dataUpdatedAt
   } = useQuery({
     queryKey: ['courseDetails', courseId],
     queryFn: () => getCourseDetails(courseId),
+    enabled: !!courseId,
     refetchOnWindowFocus: false,
   });
-  let globalDefaults: { [key: string]: any } | undefined;
-  if (data === undefined && courseId) {
-    // If course-specific waffle flags were requested, first default to the
-    // global (studio-wide) flags until we've loaded the course-specific ones.
-    globalDefaults = queryClient.getQueryData(['courseDetails', undefined]);
-  }
   return {
-    ...globalDefaults,
     ...data,
     id: courseId,
     isLoading,
     isError,
     refetch,
+	dataUpdatedAt,
   };
 };

src/schedule-and-details/hooks.jsx

@@ -52,7 +52,7 @@ const useSaveValuesPrompt = (
     if (!isQueryPending && !isEditableState) {
       setEditedValues(initialEditedData);
     }
-  }, [initialEditedData.isLoading]);
+  }, [initialEditedData.dataUpdatedAt]);
 
   useEffect(() => {
     const errors = validateScheduleAndDetails(editedValues, canShowCertificateAvailableDateField, intl);