Skip to content

feat: added markdown editor for editing problems in markdown format - #1805

Merged
bradenmacdonald merged 21 commits into
openedx:masterfrom
mitodl:anas/add-markdown-editor
Apr 23, 2025
Merged

feat: added markdown editor for editing problems in markdown format#1805
bradenmacdonald merged 21 commits into
openedx:masterfrom
mitodl:anas/add-markdown-editor

Conversation

@Anas12091101

@Anas12091101 Anas12091101 commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

Description

This PR adds support for the Markdown editor in the new Authoring MFE, restoring functionality that was previously available only in the legacy Studio.

A new course-specific Waffle Flag, contentstore.use_react_markdown_editor , has been introduced in this openedx/openedx-platform#36512 to control access to the Markdown editor.

When the waffle flag is enabled for a course, the Switch to Markdown Editor button will appear in the problem editor page, allowing authors to create and edit problems using Markdown.

For Teak, as per the discussion here, once a user switches to the Markdown editor, they cannot revert to the form-based or advanced editors unless the waffle flag is turned off for that course.

Problems created using the form-based or advanced editors in the Authoring MFE do not support Markdown, as there is currently no OLX-to-Markdown converter available. As a result, the Switch to Markdown Editor button will not be shown for these problems. Work on the converter can begin after the Teak release.

Useful information to include:

  • Which edX user roles will this change impact? Common user roles are "Course Author"
  • Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).

Creating a new problem using markdown editor

Screen.Recording.2025-04-10.at.7.33.52.PM.mov

Editing existing problems which have valid markdown

Screen.Recording.2025-04-10.at.8.15.16.PM.mov
  • Provide links to the description of corresponding configuration changes. Remember to correctly annotate these
    changes.

Supporting information

Testing instructions

  • Check out the branch anas/add-markdown-waffle-flag in the edx-platform repository.

  • Create a Waffle Flag Course Override named contentstore.use_react_markdown_editor for the course where you want to enable the Markdown editor.

  • In the authoring MFE, create a new problem by selecting any simple problem type.

  • On the problem editor page, click "Show advanced settings", then click "Switch to Markdown Editor".

  • You should see a default Markdown template for the selected problem type.

  • Edit the problem content as needed and save it.

  • Verify that:

    • The problem renders correctly on the unit page.
    • Clicking the edit (pencil) icon opens the problem in the Markdown editor again.
  • For existing problems, the Switch to Markdown Editor button will only be available if the problem was originally created in the legacy Studio using Markdown.
    Problems created in the authoring MFE using the form-based or advanced editors do not support Markdown yet, as there is currently no OLX-to-Markdown converter. Work on this converter can begin after the Teak release.

Other information

Include anything else that will help reviewers and consumers understand the change.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 10, 2025
@openedx-webhooks

openedx-webhooks commented Apr 10, 2025

Copy link
Copy Markdown

Thanks for the pull request, @Anas12091101!

This repository is currently maintained by @openedx/2u-tnl.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details
Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@@ -0,0 +1,541 @@
const convertMarkdownToXml = (markdown) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this isn't really a new file and it may be too late anyways so I won't block on this, but when adding new files in any of our frontend repos, please use TypeScript .ts[x] instead of .js[x] ! Especially something with this much complicated logic.

@@ -0,0 +1,305 @@
import convertMarkdownToXml from './convertMarkdownToXML';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocker for merge, but new files should be .ts not .js.

};

export const SwitchEditorCardInternal = SwitchEditorCard; // For testing only
export default injectIntl(connect(mapStateToProps, mapDispatchToProps)(SwitchEditorCard));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this file is just renamed, and not a new file, so consider this optional / non-blocking:

injectIntl is deprecated. Please use useIntl instead. openedx/frontend-app-learning#1634

Also, please don't use connect or really any other higher order components like this - instead of mapStateToProps and mapDispatchToProps, use the useSelector and useDispatch hooks respectively.

Once you do this you can likely get rid of SwitchEditorCardInternal as it won't be needed.

(Fine to leave in existing code, but new code should follow the new practices)

Comment on lines +54 to +60
SwitchEditorCard.propTypes = {
intl: intlShape.isRequired,
switchEditor: PropTypes.func.isRequired,
isMarkdownEditorEnabled: PropTypes.bool.isRequired,
problemType: PropTypes.string.isRequired,
editorType: PropTypes.string.isRequired,
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this file is just renamed, and not a new file, so consider this optional / non-blocking:

if (result.length === 1 || !result[1]) {
return label;
}
// xss-lint: disable=javascript-concat-html

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove all these xss-lint comments? We don't have this linter in React/MFE repos and don't need it.

Comment thread src/editors/utils/index.ts Outdated
export { default as removeItemOnce } from './removeOnce';
export { default as formatDuration } from './formatDuration';
export { default as snakeCaseKeys } from './snakeCaseKeys';
export { default as convertMarkdownToXml } from './convertMarkdownToXML';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer require the use of default exports, so you can make this a regular export in convertMarkdownToXML.js and then simplify this to export { convertMarkdownToXml } from './convertMarkdownToXML'; if you want. That's more flexible and consistent. But this is optional.

@Anas12091101
Anas12091101 marked this pull request as ready for review April 10, 2025 18:59
@codecov

codecov Bot commented Apr 10, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.14286% with 10 lines in your changes missing coverage. Please review.

Project coverage is 93.73%. Comparing base (3b2adc2) to head (4e8810f).
Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
...ingsWidget/settingsComponents/SwitchEditorCard.jsx 64.28% 5 Missing ⚠️
...omponents/EditProblemView/SettingsWidget/index.jsx 50.00% 2 Missing ⚠️
...ProblemEditor/components/EditProblemView/index.jsx 75.00% 1 Missing ⚠️
src/editors/data/redux/problem/selectors.ts 50.00% 1 Missing ⚠️
src/editors/utils/convertMarkdownToXML.js 99.62% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1805      +/-   ##
==========================================
+ Coverage   93.63%   93.73%   +0.09%     
==========================================
  Files        1141     1143       +2     
  Lines       23368    23784     +416     
  Branches     5025     5123      +98     
==========================================
+ Hits        21881    22294     +413     
- Misses       1411     1414       +3     
  Partials       76       76              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pdpinch

pdpinch commented Apr 12, 2025

Copy link
Copy Markdown

@Anas12091101 something went wrong with the 2nd screencast. Can you upload it again?

image

@Anas12091101

Copy link
Copy Markdown
Contributor Author

@pdpinch It's showing me fine. I have also checked on a private window:
Screenshot 2025-04-14 at 2 37 23 PM

Re-adding it here:

Screen.Recording.2025-04-10.at.8.15.16.PM.mov

@Anas12091101
Anas12091101 force-pushed the anas/add-markdown-editor branch from 6101797 to f9f13c4 Compare April 14, 2025 10:28
@mphilbrick211 mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Apr 14, 2025
kdmccormick pushed a commit to openedx/openedx-platform that referenced this pull request Apr 15, 2025
This PR adds contentstore.use_react_markdown_editor course waffle flag in cms.
This flag helps in enabling the markdown editor in the authoring mfe.

This PR also adds the markdown_edited field in the Problem Xblock to persist the
user's choice of switching to the markdown editor on the authoring MFE.

More details in the authoring MFE PR: openedx/frontend-app-authoring#1805
@Anas12091101

Copy link
Copy Markdown
Contributor Author

Hi @bradenmacdonald, I wanted to check in on this PR as we’re hoping to get it merged before the Teak's cut. Would you be able to take a look when possible? Happy to address any concerns.

@Anas12091101
Anas12091101 force-pushed the anas/add-markdown-editor branch from f9f13c4 to a6b3daa Compare April 17, 2025 12:35
@bradenmacdonald

bradenmacdonald commented Apr 17, 2025

Copy link
Copy Markdown
Contributor

Hi @Anas12091101, sure I can do the review for you soon. But is there someone from your team who can first test this thoroughly, and give a preliminary approval? I am not enough of a capa expert to know how to test this thoroughly. Also can you please add a couple more tests to increase the coverage to get the build passing? (Or add annotations to skip coverage if things definitely don't need tests). Code coverage checks must be passing or I can't merge it.

And also I should have clarified: Just because I come by a PR and do a review doesn't mean that I'm committing to being the main reviewer for the whole PR and formally approving it. But I can do that for you here if you can help with the testing side, sure.

@Anas12091101

Copy link
Copy Markdown
Contributor Author

@asadali145

New markdown does not have the buttons as we have in the old editor.

We can look into adding those shortcut buttons after we wrap up the Teak work.

But when we edit such a markdown in the new editor, it does not get rendered by the unit page and shows that there are issues with the block

Thanks for catching that! This was due to a bug in the new Markdown-to-XML converter, which I’ve fixed here: a644ff5.
Could you test it again and confirm if the issue is resolved

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, though I do wish you'd added typescript types to the markdown parser code. Just a few small issues I noticed:

Comment thread src/editors/__snapshots__/EditorContainer.test.jsx.snap Outdated
Comment thread src/editors/data/redux/thunkActions/requests.js
@asadali145

Copy link
Copy Markdown
Contributor

Thanks for catching that! This was due to a bug in the new Markdown-to-XML converter, which I’ve fixed here: a644ff5.
Could you test it again and confirm if the issue is resolved

Thanks @Anas12091101, This looks good now.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

I found another bug:

Step Screenshot
1) Edit a multiple choice problem using the visual editor. I did this in a library but it would probably happen in a course too. Screenshot 2025-04-23 at 11 03 10 AM
2) Save changes Screenshot 2025-04-23 at 11 04 05 AM
3) Edit the component again. It has reverted to the advanced editor 🐛. It should still be using the visual editor. Screenshot 2025-04-23 at 11 05 22 AM

From looking at the OLX, the problem is that the first edit adds markdown_edited="false" to the OLX. While this seems to be correct, it also prevents the visual editor from working again. Most likely you just need to add markdown_edited to the list of allowed OLX attributes for the visual editor. I confirmed that removing that attribute causes the visual editor to work again.

@asadali145 Did you test the visual and advanced editors too?

@bradenmacdonald

bradenmacdonald commented Apr 23, 2025

Copy link
Copy Markdown
Contributor

Here is where you need to make the fix (probably ignoredOlxAttributes):

export const settingsOlxAttributes = [
'@_display_name',
'@_weight',
'@_max_attempts',
'@_showanswer',
'@_show_reset_button',
'@_submission_wait_seconds',
'@_attempts_before_showanswer_button',
] as const;
export const ignoredOlxAttributes = [
// '@_markdown', // Not sure if this is safe to ignore; some tests seem to indicate it's not.
'@_url_name',
'@_x-is-pointer-node',
] as const;
- but I notice there is an existing markdown field commented out. Was the field previously named markdown or was it always named markdown_edited ?

Edit: Oh, I see there are two fields - markdown for the markdown and markdown_edited for the boolean. I guess the latter is safe to ignore but if the first is present it should probably skip use of the visual editor? Not sure.

Edit 2: an alternate fix would be to not add markdown_edited="false" when it's false; only add it when it's true.

@Anas12091101

Anas12091101 commented Apr 23, 2025

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald, thanks for identifying this issue! It turns out the problem was specific to libraries and doesn't occur in courses. I’ve added the markdown_edited attribute to the ignoredOlxAttributes, which resolves the issue.

tonybusa pushed a commit to tonybusa/edx-platform that referenced this pull request Apr 23, 2025
This PR adds contentstore.use_react_markdown_editor course waffle flag in cms.
This flag helps in enabling the markdown editor in the authoring mfe.

This PR also adds the markdown_edited field in the Problem Xblock to persist the
user's choice of switching to the markdown editor on the authoring MFE.

More details in the authoring MFE PR: openedx/frontend-app-authoring#1805

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks for making that fix. I'm going to merge this now.

@bradenmacdonald
bradenmacdonald merged commit 380f3be into openedx:master Apr 23, 2025
@github-project-automation github-project-automation Bot moved this from In Eng Review to Done in Contributions Apr 23, 2025
@Anas12091101

Copy link
Copy Markdown
Contributor Author

Thanks so much @bradenmacdonald for all your help throughout. Really appreciate it!

@bradenmacdonald

Copy link
Copy Markdown
Contributor

Thanks for implementing this - it's great to have it (back) :)

UsamaSadiq pushed a commit to openedx/openedx-platform that referenced this pull request May 14, 2025
This PR adds contentstore.use_react_markdown_editor course waffle flag in cms.
This flag helps in enabling the markdown editor in the authoring mfe.

This PR also adds the markdown_edited field in the Problem Xblock to persist the
user's choice of switching to the markdown editor on the authoring MFE.

More details in the authoring MFE PR: openedx/frontend-app-authoring#1805
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants