openedx / frontend-app-authoring

Front-end for the Open edX Studio experience, implemented in React and Paragon.
GNU Affero General Public License v3.0
14 stars 78 forks source link

Save changes button displayed for new course without changes in settings #1265

Open DmytroAlipov opened 2 months ago

DmytroAlipov commented 2 months ago

Reproduce

  1. Enablecontentstore.new_studio_mfe.use_new_schedule_details_page
  2. Create course in Studio
  3. Open Settings -> Schedule & details 123
  4. You can also see a broken placeholder image for the Course overview field

1234

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):

    if (editedValues[fieldName] !== value) {
      setEditedValues((prevEditedValues) => ({
        ...prevEditedValues,
        [fieldName]: value || '',
      }));

      if (!showModifiedAlert) {
        setShowModifiedAlert(true);
      }
    }

For the Course overview field, initially, the template fragments were not equal:

Requirements

Add information about the skills and knowledge students need to take this course.

Course Staff

Course Staff Image #1

Staff Member #1

Biography of instructor/staff member #1

Course Staff Image #2

Staff Member #2

Biography of instructor/staff member #2

Frequently Asked Questions

What web browser should I use?

The Open edX platform works best with current versions of Chrome, Edge, Firefox, or Safari.

See our list of supported browsers for the most up-to-date information.

Question #2

Your answer would be displayed here.


---

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.
- Course Authoring MFE Redwood.1 -> "@edx/frontend-lib-content-components": "^2.1.11"
- Course Authoring MFE Redwood.2 -> "@edx/frontend-lib-content-components": "^2.5.1"

I have concluded those recent changes in the [frontend-lib-content-components](https://github.com/openedx-unsupported/frontend-lib-content-components) library have impacted the Course Authoring MFE.

The [replaceStaticWithAsset](https://github.com/openedx-unsupported/frontend-lib-content-components/blob/a4f5d03837380f1640e3455e523f188e1408295d/src/editors/sharedComponents/TinyMceWidget/hooks.js#L74) function is responsible for path transformation. If I understand correctly, the changes in [this MR](https://github.com/openedx-unsupported/frontend-lib-content-components/pull/484) 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?
DmytroAlipov commented 2 months ago

Hi @jmakowski1123, @mariajgrimaldi, @KristinAoki, @arbrandes, @jesperhodge

Could you kindly pay attention to this issue at your earliest convenience?

Thank you!

arbrandes commented 1 month ago

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:

  1. Why is TinyMCEWidget trying to replace those links with course-specific ones, when that will never work for those links in particular?
  2. Why is TinyMCEWidget automatically modifying default content on initial render at all? That doesn't seem to make much sense. It's ultimately why the save notice is showing.
  3. Why does the default course overview content contain images in the first place, given that it's not possible to add them via TinyMCEWidget on the details page? Not even via editing the HTML source, which, by the way, seems to be broken. See the next item:
  4. Why is it not possible to edit the HTML source via TinyMCEWidget on the details page, and have the changes persist?

@KristinAoki, do you happen to have some insight into any of these?

ormsbee commented 1 month ago

@arbrandes:

  1. Why is TinyMCEWidget trying to replace those links with course-specific ones, when that will never work for those links in particular?

Because the AboutBlock is a subclass of HTMLBlock, and inherits all the magic it uses to auto-convert static URL references, as well as its assumptions about how it's expected to be used with staff-uploadable assets.

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.

  1. Why is TinyMCEWidget automatically modifying default content on initial render at all? That doesn't seem to make much sense. It's ultimately why the save notice is showing.

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).

  1. Why does the default course overview content contain images in the first place, given that it's not possible to add them via TinyMCEWidget on the details page?

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.

  1. Why is it not possible to edit the HTML source via TinyMCEWidget on the details page, and have the changes persist?

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.

ormsbee commented 1 month ago

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:

src="/asset-v1:RG+dima-visibility+2024+type@asset+block@images/placeholder-faculty.png"

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).

ormsbee commented 1 month ago

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?

KristinAoki commented 1 month ago

@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.

DmytroAlipov commented 2 weeks ago

Hi @KristinAoki @ormsbee! I wanted to follow up and ask about the recent progress in solving the bug we discussed. Any updates would be greatly appreciated. Thank you!