WIP - [Fixed Bug 703316] - Invitation email is impersonal #791

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
@jonathan-s
Contributor

jonathan-s commented Jan 30, 2014

No description provided.

@dpoirier

This comment has been minimized.

Show comment
Hide comment
@dpoirier

dpoirier Jan 30, 2014

Contributor

I think this'll need to be a little more involved, unfortunately. The from address passed to send_mail() is used as the envelope from address, and should be simply an email address. To set the contents of the From header, we won't be able to use the Django send_mail(). Instead, we can construct an EmailMessage and pass it in the headers.

Contributor

dpoirier commented Jan 30, 2014

I think this'll need to be a little more involved, unfortunately. The from address passed to send_mail() is used as the envelope from address, and should be simply an email address. To set the contents of the From header, we won't be able to use the Django send_mail(). Instead, we can construct an EmailMessage and pass it in the headers.

@jonathan-s

This comment has been minimized.

Show comment
Hide comment
@jonathan-s

jonathan-s Jan 30, 2014

Contributor

I'll take a look at it!

Contributor

jonathan-s commented Jan 30, 2014

I'll take a look at it!

@glogiotatidis

View changes

mozillians/phonebook/models.py
@@ -40,9 +40,10 @@ def send(self, sender=None):
"""
if sender:
+ from_reply = sender.full_name + " via " + settings.FROM_NOREPLY

This comment has been minimized.

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

We use ' instead of " in python code, unless there is a reason not to. Please change this for consistency.

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

We use ' instead of " in python code, unless there is a reason not to. Please change this for consistency.

@glogiotatidis

View changes

mozillians/phonebook/models.py
@@ -40,9 +40,10 @@ def send(self, sender=None):
"""
if sender:

This comment has been minimized.

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

If there is no sender, then from_reply will not be set and the send_mail will fail. Please add
from_reply = settings.FROM_NOREPLY just above this line to set a default value.

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

If there is no sender, then from_reply will not be set and the send_mail will fail. Please add
from_reply = settings.FROM_NOREPLY just above this line to set a default value.

@glogiotatidis

View changes

mozillians/templates/phonebook/invite_email.txt
@@ -1,5 +1,7 @@
{% trans sender=sender %}
-Hi there. {{ sender }} has invited you to join mozillians.org,
+Hi there,

This comment has been minimized.

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

Localization tools will strip any newlines in trans blocks, thus this block will be send as a single line.

To keep the formating, break this into multiple blocks like this

{{ _('Hi there') }},

{% trans sender=sender %}
{{ sender }} has invited you to join mozillians.org,
the community directory for Mozilla contributors. You
can create a community profile for yourself and search
for other contributors to learn more about them or get
in touch.
{% endtrans %}
@glogiotatidis

glogiotatidis Jan 30, 2014

Member

Localization tools will strip any newlines in trans blocks, thus this block will be send as a single line.

To keep the formating, break this into multiple blocks like this

{{ _('Hi there') }},

{% trans sender=sender %}
{{ sender }} has invited you to join mozillians.org,
the community directory for Mozilla contributors. You
can create a community profile for yourself and search
for other contributors to learn more about them or get
in touch.
{% endtrans %}
@glogiotatidis

View changes

mozillians/templates/phonebook/invite_email.txt
@@ -13,4 +15,7 @@ in touch.
{{ personal_message }}
{% endif %}
-{{ _('Join Mozillians:') }} {{ link }}
+{{ _('Take two minutes to create your mozillians.org profile:') }}

This comment has been minimized.

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

Add an extra blank line before this line to separate the personal message.

Additionally we can add a filter to indent in the contents of personal message so it's more distinguishable.

Try
{{ personal_message|indent(indentfirst=True) }}

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

Add an extra blank line before this line to separate the personal message.

Additionally we can add a filter to indent in the contents of personal message so it's more distinguishable.

Try
{{ personal_message|indent(indentfirst=True) }}

This comment has been minimized.

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

nit: you have an extra space at the end of this line. You may want to configure your editor to remove whitespace.

@glogiotatidis

glogiotatidis Jan 30, 2014

Member

nit: you have an extra space at the end of this line. You may want to configure your editor to remove whitespace.

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Jan 30, 2014

Member

Always use present tense in your commit messages, Fix bug XXX and a message that represents the change.
e.g. [fix bug 703316] Personalize invitation message.

Member

glogiotatidis commented Jan 30, 2014

Always use present tense in your commit messages, Fix bug XXX and a message that represents the change.
e.g. [fix bug 703316] Personalize invitation message.

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Jan 30, 2014

Member

@jonathan-s thanks for the pull request! I added some comments. You can make the changes and amend to your current commit, so we have only one commit to merge in for this bug.

Feel free to reply here or ping me on mozilla's IRC (giorgos) in #commtools if you have questions and / or new suggestions

Member

glogiotatidis commented Jan 30, 2014

@jonathan-s thanks for the pull request! I added some comments. You can make the changes and amend to your current commit, so we have only one commit to merge in for this bug.

Feel free to reply here or ping me on mozilla's IRC (giorgos) in #commtools if you have questions and / or new suggestions

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Jan 30, 2014

Member

@dpoirier i'm not sure i follow you. from_reply is set just fine here. Here is an example

MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Please join me on mozillians.org
From: 4979c8ade727d501f345c0dc06eb3277d9bc9be2 via Mozillians  <no-reply@mozillians.org>

can you please elaborate?

Member

glogiotatidis commented Jan 30, 2014

@dpoirier i'm not sure i follow you. from_reply is set just fine here. Here is an example

MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Please join me on mozillians.org
From: 4979c8ade727d501f345c0dc06eb3277d9bc9be2 via Mozillians  <no-reply@mozillians.org>

can you please elaborate?

@jonathan-s

This comment has been minimized.

Show comment
Hide comment
@jonathan-s

jonathan-s Jan 30, 2014

Contributor

@glogiotatidis It seems like I'm not entirely friends with git yet. I did an amendment and tried pushing it. Then I got a merge conflict which I resolved, as the merge conflict resolved I got a new commit. Once I had that I tried doing a rebase to squash those commits, but I didn't succeed with that. Oh well.

And Thanks a lot for the comments!!

Contributor

jonathan-s commented Jan 30, 2014

@glogiotatidis It seems like I'm not entirely friends with git yet. I did an amendment and tried pushing it. Then I got a merge conflict which I resolved, as the merge conflict resolved I got a new commit. Once I had that I tried doing a rebase to squash those commits, but I didn't succeed with that. Oh well.

And Thanks a lot for the comments!!

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Jan 30, 2014

Member

I think I know what happened. When you amend, you change git's history. So you'll have to
git push -f to force the push to github, otherwise git will complain.

Do not worry, I'll review your changes (maybe tomorrow) and I'll fix this for you.

Thanks for updating!

Member

glogiotatidis commented Jan 30, 2014

I think I know what happened. When you amend, you change git's history. So you'll have to
git push -f to force the push to github, otherwise git will complain.

Do not worry, I'll review your changes (maybe tomorrow) and I'll fix this for you.

Thanks for updating!

@jonathan-s

This comment has been minimized.

Show comment
Hide comment
@jonathan-s

jonathan-s Jan 30, 2014

Contributor

Thank you! Then I know when to use git push -f the next time!

Contributor

jonathan-s commented Jan 30, 2014

Thank you! Then I know when to use git push -f the next time!

@jonathan-s

This comment has been minimized.

Show comment
Hide comment
@jonathan-s

jonathan-s Jan 30, 2014

Contributor

I also tried to find the builtin indent template filter, but I couldn't find it. So I assume it's a customized filter within this project.

Contributor

jonathan-s commented Jan 30, 2014

I also tried to find the builtin indent template filter, but I couldn't find it. So I assume it's a customized filter within this project.

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Jan 30, 2014

Member

Sorry for not providing a link in the first place. It's a jinja filter (we use jinja instead of django's template engine)

http://jinja.pocoo.org/docs/templates/#indent

Member

glogiotatidis commented Jan 30, 2014

Sorry for not providing a link in the first place. It's a jinja filter (we use jinja instead of django's template engine)

http://jinja.pocoo.org/docs/templates/#indent

@jonathan-s

This comment has been minimized.

Show comment
Hide comment
@jonathan-s

jonathan-s Jan 30, 2014

Contributor

Ah, then it makes sense! Is there any reason for using jinja instead of Django's own template engine?

Contributor

jonathan-s commented Jan 30, 2014

Ah, then it makes sense! Is there any reason for using jinja instead of Django's own template engine?

@dpoirier

This comment has been minimized.

Show comment
Hide comment
@dpoirier

dpoirier Jan 30, 2014

Contributor

@glogiotatidis apparently I was wrong, Django send_mail is smarter than I thought.

Contributor

dpoirier commented Jan 30, 2014

@glogiotatidis apparently I was wrong, Django send_mail is smarter than I thought.

@dpoirier

This comment has been minimized.

Show comment
Hide comment
@dpoirier

dpoirier Jan 31, 2014

Contributor

@jonathan-s I wasn't around when they chose Jinja, but it's often considered more flexible and faster than Django's own template engine. (I'm not sure how much the "faster" part is true anymore.)

Contributor

dpoirier commented Jan 31, 2014

@jonathan-s I wasn't around when they chose Jinja, but it's often considered more flexible and faster than Django's own template engine. (I'm not sure how much the "faster" part is true anymore.)

@dpoirier

View changes

mozillians/templates/phonebook/invite_email.txt
{% trans sender=sender %}
-Hi there. {{ sender }} has invited you to join mozillians.org,
+{{ sender }} has invited you to join mozillians.org,

This comment has been minimized.

@dpoirier

dpoirier Feb 3, 2014

Contributor

The bug suggested this paragraph start:

_Inviter's full name_ (_inviter's email address_) has invited you to join mozillians.org, the community directory for Mozilla contributors.

Even if {{ sender }} happens to get rendered that way, it would probably be better to format the message explicitly in the template.

@dpoirier

dpoirier Feb 3, 2014

Contributor

The bug suggested this paragraph start:

_Inviter's full name_ (_inviter's email address_) has invited you to join mozillians.org, the community directory for Mozilla contributors.

Even if {{ sender }} happens to get rendered that way, it would probably be better to format the message explicitly in the template.

@dpoirier

View changes

mozillians/templates/phonebook/invite_email.txt
+{{ _('Take two minutes to create your mozillians.org profile:') }}
+{{ link }}
+
+{{ _('Thanks for contributing to Mozilla') }} :)

This comment has been minimized.

@dpoirier

dpoirier Feb 3, 2014

Contributor

Period at the end of this sentence?

@dpoirier

dpoirier Feb 3, 2014

Contributor

Period at the end of this sentence?

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Feb 11, 2014

Member

hey @jonathan-s. sorry for the multiday delay. Do you feel comfortable to go ahead with the changes Dan suggests and also create a test to verify that everything works as expected?

If the test is over your head, I'm happy to do that for you. Let me know.

Thanks!

Member

glogiotatidis commented Feb 11, 2014

hey @jonathan-s. sorry for the multiday delay. Do you feel comfortable to go ahead with the changes Dan suggests and also create a test to verify that everything works as expected?

If the test is over your head, I'm happy to do that for you. Let me know.

Thanks!

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Feb 27, 2014

Member

hey @jonathan-s . no pressure, I'm just checking in to say that if you're stuck I'm happy to help :)

Member

glogiotatidis commented Feb 27, 2014

hey @jonathan-s . no pressure, I'm just checking in to say that if you're stuck I'm happy to help :)

@jonathan-s

This comment has been minimized.

Show comment
Hide comment
@jonathan-s

jonathan-s Feb 28, 2014

Contributor

Hi @glogiotatidis, well the thing I am stuck with is getting that 'gettext' thing to work properly on my computer. That's the one thing that gives me errors and sort of prevents me from going further so to speak.

Contributor

jonathan-s commented Feb 28, 2014

Hi @glogiotatidis, well the thing I am stuck with is getting that 'gettext' thing to work properly on my computer. That's the one thing that gives me errors and sort of prevents me from going further so to speak.

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Mar 2, 2014

Member

Sorry to hear that @jonathan-s. How do you like to move on with this? Would you like to try settings up debian / ubuntu on a virtualbox? Alternatively I can finish this off for you and you go ahead with another bug?

It would be great that you continue to work on mozillians, I don't want a stupid installation issue to hold you back :(

Would you consider the following solution: coding locally but testing your code to a remote dev environment built for you?

Member

glogiotatidis commented Mar 2, 2014

Sorry to hear that @jonathan-s. How do you like to move on with this? Would you like to try settings up debian / ubuntu on a virtualbox? Alternatively I can finish this off for you and you go ahead with another bug?

It would be great that you continue to work on mozillians, I don't want a stupid installation issue to hold you back :(

Would you consider the following solution: coding locally but testing your code to a remote dev environment built for you?

@jonathan-s

This comment has been minimized.

Show comment
Hide comment
@jonathan-s

jonathan-s Mar 3, 2014

Contributor

It sounds like a good idea to set up a virtualbox with ubuntu / debian. That would probably help in the long run. Although I'd need some help doing that.

Contributor

jonathan-s commented Mar 3, 2014

It sounds like a good idea to set up a virtualbox with ubuntu / debian. That would probably help in the long run. Although I'd need some help doing that.

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Mar 3, 2014

Member

OK let's move forward with this. There're plenty of guides online on how to install ubuntu on virtualbox and as always if you get stuck feel free to ping us. :)

Member

glogiotatidis commented Mar 3, 2014

OK let's move forward with this. There're plenty of guides online on how to install ubuntu on virtualbox and as always if you get stuck feel free to ping us. :)

@jonathan-s

This comment has been minimized.

Show comment
Hide comment
@jonathan-s

jonathan-s Mar 8, 2014

Contributor

l box with ubuntu on it. But instead it throws an error when I try to create a user through the UserFactory, traceback here: http://pastebin.mozilla.org/4517668 any pointers perchance giorgos?

Contributor

jonathan-s commented Mar 8, 2014

l box with ubuntu on it. But instead it throws an error when I try to create a user through the UserFactory, traceback here: http://pastebin.mozilla.org/4517668 any pointers perchance giorgos?

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Mar 10, 2014

Member

The error you see it's related to ElasticSearch. Read step 7 here http://mozillians.readthedocs.org/en/latest/installation-virtualenv.html

You'll have to run ELasticSearch on every boot of your VM

Let me know if that helps.

Member

glogiotatidis commented Mar 10, 2014

The error you see it's related to ElasticSearch. Read step 7 here http://mozillians.readthedocs.org/en/latest/installation-virtualenv.html

You'll have to run ELasticSearch on every boot of your VM

Let me know if that helps.

@glogiotatidis glogiotatidis changed the title from [Fixed Bug 703316] - Invitation email is impersonal to WIP - [Fixed Bug 703316] - Invitation email is impersonal Mar 26, 2014

@glogiotatidis

This comment has been minimized.

Show comment
Hide comment
@glogiotatidis

glogiotatidis Apr 7, 2014

Member

Closing due to inactivity.

Member

glogiotatidis commented Apr 7, 2014

Closing due to inactivity.

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