[MAGNOLIA-2088] Images in Rich Editor Paragraphs Created before 3.5 No Longer Editable Created: 17/Mar/08  Updated: 23/Jan/13  Resolved: 20/Mar/08

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: 3.5.4
Fix Version/s: 3.5.5

Type: Bug Priority: Major
Reporter: Sean McMains Assignee: Magnolia International
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File 2088.diff     Text File MAGNOLIA-2088.patch     Text File MAGNOLIA-2088.patch    
Issue Links:
supersession
is superseded by MAGNOLIA-2092 Process website content and replace r... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

To reproduce:

  • Create a Rich Editor paragraph in v3.0.5 of Magnolia. Include an image.
  • The path is recorded as "rich/contentParagraph/0/content_files/file/Picture 2.png" or similar.
  • Bootstrap that content into v3.5.4 of Magnolia.
  • The image displays correctly on the page.
  • Edit that paragraph.

Expected behavior:

  • The image should display and be editable in FCK Editor normally.

Actual behavior:

  • The image appears broken.


 Comments   
Comment by Sean McMains [ 17/Mar/08 ]

Here's a diff of the old code added back in to make this work correctly with old data.

Comment by Sean McMains [ 17/Mar/08 ]

I've added a diff that adds back in the code responsible for handling the old-style links. Unfortunately, it breaks FckEditorDialogTest.testConvertToViewShouldNotConvertNonUUIDLinks, as it must. (I'm just disabling that test in our build for the moment.)

Comment by Sean McMains [ 18/Mar/08 ]

I should point out that, since Magnolia now removes files that are no longer referenced, data will be lost if editors go in and edit this existing content. Getting a fix for this is therefore fairly urgent for us.

Comment by Magnolia International [ 18/Mar/08 ]

Sean, thanks for the patch, but doesn't it lack some sort of check on the return value of LinkUtil.convertUUIDsToEditorLinks before it proceeds ?

Comment by Sean McMains [ 18/Mar/08 ]

Possibly, though I was just basing this on what was in 3.0.5, which didn't have any kind of check on the return value from LinkUtil convertUUIDsToEditorLinks().

What sort of check would you see as valuable? Just making sure we got back a non-null value?

Comment by Magnolia International [ 18/Mar/08 ]

Well, as the failing test points out with this slightly corrected version of your patch, the link translation isn't correct anymore, at least for "new" links. Since your fix is supposedly only for 3.0-based content, I assume the portion of code it adds should be bypassed when the content has been added/edited with 3.5.

Comment by Sean McMains [ 18/Mar/08 ]

Thanks for your help and feedback with this. Please forgive me if I'm being obtuse here, but the only tests I'm seeing fail are those in testConvertToViewShouldNotConvertNonUUIDLinks. These tests don't seem to have to do with "new" links, which will use the UUID pattern.

This test seems to explicitly test for non-backwards compatibility by making sure that convertToView() doesn't touch anything except the UUID links. But for backward compatibility, we have to adjust those links. Thus, those tests don't appear to be valid when we're trying to get the old-style links working correctly.

Or am I misunderstanding something?

Comment by Magnolia International [ 18/Mar/08 ]

No, that seems to be the case indeed, sorry. There seems to be a trailing slash missing after the context path though. Probably in your case, the path property was not set, so you didn't notice it... ?

Comment by Sean McMains [ 18/Mar/08 ]

This also appears to be an issue with the test harness, actually. When running the tests, MgnlContext.getContextPath() returns "some-context" (with no slash). When running as a web app, however, I get back "/magnolia-empty-webapp-3.5.4".

Likewise, this.getTopParent().getConfigValue("path") returns "here" in test mode, and "/testing-site-destroyer/paragraph-examples/rich" (with an initial slash) when it's running as a web app.

I think the default values for the test harness should probably be updated with initial slashes too.

Comment by Magnolia International [ 18/Mar/08 ]

yes, i added the leading slash locally to context path locally - will also double check what the use of the path property is.

Comment by Sean McMains [ 18/Mar/08 ]

I went ahead and checked in your patch to our local system and have had our QA and customer service people pounding on it. They're reporting good results all around, so we'll probably deploy this fix in the next day or so once we're satisfied that it has been burned in sufficiently.

Comment by Magnolia International [ 20/Mar/08 ]

patch applied on trunk. Thanks a lot for reporting and fixing !

Generated at Mon Feb 12 03:33:18 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.