Skip to content

Conversation

@JorneVL
Copy link

@JorneVL JorneVL commented Nov 2, 2020

This PR allows sending mails through other connections than the default in Django settings
The new connection can be created based on a Site Setting or just hardcoded.

@vsalvino
Copy link
Contributor

vsalvino commented Nov 2, 2020

It looks like this change moves the email send into a new private method, but does not actually change the functionality at all. Can you explain your reasoning?

@JorneVL
Copy link
Author

JorneVL commented Nov 2, 2020

That is correct, I implemented in a local test version as a public method. But I thought that the private method should be better.
The idea is that you override this method on a parent class (which was possible with my public method).
Do I need to change the method to public so it is possible to override?

# Send email
self.__send_mail(request, message_args)

def __send_mail(self, resuest, message_args):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this public, e.g. send_mail() would be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it doesn't look like the request arg is used/needed here.

Copy link
Author

Choose a reason for hiding this comment

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

My use case has the next code, so request can not be removed

mail_settings = MailSettings.for_request(request)
connection = get_connection(
    username=mail_settings.username, password=mail_settings.password, fail_silently=False
)

Add optional method for content_subtype
@JorneVL JorneVL requested a review from vsalvino November 2, 2020 18:27
@vsalvino vsalvino added this to the 0.20.0 milestone Nov 20, 2020
@vsalvino vsalvino removed this from the 0.20.0 milestone Jan 25, 2021
@vsalvino vsalvino added this to the 0.22.0 milestone Sep 10, 2021
@vsalvino
Copy link
Contributor

Thanks for your contribution, and your patience, @JorneVL !

This will be merged via #443 and will go out in version 0.22.0

@vsalvino vsalvino closed this Sep 10, 2021
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.

2 participants