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

Remove limited content from email notifications #5494

Merged
merged 1 commit into from Dec 29, 2014

Conversation

Projects
None yet
4 participants
@margori
Contributor

margori commented Dec 24, 2014

This is my solution to pull request #4508 that solves issue #4342 and #4266

I have doubts in messages 'notifier.a_private_message' and 'notifier.a_limited_post_comment' due to I'm not a native English speaker.

I also updated 'spec/mailers/notifier_spec.rb' file 'cause now if post is public or not changes email text, but I don't know if this file requires new tests for private mails. What do you think?

@goobertron

This comment has been minimized.

Show comment
Hide comment
@goobertron

goobertron Dec 24, 2014

I have doubts in messages 'notifier.a_private_message' and 'notifier.a_limited_post_comment' due to I'm not a native English speaker.

I'll look at them now.

I have doubts in messages 'notifier.a_private_message' and 'notifier.a_limited_post_comment' due to I'm not a native English speaker.

I'll look at them now.

@goobertron

This comment has been minimized.

Show comment
Hide comment
@goobertron

goobertron Dec 24, 2014

In terms of the English, they read fine, except for notifier.a_limited_post_comment, I'd say "There's a new comment on your limited post in diaspora* to check out." (move 'to check out' and use 'on' instead of 'in').

In terms of style/content, I think 'for you' reads better than 'to check out' (which is a bit functional); however, the private message/comment will probably not always be for you (rather than someone else), so it's not exactly appropriate here. In fact, 'for you to check out' reads better, and doesn't imply that the message/comment is necessarily for you. What do others think? Any better ideas?

One other thing, is notifier.a_limited_post_comment only for comments on posts the user themselves has made, or does it also apply to a comment made on a post by someone else which the user has previously commented on? If the latter, the wording will need to be changed to read 'on a limited post' rather than 'on your limited post'.

So I'd suggest:

a_private_message: "There's a new private message in diaspora* for you to check out."
a_limited_post_comment: "There's a new comment on a limited post in diaspora* for you to check out."

I can't comment on your code, I'm afraid, but thanks for doing this; and I hope my comments on the English are helpful.

In terms of the English, they read fine, except for notifier.a_limited_post_comment, I'd say "There's a new comment on your limited post in diaspora* to check out." (move 'to check out' and use 'on' instead of 'in').

In terms of style/content, I think 'for you' reads better than 'to check out' (which is a bit functional); however, the private message/comment will probably not always be for you (rather than someone else), so it's not exactly appropriate here. In fact, 'for you to check out' reads better, and doesn't imply that the message/comment is necessarily for you. What do others think? Any better ideas?

One other thing, is notifier.a_limited_post_comment only for comments on posts the user themselves has made, or does it also apply to a comment made on a post by someone else which the user has previously commented on? If the latter, the wording will need to be changed to read 'on a limited post' rather than 'on your limited post'.

So I'd suggest:

a_private_message: "There's a new private message in diaspora* for you to check out."
a_limited_post_comment: "There's a new comment on a limited post in diaspora* for you to check out."

I can't comment on your code, I'm afraid, but thanks for doing this; and I hope my comments on the English are helpful.

@margori

This comment has been minimized.

Show comment
Hide comment
@margori

margori Dec 24, 2014

Contributor

Thanks, Goobertron. Let's wait for a couple of comment more to decide about those messages.

Contributor

margori commented Dec 24, 2014

Thanks, Goobertron. Let's wait for a couple of comment more to decide about those messages.

Show outdated Hide outdated app/helpers/notifier_helper.rb
@@ -15,6 +17,10 @@ def post_message(post, opts={})
# @param opts [Hash] Optional hash. Accepts :length parameters.
# @return [String] The truncated and formatted comment.
def comment_message(comment, opts={})
comment.message.plain_text_without_markdown truncate: opts.fetch(:length, 600)
if !comment.post.public?

This comment has been minimized.

@jhass

jhass Dec 27, 2014

Member

Please swap the if bodies here.

@jhass

jhass Dec 27, 2014

Member

Please swap the if bodies here.

@margori

This comment has been minimized.

Show comment
Hide comment
@margori

margori Dec 29, 2014

Contributor

@goobertron Your suggestion included.

@jhass Requested swap made.

Contributor

margori commented Dec 29, 2014

@goobertron Your suggestion included.

@jhass Requested swap made.

@jhass jhass added this to the next-major milestone Dec 29, 2014

@jhass jhass merged commit fe60528 into diaspora:develop Dec 29, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

jhass added a commit that referenced this pull request Dec 29, 2014

Merge pull request #5494 from margori/4266-remove-content-from-email-…
…notifications

Remove limited content from email notifications
@jhass

This comment has been minimized.

Show comment
Hide comment
@jhass

jhass Dec 29, 2014

Member

Awesome, thank you!

Member

jhass commented Dec 29, 2014

Awesome, thank you!

@Flaburgan

This comment has been minimized.

Show comment
Hide comment
@Flaburgan

Flaburgan Dec 29, 2014

Member

Thanks! I'll rebase #4830 tonight, it should be ready to be merged soon.

Member

Flaburgan commented Dec 29, 2014

Thanks! I'll rebase #4830 tonight, it should be ready to be merged soon.

@margori margori deleted the margori:4266-remove-content-from-email-notifications branch Dec 29, 2014

@margori

This comment has been minimized.

Show comment
Hide comment
@margori

margori Dec 29, 2014

Contributor

Welcome, boys and girls. This is a real pleasure.

Contributor

margori commented Dec 29, 2014

Welcome, boys and girls. This is a real pleasure.

@goobertron

This comment has been minimized.

Show comment
Hide comment
@goobertron

goobertron Dec 29, 2014

Thanks from me too. It's great to have you as part of the team, and I hope you continue coding for Diaspora!

Thanks from me too. It's great to have you as part of the team, and I hope you continue coding for Diaspora!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment