Skip to content

Conversation

@kevinetienne
Copy link
Contributor

It looks like we have two different behaviors to send emails in django-user-management.
In some places we are sending html and text emails and in PasswordReset we only send html emails.

Using incuna-pigeon would make sending emails behaving the same accross the project and would allow apps using django-user-management to customise handlers to send emails.

As this project is compatible with django1.6 and 1.7, I think it makes sense to define the notification handlers in settings.

@meshy
Copy link
Contributor

meshy commented Mar 12, 2015

👍

@kevinetienne
Copy link
Contributor Author

Not yet ready still need a setting to use a custom Notification

Choose a reason for hiding this comment

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

You use {{ protocol }} elsewhere. Is there a reason to be different here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should be protocol

Choose a reason for hiding this comment

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

You could add a Notification subclass and attach text_email_template and html_email_template as class attributes. In projects we've then added the Notification subclass to an AppConfig to allow overriding/extension.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 280d595 on use-incuna-pigeon into be3ca0a on master.

Kevin Etienne added 2 commits March 13, 2015 09:11
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e2387e6 on use-incuna-pigeon into be3ca0a on master.

@kevinetienne
Copy link
Contributor Author

Updated

Copy link
Contributor

Choose a reason for hiding this comment

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

First line of multiline docstrings on new line, please.

@kevinetienne
Copy link
Contributor Author

Updated

Choose a reason for hiding this comment

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

'my_custom_email.txt'

Choose a reason for hiding this comment

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

I would do from user_management.utils import notifications and then notifications.PasswordResetNotification later.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ca69ca8 on use-incuna-pigeon into be3ca0a on master.

Choose a reason for hiding this comment

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

Instead of mocking the notification, you could mock the user I think. You don't care how uid or token are calculated for this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both notification and user are very silly.

Choose a reason for hiding this comment

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

Huh? Can you explain what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of mocking the notification, you could mock the user I think.

user are a stupid-head. :P

Copy link
Contributor

Choose a reason for hiding this comment

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

http://img.pandawhale.com/post-25810-nevermind-gif-IT-Crowd-Imgur-yvca.gif

Choose a reason for hiding this comment

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

😒

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense I think, the link (uid and token) is already asserted when rendering the template, probably best to make an assertCountEqual.

@kevinetienne
Copy link
Contributor Author

Don't merge yet I've spotted an error in the templates

@kevinetienne
Copy link
Contributor Author

Templates have been updated

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0c6ff45 on use-incuna-pigeon into 6e4d651 on master.

LilyFirefly pushed a commit that referenced this pull request Mar 16, 2015
Add `incuna-pigeon` to send notifications
@LilyFirefly LilyFirefly merged commit 92500c6 into master Mar 16, 2015
@LilyFirefly LilyFirefly deleted the use-incuna-pigeon branch March 16, 2015 16:00
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.

5 participants