[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: |
|
||||||||
| Issue Links: |
|
||||||||
| 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:
Expected behavior:
Actual behavior:
|
| 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 ! |