Skip to content

Email option to embed images as base64 instead of link #32061

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

Merged
merged 17 commits into from
Mar 5, 2025

Conversation

sommerf-lf
Copy link
Contributor

@sommerf-lf sommerf-lf commented Sep 17, 2024

ref: #15081
ref: #14037

Documentation: https://gitea.com/gitea/docs/pulls/69

Example

Content:
image
Result in Email:
image
Result with source code:
(first image is external image, 2nd is now embedded)
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 17, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code docs-update-needed The document needs to be updated synchronously labels Sep 17, 2024
@pull-request-size pull-request-size bot added size/M and removed size/L labels Sep 17, 2024
@sommerf-lf sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch 2 times, most recently from 6bfebf7 to 17439b9 Compare September 18, 2024 09:16
@sommerf-lf
Copy link
Contributor Author

sommerf-lf commented Sep 18, 2024

Please have a look at this and let me know if I did some structural things wrong, as I have zero experience with Go but came across the mentioned issues myself and tried to add the optional setting to fix them.

@sommerf-lf sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch 2 times, most recently from cc35853 to 37428d3 Compare September 18, 2024 15:02
@sommerf-lf sommerf-lf marked this pull request as ready for review September 18, 2024 15:17
@sommerf-lf sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch from 37428d3 to d76e949 Compare September 18, 2024 20:14
@lunny
Copy link
Member

lunny commented Sep 19, 2024

Could you have some tests?

@sommerf-lf
Copy link
Contributor Author

Could you have some tests?

I'll have a look. But I'm not quite sure as to what the focus of these test should be, as outgoing emails aren't tested anywhere. I could simply test my own additions whithout any actual email integration (the parsing of the html and replacement of the img tags and whether other image tags stay the same). Sounds fine?

@lunny
Copy link
Member

lunny commented Sep 19, 2024

Could you have some tests?

I'll have a look. But I'm not quite sure as to what the focus of these test should be, as outgoing emails aren't tested anywhere. I could simply test my own additions whithout any actual email integration (the parsing of the html and replacement of the img tags and whether other image tags stay the same). Sounds fine?

Yes. Thank you.

@sommerf-lf sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch from d76e949 to 49c1522 Compare September 20, 2024 17:18
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 20, 2024
@sommerf-lf sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch from 49c1522 to d072c3e Compare September 20, 2024 17:22
@sommerf-lf
Copy link
Contributor Author

I added test to this. however I can't integrate them into services/mailer/mail_test.go because I need the testing environment which would form an import cycle. Now I exported the MailCommentContext and my newly added functions. Let me know if there is a way around this, but I see this as nothing big as these functions are not context specific. I'll squash the commits after you reviewed it

@sommerf-lf sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch 2 times, most recently from 763b655 to 6814a96 Compare September 20, 2024 21:51
@sommerf-lf
Copy link
Contributor Author

Still fails because I didn't account for using minio. Will rework...

@sommerf-lf sommerf-lf changed the title Email option to embed images as base64 instead of link [WIP] Email option to embed images as base64 instead of link Sep 21, 2024
@sommerf-lf sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch from cd3fea1 to 1665cba Compare September 22, 2024 12:26
@sommerf-lf sommerf-lf changed the title [WIP] Email option to embed images as base64 instead of link Email option to embed images as base64 instead of link Sep 22, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 22, 2024
@wxiaoguang
Copy link
Contributor

Will do some refactoring first before continuing this. Thank you for your patience.

@wxiaoguang wxiaoguang force-pushed the feature_B64_EMBED_IMAGES branch 2 times, most recently from 7cd0f0e to f3d6e2d Compare March 3, 2025 04:17
@wxiaoguang wxiaoguang force-pushed the feature_B64_EMBED_IMAGES branch from f3d6e2d to 599b78d Compare March 3, 2025 04:21
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 5, 2025
@wxiaoguang wxiaoguang marked this pull request as ready for review March 5, 2025 02:18
@wxiaoguang wxiaoguang added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Mar 5, 2025
@wxiaoguang
Copy link
Contributor

Made some changes:

  • Only keep one "EMBED_ATTACHMENT_IMAGES" config option, the size limit is hard-coded to 9MB. I think this limit is good enough to most users, and avoid bloating Gitea's config system too much.
    • If there are special requests, there is always a chance to add the "limit" config option back.
  • Instead of "guessing" the "attachments" path, use a strict parser to parse Gitea's route path

@wxiaoguang wxiaoguang added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 5, 2025
@lunny lunny enabled auto-merge (squash) March 5, 2025 16:05
@lunny lunny merged commit 7cdde20 into go-gitea:main Mar 5, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 5, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 6, 2025
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Refactor: move part of updating protected branch logic to service layer (go-gitea#33742)
  Update changelog for v1.23.5 (go-gitea#33797)
  Email option to embed images as base64 instead of link (go-gitea#32061)
  Update TypeScript types (go-gitea#33799)
  Disable vet=off again (go-gitea#33794)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-update-needed The document needs to be updated synchronously lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants