SAK-51055 T&Q cc+ import issues with embedded image in CKEditor #13423
Conversation
|
@stetsche Since this touched your work on performance improvement on Samigo site copy - I think it needs a review from you. |
There was a problem hiding this comment.
@csev , thanks for the improvements! I left a few comments regarding some context and your changes. Do you think we should do another iteration of performance testing with the added code, or do you think it's really light additions?
CC: @bgarciaentornos
| @@ -453,25 +472,26 @@ public void updateEntityReferences(String toContext, Map<String, String> transve | |||
| continue; | |||
| } | |||
|
|
|||
| // TODO: why are these copyAttachments = false? | |||
There was a problem hiding this comment.
While I can't answer the why definitely, what copyAttachments does, did not happen for instructions and descriptions, so that boolean exists to keep the previous behavior. My focus was on improving performance while maintaining functionality, I didn't want to add more process time, especially without fully understanding this why.
There was a problem hiding this comment.
I feel the same - it looks like copyAttachments makes another copy of embedded things like images attachments and only is used for the questions and answers and not the assessment-level text - so I will follow that pattern as well.
| "feedback" + itemFeedback.getTypeId(), itemContentCache, entrySet, transversalMap, mcx, itemFeedback.getText()); | ||
| log.debug("after migratedText: {}", migratedText); | ||
| itemFeedback.setText(migratedText); | ||
| itemFeedbacksChanged = true; |
There was a problem hiding this comment.
Should we check for a change?
| return false; | ||
| } | ||
|
|
||
| private String migrateTextString(AssessmentService assessmentService, String toContext, String itemHash, |
There was a problem hiding this comment.
I don't fully understand this yet. It looks like you added some code to the part where the migration happens and then moved that into it's own method. But why does that also include the caching part? We would be double checking the caches?
If I understand the changes right, I think migrateTextString should just migrate without checking the cache and migrateText would call it after checking caches.
There was a problem hiding this comment.
Thanks - I will look into that.
There was a problem hiding this comment.
Actually I will move all caching into migrateTextString(). The way I first did it avoided allocating one string that would be quickly deallocated. But making the code simpler and clearer and not duplicating logic is more important. Now migrateText() relies on migrateTextString() completely including all cache processing except for the check for differences, and using the setter and getter. I wanted the instruction text to be cached as well.
There was a problem hiding this comment.
If you move caching to migrateTextString what purpose is left for migrateText? Why do we actually need two methods?
Either way, please ping me when you want me to look at it again.
There was a problem hiding this comment.
Looking at the use of migrateTextString again, it looks like the only difference is that you apply getter and setter manually. If you pass ItemFeedback::getText and ItemFeedback::setText to migrateText should do that for you, also returning the value for of itemFeedbacksChanged.
There are three steps to this change - they are separate commits below:
To do this, I extended the calling sequences for internal methods for updateEntityReferences() and migrateText() so they can handle the "Import from Site" or "merge()" use cases. The method signatures got a little long but I really wanted to avoid duplicating the complex code that goes through and finds all the RTE areas post copy or post merge. It seemed more reliable in the long run the have a few more parameters and if tests that a lot of intricate code duplicated.
@stetsche Since this touched your work on performance improvement on Samigo site copy - I think it needs a review from you.
Comments welcome.