-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Save changes button displayed for new course without changes in settings #1265
Comments
Hi @jmakowski1123, @mariajgrimaldi, @KristinAoki, @arbrandes, @jesperhodge Could you kindly pay attention to this issue at your earliest convenience? Thank you! |
The default course overview comes from https://github.com/openedx/edx-platform/blob/master/xmodule/templates/about/overview.yaml, and the images therein are hosted in edx-platform itself. This raises a few questions:
@KristinAoki, do you happen to have some insight into any of these? |
Because the AboutBlock is a subclass of But a difference between the server side and the TinyMCE code is that the server side is smart enough to say, "Do this substitution if that static asset actually exists locally in the course, otherwise leave the link alone." The TinyMCE editor hook is not smart enough to do that.
It's because the TinyMCEWidget is trying to give you a WYSIWYG experience, so it needs to convert the URLs to something that it thinks will actually work while they're in the editor (and then quickly convert them back to the "/static/..." text when saving).
Honestly, the easiest way out of this is likely to get rid of the default image and let people add ones from their course if they care.
I'm not sure exactly what's meant here, but there is a thing where we try to auto-convert them into the "/static/..." form expected in OLX in the event that people accidentally put in full links to course static assets. Because that's the form that's going to be the most portable when they export this course and import it somewhere else, do a re-run, etc. |
Oh, but TinyMCE is supposed to convert it back regardless, but I bet it's breaking when it's trying to convert from its "preview" form to it's "write this back to disk" form because it doesn't know how to deal with "/" in the path:
That's not a valid AssetKey. I think if it substitutes a "_" for the "/", it'll at least save in a form that won't be broken, and that will render properly in Studio and the LMS (even if preview in the editor will be broken). |
Ack, but then TinyMCE has no good way of knowing how to map it back to the actual form with slashes... Okay, so maybe the fix here is to make TinyMCE smarter and actually try to look up the asset to see if its transformed form exists before it decides to do that transformation? |
@ormsbee thank you for your explanations to questions 1 and 2. @arbrandes regarding question 3, the default overview has images present because that is what is in the template from the legacy experience. Regarding question 4, the ability to edit the html source is because of how the editor is rendered. The code for the WYSIWYG experience prevents the "HTML" button from appearing on this page. |
Hi @KristinAoki @ormsbee! |
Reproduce
contentstore.new_studio_mfe.use_new_schedule_details_page
Expected Result
The "You've made some changes" message with the Save button should not appear if no changes were made in the Schedule & Details section. It's important to note that no changes should be present, as this is a newly created course.
Investigation
I have identified the cause behind the appearance of the window with the save button:
This determines whether there are any changes in the edit fields for course information (hooks.jsx):
For the
Course overview
field, initially, the template fragments were not equal:The difference is (this is another bug related to this issue):
src="/asset-v1:RG+dima-visibility+2024+type@asset+block@images/placeholder-faculty.png"
src="/static/images/placeholder-faculty.png"
This behavior was not present in the Redwood.1 release, but appeared in Redwood.2.
I have concluded those recent changes in the frontend-lib-content-components library have impacted the Course Authoring MFE.
The replaceStaticWithAsset function is responsible for path transformation. If I understand correctly, the changes in this MR affected the images in the Course Overview field.
Since
frontend-lib-content-components
is deprecated, no further modifications can be made to it. I checked the master branch, and the same bug exists there, as the code from this library was migrated to Course Authoring MFE. However, I am unable to fix this bug as the functionality is quite complex, and my frontend expertise is limited.@arbrandes @KristinAoki Could you assist me in resolving this issue?
The text was updated successfully, but these errors were encountered: