Pylons / pyramid_mailer

A package for sending email from your Pyramid application
https://docs.pylonsproject.org/projects/pyramid-mailer/en/latest/
Other
50 stars 40 forks source link

bcc field visibility with `pyramid_mailer debug` #86

Closed RaHus closed 6 years ago

RaHus commented 7 years ago

We are using pyramid mailer debug for manual QA of features. It would be nice to have a configuration option to include the bcc field (in any format) as part of the .msg files for ease of testing. I can provide the PR if we agree this would be useful.

tseaver commented 7 years ago

@RaHus How do you plan to configure that option?

RaHus commented 7 years ago

The way i see it a new configuration directive like: pyramid_mailer.debug_include_bcc = true would be enough(?)

tseaver commented 7 years ago

That will be tricky: pyramid_mailer.message.Message.to_message doesn't have any way to get to the Mailer instance, which is where all the config variables are stored.

RaHus commented 7 years ago

Is it ok to change the signature of to_message() to to_message(debug_include_bcc=False) and then on DebugMailer._send do a to_message(debug_include_bcc=True)? I guess Its not very clean to add a keyword argument in to_message that relates to a debug feature, but in the end to_message would need to handle the actual inclusion of the data to the message (it'll need to know about this feature). As an alternative we could add a Message.debug_include_bcc class instance attribute which _send() will populate on the instance and to_message will read to enable this functionality.

RaHus commented 7 years ago

And if all the above is too intrusive/suboptimal/ugly (it is probably), we could consider allowing users to provide their own DebugMailer subclass so you could have some control over the behaviour of _send, and possibly add a recipe in the docs covering the use case this ticket is about

tseaver commented 7 years ago

I think it would be better to have a module-level global flag, e.g.:

# message.py

_DEBUG_MESSAGE_INCLUDES_BCC_HEADER = False

...
        if self.bcc and _DEBUG_MESSAGE_INCLUDES_BCC_HEADER:
            base['Bcc'] = ', '.join(self.bcc)

and then add configuration support to flip that flag on.

RaHus commented 7 years ago

Would that maybe cause a race condition if an environment instantiates multiple pyramid apps with multiple conf files? Not sure, but other than that i am ready to provide the PR with this implementation

mmerickel commented 7 years ago

Doesn’t the mailer invoke to_message so it could pass some flag? I’d much prefer a solution that worked per-mailer than the global option.

mmerickel commented 7 years ago

I looked quickly at the source and there is also a mailer._send method which is there for the purposes of writing the message to a file. I wonder if it could just be improved to include BCC info somehow.

RaHus commented 7 years ago

Now that i look at it again it can be done by only modifying DebugMailer._send like so:

    # mailer.py
    def _send(self, message, fail_silently=False):
        ...
        if self._debug_include_bcc:
            message.extra_headers['Bcc'] = ', '.join(message.bcc)

to_message later on does a base.update(dict(self.extra_headers)) so its equivalent to tseavers base['Bcc'] = ', '.join(self.bcc)

RaHus commented 6 years ago

This got merged and nothing else came up after but there hasn't been any release in pypi recently. I would love to get this package managed through normal pypi packages instead of gitsources. Are there specific requirements for the versioned releases which we are not meeting at this point? Any way i can help?

stevepiercy commented 6 years ago

There's no show stopper. I think it's just a matter of someone with the time to cut a release. There's minor housekeeping that needs to be addressed at some point, and I've created an issue #88 for that.

RaHus commented 6 years ago

I'll try to provide a PR in the next 3 days for the items listed in #88 if that's ok with you. Also closing this ticket

digitalresistor commented 6 years ago

@RaHus No worries on doing that work, we'll likely get it done. There are likely more house keeping tasks necessary. I'll also cut a release after verifying everything is up to snuff.