Allow django_backend to take advantage of message_streams#103
Allow django_backend to take advantage of message_streams#103nicholasserra merged 3 commits intothemartorana:masterfrom hkhanna:backend-message-stream
Conversation
|
Hello, just wanted to bump this PR because we'd like to start using Postmark broadcast streams and can't until this is merged. Happy to help with testing or whatever else is needed to get this merged! |
|
Agreed! I'd love to get this merged as well. @themartorana let me know if there is anything you need from me. |
| self.track_opens = getattr(settings, 'POSTMARK_TRACK_OPENS', False) | ||
|
|
||
| if 'message_stream' in kwargs: | ||
| self.message_stream = kwargs['message_stream'] |
There was a problem hiding this comment.
Did you also test the case where you can pass message_stream into the EmailMessage kwargs? As opposed to setting it on the instance after instantiation? I ask because these other kwargs track_opens and tag del the kwarg after
del kwargs['tag']
Thinking we probably need to do the same here as it might raise an error upstream.
There was a problem hiding this comment.
Good catch! I've added it. But note that I can't find any reference to these classes anywhere, including the README.
I'm guessing users can use these as a convenience if they want to pass args to the __init__() function of an EmailMultiAlternatives subclass. But the way the README suggests usage is just to set tag or message_stream as an attribute on EmailMultiAlternatives after instantiation.
|
|
||
| if 'message_stream' in kwargs: | ||
| self.message_stream = kwargs['message_stream'] | ||
| else: |
There was a problem hiding this comment.
Same comment as above ^
| else: | ||
| attachments.append(item) | ||
|
|
||
| message_stream = getattr(message, "message_stream", None) |
There was a problem hiding this comment.
Mixed ticks and quotes. Can we change this to ticks to match the others?
|
Thanks! Sorry for the delay. Was this manually tested? I'll likely do the same. Think theres a unit test we can add too? Will look more in depth asap. |
Yup, I manually tested it back in August and confirmed it selected the correct stream on |
|
Added a unit test and made the requested changes. Thanks @nicholasserra! Hope we can get this merged soon. |
|
Thank you :) I'll check this out this week. |
|
Tested LGTM thank you I'll try to get this out in a new version ASAP |
3 participants
#94 allowed the
PMMailclass to add amessage_stream, but this functionality was not provided to the ordinary django_backend. This PR allows a developer to set themessage_streamattribute on theEmailMessageinstance to use a non-default message stream.