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

Don't truncate notification emails anymore #4830

Merged

Conversation

@Flaburgan
Copy link
Member

commented Mar 6, 2014

Close #4342

@goobertron

This comment has been minimized.

Copy link

commented Mar 6, 2014

Do we need some amended spec for this? Perhaps in spec/helpers/notifier_helper_spec.rb and spec/mailers/notifier_spec.rb?

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2014

@goobertron I just removed the tests which checked that (see the "files changed" tab). Did you think about anything else?

@goobertron

This comment has been minimized.

Copy link

commented Mar 6, 2014

It may just need removal of the word 'truncated' from lines 122 and 152 of spec/mailers/notifier_spec.rb. Unless there are other cases in which truncated posts are sent by email.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2014

Nice catch, I'm gonna remove those.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2014

Green Travis :D

@jhass

This comment has been minimized.

Copy link
Member

commented Mar 6, 2014

Please remove the length option from the calls.

In #4342 you said that we should implement #4266 first and I agree. What made you change your mind?

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2014

@MrZyx because in #4828 I try to interpret html mentions <a href="profile"> etc in the email. But sometimes, the truncate cut the html in the middle, so it is not correct anymore and is displayed broken. As we want to remove the truncate anyway, I did it now.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2014

Length removed from calls.

@jhass

This comment has been minimized.

Copy link
Member

commented Mar 7, 2014

Still, it makes the situation for #4266 worse, I'd feel a lot better if that's fixed first.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2014

Okay, I'm going to adopt this pull request too, but not today.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2014

Let's wait for #4836 to be merged first.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2014

Let's wait for #4836 to be merged first.

Merged!

@Flaburgan Flaburgan force-pushed the Flaburgan:dont-truncate-notification-emails branch 2 times, most recently from 9713aa1 to 4e682d3 Nov 7, 2014

@Flaburgan Flaburgan force-pushed the Flaburgan:dont-truncate-notification-emails branch from 4e682d3 to 25b5bfd Dec 29, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2014

Rebased, this should be ready to be merged.

@svbergerem

This comment has been minimized.

Copy link
Member

commented Jan 4, 2015

Looks good to me. It would be great if someone with more RoR knowledge could review and merge this PR.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2015

Looks good from here too. If no one opposes, after a rebase and commit squash IMHO can merge.

@Flaburgan Flaburgan force-pushed the Flaburgan:dont-truncate-notification-emails branch from 25b5bfd to 48740a6 Jan 10, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2015

Rebased.

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2015

Doh, it's, again, out of sync :P A new rebase, sorry :) Also, could you squash the two commits to one? Will then merge asap before another commit gets in the way.

@Flaburgan Flaburgan force-pushed the Flaburgan:dont-truncate-notification-emails branch from 48740a6 to 46223a2 Jan 10, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2015

Done ;)

@jaywink

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2015

Mergy mergy, thanks @Flaburgan !

jaywink added a commit that referenced this pull request Jan 10, 2015
Merge pull request #4830 from Flaburgan/dont-truncate-notification-em…
…ails

Don't truncate notification emails anymore

@jaywink jaywink merged commit e558562 into diaspora:develop Jan 10, 2015

1 check passed

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

@jaywink jaywink added this to the next-major milestone Jan 10, 2015

@Flaburgan Flaburgan deleted the Flaburgan:dont-truncate-notification-emails branch Oct 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.