Fix images URL in newsletters#10761
Conversation
ahukkanen
left a comment
There was a problem hiding this comment.
Nice PR and thanks a lot for your contribution!
I see one potential problem with this approach because the values are persisted to the database for all records (not only newsletters), see my comment below.
I was thinking that could we instead do some pre-processing on the email template, like finding all the <img> tags in it and injecting the organization hostname and protocol in front of the image paths instead?
We already perform some pre-processing on the generated email through premailer, so I think we could apply similar logic here.
Or alternatively we could maybe utilize the Decidim's content parsers / renderers and replace the images with some placeholders. But I'd imagine that involving a lot more work than the approach mentioned above.
decidim-core/app/controllers/decidim/editor_images_controller.rb
Outdated
Show resolved
Hide resolved
|
Thank you for the review ! I will take a look on how to rewrite relative URLs in I convert this PR to draft and reopen it once implemented |
andreslucena
left a comment
There was a problem hiding this comment.
I tried it out locally and i t works as expected!
I have to admin that I've seen this bug in some newsletters from https://canodrom.barcelona/ but I didn't have the time to invest on the fix... Great to see it fixed already 👏🏽 👏🏽
Sorry for the delay in checking this out. I only have some really minors cosmetic changes and this should be ready to be merged.
We would also need a final check from Antti, but that can be done once you do these last set of changes.
Can you also make a merge with develop just to be sure that everything is alright in the specs? I've done locally and it seems like it was OK.
|
Hello, I planned to apply changes during the week ! |
Perfect! Let me know when this is ready for another round |
675207f to
2796c24
Compare
|
I applied your suggestions, to be more easy to read I forced push to cleanup git history The unique commit contains your change request @andreslucena and also @ahukkanen's changes request Thanks ! About the failing spec "Link PR Format / Check PR title", I think this is not directly related to the title but to the labels right ? |
Yes, it is! You can ignore that, it's a new action that we're still ironing out. |
andreslucena
left a comment
There was a problem hiding this comment.
One final look and this looks good 👏🏽 👏🏽
As @ahukkanen made a first review some time ago, we'll wait a couple of weeks until he has availability to review and merge this. If you don't have time let me know and we'll merge this as is so we can (finally) close this long standing PR.
Thanks for your patience @Quentinchampenois!
|
@Quentinchampenois sorry for the delay on this PR. May I ask you a final merge with Let me know if you don't have the bandwidth, I can create another PR based on this one. Thanks for your help and patience in this issue. |
4407cd6 to
937fa0a
Compare
Antti did not have time to review it in these months so I'll take it out of his hands
andreslucena
left a comment
There was a problem hiding this comment.
Just tried it out one last time and it works great. Code-wise is 💯
Thanks for taking care of this issue @Quentinchampenois 👏🏽 👏🏽
🎩 What? Why?
Please describe your pull request.
Images uploaded with the WYSWYG editor are not accessible out of the application like images uploaded in newsletter.
At the moment,
EditorImagesController#createreturns the path of the newly uploaded image. Source will be correctly resolved from the application, however we should return a full URL for external resources like newsletter.This PR allows to rewrite source url in
imgHTML tags of newsletters content on the fly. Uploaded images url will be stored as relative path in database, and organization host with protocol will be interpolated on each image of the content just before being sent.📌 Related Issues
Link your PR to an issue
Testing
Describe the best way to test or validate your PR.