Skip to content
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

Open
DmytroAlipov opened this issue Sep 7, 2024 · 7 comments
Labels
bug Report of or fix for something that isn't working as intended release testing Affects the upcoming release (attention needed)

Comments

@DmytroAlipov
Copy link
Contributor

DmytroAlipov commented Sep 7, 2024

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:

  • edited value:
<section class="about">
<h2>About This Course</h2>
<p>Include your long course description here. The long course description should contain 150-400 words.</p>
<p>This is paragraph 2 of the long course description. Add more paragraphs as needed. Make sure to enclose them in paragraph tags.</p>
</section>
<section class="prerequisites">
<h2>Requirements</h2>
<p>Add information about the skills and knowledge students need to take this course.</p>
</section>
<section class="course-staff">
<h2>Course Staff</h2>
<article class="teacher">
<div class="teacher-image"><img src="/asset-v1:RG+dima-visibility+2024+type@asset+block@images/placeholder-faculty.png" align="left" alt="Course Staff Image #1" /></div>
<h3>Staff Member #1</h3>
<p>Biography of instructor/staff member #1</p>
</article>
<article class="teacher">
<div class="teacher-image"><img src="/asset-v1:RG+dima-visibility+2024+type@asset+block@images/placeholder-faculty.png" align="left" alt="Course Staff Image #2" /></div>
<h3>Staff Member #2</h3>
<p>Biography of instructor/staff member #2</p>
</article>
</section>
<section class="faq">
<section class="responses">
<h2>Frequently Asked Questions</h2>
<article class="response">
<h3>What web browser should I use?</h3>
<p>The Open edX platform works best with current versions of Chrome, Edge, Firefox, or Safari.</p>
<p>See our <a href="https://edx.readthedocs.org/projects/open-edx-learner-guide/en/latest/front_matter/browsers.html">list of supported browsers</a> for the most up-to-date information.</p>
</article>
<article class="response">
<h3>Question #2</h3>
<p>Your answer would be displayed here.</p>
</article>
</section>
</section>

  • it getting from the backend
<section class="about">
  <h2>About This Course</h2>
  <p>Include your long course description here. The long course description should contain 150-400 words.</p>

  <p>This is paragraph 2 of the long course description. Add more paragraphs as needed. Make sure to enclose them in paragraph tags.</p>
</section>

<section class="prerequisites">
  <h2>Requirements</h2>
  <p>Add information about the skills and knowledge students need to take this course.</p>
</section>

<section class="course-staff">
  <h2>Course Staff</h2>
  <article class="teacher">
    <div class="teacher-image">
      <img src="/static/images/placeholder-faculty.png" align="left" alt="Course Staff Image #1" />
    </div>

    <h3>Staff Member #1</h3>
    <p>Biography of instructor/staff member #1</p>
  </article>

  <article class="teacher">
    <div class="teacher-image">
      <img src="/static/images/placeholder-faculty.png" align="left" alt="Course Staff Image #2" />
    </div>

    <h3>Staff Member #2</h3>
    <p>Biography of instructor/staff member #2</p>
  </article>
</section>

<section class="faq">
  <section class="responses">
    <h2>Frequently Asked Questions</h2>
    <article class="response">
      <h3>What web browser should I use?</h3>
      <p>The Open edX platform works best with current versions of Chrome, Edge, Firefox, or Safari.</p>
      <p>See our <a href="https://edx.readthedocs.org/projects/open-edx-learner-guide/en/latest/front_matter/browsers.html">list of supported browsers</a> for the most up-to-date information.</p>
    </article>

    <article class="response">
      <h3>Question #2</h3>
      <p>Your answer would be displayed here.</p>
    </article>
  </section>
</section>

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 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?

@DmytroAlipov DmytroAlipov added the bug Report of or fix for something that isn't working as intended label Sep 7, 2024
@DmytroAlipov
Copy link
Contributor Author

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

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

Thank you!

@mariajgrimaldi mariajgrimaldi added the release testing Affects the upcoming release (attention needed) label Oct 15, 2024
@arbrandes
Copy link
Contributor

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
Copy link
Contributor

ormsbee commented Oct 23, 2024

@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
Copy link
Contributor

ormsbee commented Oct 23, 2024

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
Copy link
Contributor

ormsbee commented Oct 23, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended release testing Affects the upcoming release (attention needed)
Projects
Status: Backlog
Development

No branches or pull requests

5 participants