Skip to content
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

Improve email notification text #5321

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timothyhay256
Copy link

@timothyhay256 timothyhay256 commented Jan 12, 2025

Fixes #5039 and #3361

When in conjunction with LemmyNet/lemmy-translations#154 this improves the message for email notifications for a few fields.

If I am understanding everything correctly, this shouldn't be merged until translations are complete, atleast for the languages in crates/utils/build.rs, since Rosetta will fail to build otherwise.

The description from the lemmy-translations PR is below:

Using a println!("{body"}); inside send_email_to_user() to show the HTML that would be sent to the user.

notification_post_reply_body :
<h1>Post reply</h1><br><div>{username} - {comment_text}</div><br><a href=\"{inbox_link}\">inbox</a> to
<h1>Post reply</h1><br><div>{username} replied to \"{post_title}\": {comment_text}</div><br><a href=\"{comment_link}\">comment</a> <a href=\"{inbox_link}\">inbox</a>
Resulting in the following:
image

notification_comment_reply_body:
<h1>Comment reply</h1><br><div>{username} - {comment_text}</div><br><a href=\"{inbox_link}\">inbox</a> to
<h1>Comment reply</h1><br><div>{username} replied to \"{parent_comment_text}\" on \"{post_title}\": {comment_text}</div><br><a href=\"{comment_link}\">comment</a> <a href=\"{inbox_link}\">inbox</a>
Resulting in the following:
image

notification_mentioned_by_body:
<h1>Person Mention</h1><br><div>{username} - {comment_text}</div><br><a href=\"{inbox_link}\">inbox</a to
<h1>Person Mention</h1><br><div>{username} mentioned you: {comment_text}</div><br><a href=\"{comment_link}\">comment</a> <a href=\"{inbox_link}\">inbox</a>
image

notification_private_message_body:
<h1>Private message</h1><br><div>{username} - {message_text}</div><br><a href=\"{inbox_link}\">inbox</a> to
<h1>Private message</h1><br><div>{username} private messaged you: {message_text}</div><br><a href=\"{inbox_link}\">inbox</a>
Resulting in the following:
image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming I should change this to reference the commit that actually changes en.json once 154 is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesnt really matter, it gets updated to the latest commit automatically with each release. You only need to get the CI checks passing.

@Nutomic
Copy link
Member

Nutomic commented Jan 13, 2025

It currently fails to compile because some translation parameters are wrong. You should also see it if you run cargo check locally.

https://woodpecker.join-lemmy.org/repos/129/pipeline/11222/11#L815

@timothyhay256
Copy link
Author

It currently fails to compile because some translation parameters are wrong. You should also see it if you run cargo check locally.

https://woodpecker.join-lemmy.org/repos/129/pipeline/11222/11#L815

I ensured that every place that uses notification_post_reply_body has every argument, so correct me if I am wrong, isn't the build failing because the fallback language has parameters that the build languages don't have? I am referencing this line in the Rosetta docs: "Languages that are not fallback languages must have the same parameters as the fallback language."
In which case the languages that are being built in build.rs need translations before it will successfully build.

I tested disabling the other languages in build.rs and it was able to build successfully.

@Nutomic
Copy link
Member

Nutomic commented Jan 14, 2025

That makes sense, but anyway the tests have to pass before we can merge. I checked in weblate but there is no option to force update/delete these old translations. So I think the best we can do is wait a while for them to be updated, and delete any remaining old strings.

@dessalines
Copy link
Member

@timothyhay256 the lemmy-translations got merged, so you might need to update that again? Otherwise you might need to do a PR there to remove the broken translation strings for the languages you don't speak, that break CI. Someone will re-add them in weblate in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add context to email notification
3 participants