From e66da2cb49aa7976a2bc5403a1fbd6209fd8be7c Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 13 Mar 2025 16:11:44 +0000 Subject: [PATCH] fix: image rendering in single and multi select problems (#1731) Fix images in single and multi select problems in libraries. Found following issues and fixed them: * Images were not being rendered in any of the fields in these problems. * Base url was not being set which is used by tinymce to load images with relative path. * Answer fields were set to inline mode which does not initialize images or base paths * If same image twice is used twice in a problem, the logic of replacing `static/image.jpg` with `/static/image.jpg` would replace the first occurrence twice resulting in `//static/image.jpg`, breaking both the links. * On initialization of answer fields, the absolute static asset urls were being replaced by relative urls causing the editor being set as dirty without user changes. --- .../AnswerWidget/AnswerOption.jsx | 9 +++++++++ .../AnswerWidget/AnswerOption.test.jsx | 1 + .../EditProblemView/AnswerWidget/hooks.js | 5 ++++- .../AnswerWidget/hooks.test.js | 4 +++- .../ExplanationWidget/index.jsx | 11 +++++++++- .../ExplanationWidget/index.test.jsx | 1 + .../EditProblemView/QuestionWidget/index.jsx | 11 +++++++++- .../QuestionWidget/index.test.jsx | 1 + .../data/redux/problem/reducers.test.ts | 14 +++++++++++++ src/editors/data/redux/problem/reducers.ts | 4 ++++ .../__snapshots__/index.test.jsx.snap | 12 +++++++---- .../ExpandableTextArea/index.jsx | 3 ++- .../ExpandableTextArea/index.scss | 2 +- .../ExpandableTextArea/index.test.jsx | 9 +++++++++ .../sharedComponents/TinyMceWidget/hooks.js | 20 +++++++++++++++---- .../TinyMceWidget/hooks.test.js | 6 ++++++ .../TinyMceWidget/pluginConfig.js | 6 ++++-- 17 files changed, 103 insertions(+), 16 deletions(-) diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/AnswerOption.jsx b/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/AnswerOption.jsx index 57012064a..edfefc8db 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/AnswerOption.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/AnswerOption.jsx @@ -9,6 +9,7 @@ import { } from '@openedx/paragon'; import { FeedbackOutline, DeleteOutline } from '@openedx/paragon/icons'; import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n'; +import { getConfig } from '@edx/frontend-platform'; import messages from './messages'; import { selectors } from '../../../../../data/redux'; import { answerOptionProps } from '../../../../../data/services/cms/types'; @@ -27,6 +28,7 @@ const AnswerOption = ({ problemType, images, isLibrary, + blockId, learningContextId, }) => { const dispatch = useDispatch(); @@ -41,6 +43,10 @@ const AnswerOption = ({ const setSelectedFeedback = hooks.setSelectedFeedback({ answer, hasSingleAnswer, dispatch }); const setUnselectedFeedback = hooks.setUnselectedFeedback({ answer, hasSingleAnswer, dispatch }); const { isFeedbackVisible, toggleFeedback } = hooks.useFeedback(answer); + let staticRootUrl; + if (isLibrary) { + staticRootUrl = `${getConfig().STUDIO_BASE_URL }/library_assets/blocks/${ blockId }/`; + } const getInputArea = () => { if ([ProblemTypeKeys.SINGLESELECT, ProblemTypeKeys.MULTISELECT].includes(problemType)) { @@ -54,6 +60,7 @@ const AnswerOption = ({ images, isLibrary, learningContextId, + staticRootUrl, }} /> ); @@ -151,6 +158,7 @@ AnswerOption.propTypes = { images: PropTypes.shape({}).isRequired, learningContextId: PropTypes.string.isRequired, isLibrary: PropTypes.bool.isRequired, + blockId: PropTypes.string.isRequired, }; export const mapStateToProps = (state) => ({ @@ -158,6 +166,7 @@ export const mapStateToProps = (state) => ({ images: selectors.app.images(state), isLibrary: selectors.app.isLibrary(state), learningContextId: selectors.app.learningContextId(state), + blockId: selectors.app.blockId(state), }); export const mapDispatchToProps = {}; diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/AnswerOption.test.jsx b/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/AnswerOption.test.jsx index 59c05a67e..53d81e313 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/AnswerOption.test.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/AnswerOption.test.jsx @@ -16,6 +16,7 @@ jest.mock('../../../../../data/redux', () => ({ images: jest.fn(state => ({ images: state })), isLibrary: jest.fn(state => ({ isLibrary: state })), learningContextId: jest.fn(state => ({ learningContextId: state })), + blockId: jest.fn(state => ({ blockId: state })), }, }, thunkActions: { diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/hooks.js b/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/hooks.js index a2ac1b44e..9176d7b09 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/hooks.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/hooks.js @@ -34,12 +34,15 @@ export const setAnswerTitle = ({ hasSingleAnswer, dispatch, problemType, -}) => (updatedTitle) => { +}) => (updatedTitle, isDirty) => { let title = updatedTitle; if ([ProblemTypeKeys.TEXTINPUT, ProblemTypeKeys.NUMERIC, ProblemTypeKeys.DROPDOWN].includes(problemType)) { title = updatedTitle.target.value; } dispatch(actions.problem.updateAnswer({ id: answer.id, hasSingleAnswer, title })); + if (isDirty !== undefined) { + dispatch(actions.problem.setDirty(isDirty)); + } }; export const setSelectedFeedback = ({ answer, hasSingleAnswer, dispatch }) => (value) => { diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/hooks.test.js b/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/hooks.test.js index 028682249..e67e05f00 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/hooks.test.js +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/AnswerWidget/hooks.test.js @@ -26,6 +26,7 @@ jest.mock('../../../../../data/redux', () => ({ problem: { deleteAnswer: (args) => ({ deleteAnswer: args }), updateAnswer: (args) => ({ updateAnswer: args }), + setDirty: (args) => ({ setDirty: args }), }, }, })); @@ -117,12 +118,13 @@ describe('Answer Options Hooks', () => { hasSingleAnswer, dispatch, problemType, - })(updatedTitle); + })(updatedTitle, false); expect(dispatch).toHaveBeenCalledWith(actions.problem.updateAnswer({ id: answer.id, hasSingleAnswer, title: updatedTitle, })); + expect(dispatch).toHaveBeenCalledWith(actions.problem.setDirty(false)); }); }); describe('setSelectedFeedback', () => { diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/ExplanationWidget/index.jsx b/src/editors/containers/ProblemEditor/components/EditProblemView/ExplanationWidget/index.jsx index bf4d5b6cc..12f55ba6c 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/ExplanationWidget/index.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/ExplanationWidget/index.jsx @@ -2,9 +2,10 @@ import React from 'react'; import PropTypes from 'prop-types'; import { connect } from 'react-redux'; import { injectIntl, FormattedMessage, intlShape } from '@edx/frontend-platform/i18n'; +import { getConfig } from '@edx/frontend-platform'; + import { selectors } from '../../../../../data/redux'; import messages from './messages'; - import TinyMceWidget from '../../../../../sharedComponents/TinyMceWidget'; import { prepareEditorRef, replaceStaticWithAsset } from '../../../../../sharedComponents/TinyMceWidget/hooks'; @@ -14,6 +15,7 @@ const ExplanationWidget = ({ learningContextId, images, isLibrary, + blockId, // injected intl, }) => { @@ -24,6 +26,10 @@ const ExplanationWidget = ({ learningContextId, }); const solutionContent = newContent || initialContent; + let staticRootUrl; + if (isLibrary) { + staticRootUrl = `${getConfig().STUDIO_BASE_URL }/library_assets/blocks/${ blockId }/`; + } if (!refReady) { return null; } return (
@@ -45,6 +51,7 @@ const ExplanationWidget = ({ images, isLibrary, learningContextId, + staticRootUrl, }} />
@@ -58,6 +65,7 @@ ExplanationWidget.propTypes = { learningContextId: PropTypes.string.isRequired, images: PropTypes.shape({}).isRequired, isLibrary: PropTypes.bool.isRequired, + blockId: PropTypes.string.isRequired, // injected intl: intlShape.isRequired, }; @@ -66,6 +74,7 @@ export const mapStateToProps = (state) => ({ learningContextId: selectors.app.learningContextId(state), images: selectors.app.images(state), isLibrary: selectors.app.isLibrary(state), + blockId: selectors.app.blockId(state), }); export const ExplanationWidgetInternal = ExplanationWidget; // For testing only diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/ExplanationWidget/index.test.jsx b/src/editors/containers/ProblemEditor/components/EditProblemView/ExplanationWidget/index.test.jsx index 294e06ac5..7b7be5ffa 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/ExplanationWidget/index.test.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/ExplanationWidget/index.test.jsx @@ -16,6 +16,7 @@ jest.mock('../../../../../data/redux', () => ({ learningContextId: jest.fn(state => ({ learningContextId: state })), images: jest.fn(state => ({ images: state })), isLibrary: jest.fn(state => ({ isLibrary: state })), + blockId: jest.fn(state => ({ blockId: state })), }, }, thunkActions: { diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/QuestionWidget/index.jsx b/src/editors/containers/ProblemEditor/components/EditProblemView/QuestionWidget/index.jsx index f1bc9f11d..ee8cbe636 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/QuestionWidget/index.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/QuestionWidget/index.jsx @@ -2,9 +2,10 @@ import React from 'react'; import PropTypes from 'prop-types'; import { connect } from 'react-redux'; import { injectIntl, FormattedMessage, intlShape } from '@edx/frontend-platform/i18n'; +import { getConfig } from '@edx/frontend-platform'; + import { selectors } from '../../../../../data/redux'; import messages from './messages'; - import TinyMceWidget from '../../../../../sharedComponents/TinyMceWidget'; import { prepareEditorRef, replaceStaticWithAsset } from '../../../../../sharedComponents/TinyMceWidget/hooks'; @@ -14,6 +15,7 @@ const QuestionWidget = ({ learningContextId, images, isLibrary, + blockId, // injected intl, }) => { @@ -24,6 +26,10 @@ const QuestionWidget = ({ learningContextId, }); const questionContent = newContent || initialContent; + let staticRootUrl; + if (isLibrary) { + staticRootUrl = `${getConfig().STUDIO_BASE_URL }/library_assets/blocks/${ blockId }/`; + } if (!refReady) { return null; } return (
@@ -42,6 +48,7 @@ const QuestionWidget = ({ images, isLibrary, learningContextId, + staticRootUrl, }} />
@@ -54,6 +61,7 @@ QuestionWidget.propTypes = { learningContextId: PropTypes.string.isRequired, images: PropTypes.shape({}).isRequired, isLibrary: PropTypes.bool.isRequired, + blockId: PropTypes.string.isRequired, // injected intl: intlShape.isRequired, }; @@ -62,6 +70,7 @@ export const mapStateToProps = (state) => ({ learningContextId: selectors.app.learningContextId(state), images: selectors.app.images(state), isLibrary: selectors.app.isLibrary(state), + blockId: selectors.app.blockId(state), }); export const QuestionWidgetInternal = QuestionWidget; // For testing only diff --git a/src/editors/containers/ProblemEditor/components/EditProblemView/QuestionWidget/index.test.jsx b/src/editors/containers/ProblemEditor/components/EditProblemView/QuestionWidget/index.test.jsx index da4df5a7a..6be96389f 100644 --- a/src/editors/containers/ProblemEditor/components/EditProblemView/QuestionWidget/index.test.jsx +++ b/src/editors/containers/ProblemEditor/components/EditProblemView/QuestionWidget/index.test.jsx @@ -18,6 +18,7 @@ jest.mock('../../../../../data/redux', () => ({ learningContextId: jest.fn(state => ({ learningContextId: state })), images: jest.fn(state => ({ images: state })), isLibrary: jest.fn(state => ({ isLibrary: state })), + blockId: jest.fn(state => ({ blockId: state })), }, problem: { question: jest.fn(state => ({ question: state })), diff --git a/src/editors/data/redux/problem/reducers.test.ts b/src/editors/data/redux/problem/reducers.test.ts index 7f2acb2c8..9f4287850 100644 --- a/src/editors/data/redux/problem/reducers.test.ts +++ b/src/editors/data/redux/problem/reducers.test.ts @@ -453,5 +453,19 @@ describe('problem reducer', () => { }); }); }); + describe('setDirty', () => { + it('sets isDirty flag', () => { + expect(reducer( + { + ...testingState, + isDirty: false, + }, + actions.setDirty(false), + )).toEqual({ + ...testingState, + isDirty: false, + }); + }); + }); }); }); diff --git a/src/editors/data/redux/problem/reducers.ts b/src/editors/data/redux/problem/reducers.ts index 32cd7ea8f..f802be028 100644 --- a/src/editors/data/redux/problem/reducers.ts +++ b/src/editors/data/redux/problem/reducers.ts @@ -228,6 +228,10 @@ const problem = createSlice({ problemType: null, }; }, + setDirty: (state, { payload }) => ({ + ...state, + isDirty: payload, + }), }, }); diff --git a/src/editors/sharedComponents/ExpandableTextArea/__snapshots__/index.test.jsx.snap b/src/editors/sharedComponents/ExpandableTextArea/__snapshots__/index.test.jsx.snap index 2bd7017c3..23228aa39 100644 --- a/src/editors/sharedComponents/ExpandableTextArea/__snapshots__/index.test.jsx.snap +++ b/src/editors/sharedComponents/ExpandableTextArea/__snapshots__/index.test.jsx.snap @@ -9,12 +9,14 @@ exports[`ExpandableTextArea snapshots renders as expected with default behavior editorContentHtml="text" editorRef={ { - "current": null, + "current": { + "value": "something", + }, } } editorType="expandable" placeholder={null} - setEditorRef={[Function]} + setEditorRef={[MockFunction hooks.prepareEditorRef.setEditorRef]} updateContent={[MockFunction]} /> @@ -30,12 +32,14 @@ exports[`ExpandableTextArea snapshots renders error message 1`] = ` editorContentHtml="text" editorRef={ { - "current": null, + "current": { + "value": "something", + }, } } editorType="expandable" placeholder={null} - setEditorRef={[Function]} + setEditorRef={[MockFunction hooks.prepareEditorRef.setEditorRef]} updateContent={[MockFunction]} /> diff --git a/src/editors/sharedComponents/ExpandableTextArea/index.jsx b/src/editors/sharedComponents/ExpandableTextArea/index.jsx index a2f30d66d..4e632e386 100644 --- a/src/editors/sharedComponents/ExpandableTextArea/index.jsx +++ b/src/editors/sharedComponents/ExpandableTextArea/index.jsx @@ -11,8 +11,9 @@ const ExpandableTextArea = ({ errorMessage, ...props }) => { - const { editorRef, setEditorRef } = prepareEditorRef(); + const { editorRef, refReady, setEditorRef } = prepareEditorRef(); + if (!refReady) { return null; } return ( <>
diff --git a/src/editors/sharedComponents/ExpandableTextArea/index.scss b/src/editors/sharedComponents/ExpandableTextArea/index.scss index 39659be1f..c4dfb14ad 100644 --- a/src/editors/sharedComponents/ExpandableTextArea/index.scss +++ b/src/editors/sharedComponents/ExpandableTextArea/index.scss @@ -14,7 +14,7 @@ margin: 16px 40px; } } - + *[contentEditable="false"] { outline: 1px solid #D7D3D1; } diff --git a/src/editors/sharedComponents/ExpandableTextArea/index.test.jsx b/src/editors/sharedComponents/ExpandableTextArea/index.test.jsx index 9995360e9..d8766a8f4 100644 --- a/src/editors/sharedComponents/ExpandableTextArea/index.test.jsx +++ b/src/editors/sharedComponents/ExpandableTextArea/index.test.jsx @@ -15,6 +15,15 @@ jest.mock('@tinymce/tinymce-react', () => { jest.mock('../TinyMceWidget', () => 'TinyMceWidget'); +// Mock the TinyMceWidget +jest.mock('../TinyMceWidget/hooks', () => ({ + prepareEditorRef: jest.fn(() => ({ + editorRef: { current: { value: 'something' } }, + refReady: true, + setEditorRef: jest.fn().mockName('hooks.prepareEditorRef.setEditorRef'), + })), +})); + describe('ExpandableTextArea', () => { const props = { value: 'text', diff --git a/src/editors/sharedComponents/TinyMceWidget/hooks.js b/src/editors/sharedComponents/TinyMceWidget/hooks.js index 49ac8a574..5afbba011 100644 --- a/src/editors/sharedComponents/TinyMceWidget/hooks.js +++ b/src/editors/sharedComponents/TinyMceWidget/hooks.js @@ -250,7 +250,12 @@ export const setupCustomBehavior = ({ lmsEndpointUrl, learningContextId, }); - if (newContent) { updateContent(newContent); } + // istanbul ignore if + if (newContent) { + // update content but mark as not dirty as user did not change anything + updateContent(newContent, false); + editor.setDirty(false); + } }); } @@ -479,14 +484,21 @@ export const setAssetToStaticUrl = ({ editorValue, lmsEndpointUrl }) => { content = updatedContent; }); - /* istanbul ignore next */ + const updatedStaticUrls = []; assetSrcs.filter(src => src.startsWith('static/')).forEach(src => { // Before storing assets we make sure that library static assets points again to // `/static/dummy.jpg` instead of using the relative url `static/dummy.jpg` const nameFromEditorSrc = parseAssetName(src); - const portableUrl = `/${ nameFromEditorSrc}`; + const portableUrl = `/${nameFromEditorSrc}`; + if (updatedStaticUrls.includes(portableUrl)) { + // If same image is used multiple times in the same src, + // replace all occurence once and do not process them again. + return; + } + // track updated urls to process only once. + updatedStaticUrls.push(portableUrl); const currentSrc = src.substring(0, src.search(/("|")/)); - const updatedContent = content.replace(currentSrc, portableUrl); + const updatedContent = content.replaceAll(currentSrc, portableUrl); content = updatedContent; }); return content; diff --git a/src/editors/sharedComponents/TinyMceWidget/hooks.test.js b/src/editors/sharedComponents/TinyMceWidget/hooks.test.js index bc7cb3f90..1eb2ed787 100644 --- a/src/editors/sharedComponents/TinyMceWidget/hooks.test.js +++ b/src/editors/sharedComponents/TinyMceWidget/hooks.test.js @@ -221,6 +221,12 @@ describe('TinyMceEditor hooks', () => { const content = module.setAssetToStaticUrl({ editorValue, lmsEndpointUrl }); expect(content).toEqual(' testing link'); }); + it('returns content with updated static img links', () => { + const editorValue = ' testing link'; + const lmsEndpointUrl = getConfig().LMS_BASE_URL; + const content = module.setAssetToStaticUrl({ editorValue, lmsEndpointUrl }); + expect(content).toEqual(' testing link'); + }); }); describe('editorConfig', () => { diff --git a/src/editors/sharedComponents/TinyMceWidget/pluginConfig.js b/src/editors/sharedComponents/TinyMceWidget/pluginConfig.js index 720aacc8b..b9aa7452a 100644 --- a/src/editors/sharedComponents/TinyMceWidget/pluginConfig.js +++ b/src/editors/sharedComponents/TinyMceWidget/pluginConfig.js @@ -12,8 +12,9 @@ const pluginConfig = ({ placeholder, editorType, enableImageUpload }) => { const codeButton = editorType === 'text' ? buttons.code : ''; const labelButton = editorType === 'question' ? buttons.customLabelButton : ''; const quickToolbar = editorType === 'expandable' ? plugins.quickbars : ''; - const inline = editorType === 'expandable'; + const statusbar = editorType !== 'expandable'; const toolbar = editorType !== 'expandable'; + const autoresizeBottomMargin = editorType === 'expandable' ? 10 : 50; const defaultFormat = (editorType === 'question' || editorType === 'expandable') ? 'div' : 'p'; const hasStudioHeader = document.querySelector('.studio-header'); @@ -90,13 +91,14 @@ const pluginConfig = ({ placeholder, editorType, enableImageUpload }) => { relative_urls: true, convert_urls: false, placeholder, - inline, + statusbar, block_formats: 'Header 1=h1;Header 2=h2;Header 3=h3;Header 4=h4;Header 5=h5;Header 6=h6;Div=div;Paragraph=p;Preformatted=pre', forced_root_block: defaultFormat, powerpaste_allow_local_images: true, powerpaste_word_import: 'prompt', powerpaste_html_import: 'prompt', powerpaste_googledoc_import: 'prompt', + autoresize_bottom_margin: autoresizeBottomMargin, }, }) );