Skip to content

Conversation

@LilyFirefly
Copy link

In VerifyEmailMixin.send_validation_email we hard-code the email subject. We also assume only a html email is sent.

Ian Foote added 5 commits August 20, 2014 11:34
* Use plain text email by default
* Set EMAIL_SUBJECT_TEMPLATE on VerifyEmailMixin
* Refactor send_validation_email to allow easier testing
* Add _get_email_context helper method
* Simplify _get_email_kwargs method
* Avoid duplicating work in test__email_context and
  test__get_email_kwargs
* Allow the email templates used by send_validation_email to be
  overriden or disabled.
* Allow the context provided to the email templates to be extended.
* Add a docstring to send_validation_email.
@LilyFirefly
Copy link
Author

@adam-incuna @meshy Review?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 47820f7 on verification-email into b01229c on master.

Copy link
Contributor

Choose a reason for hiding this comment

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

The underscore suggests that this should not be lightly overridden. Is that the case?

* Make email_kwargs part of the public api
* Standardise naming to match email_context
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling dcc4672 on verification-email into b01229c on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 42b965d on verification-email into b01229c on master.

@LilyFirefly
Copy link
Author

@adam-incuna @meshy Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

{{ domain }} ?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe template isn't the best word - I'm not referring to a django template here. EMAIL_SUBJECT_TEMPLATE is a string that is used with .format in get_email_subject.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh cool :) maybe specify that?

@adam-thomas
Copy link
Contributor

Nice one, this looks like a really useful improvement!

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 225c688 on verification-email into b01229c on master.

@adam-thomas
Copy link
Contributor

👍 happy to merge if @meshy is!

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be return {...}

@meshy
Copy link
Contributor

meshy commented Aug 20, 2014

Looks good other than my comment about the return statement.

adam-thomas added a commit that referenced this pull request Aug 20, 2014
Allow more customisation of account verification email
@adam-thomas adam-thomas merged commit 4dae2b0 into master Aug 20, 2014
@adam-thomas adam-thomas deleted the verification-email branch August 20, 2014 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants