collective / collective.pwexpiry

Emulate Active Directory password complexity requirements.
1 stars 6 forks source link

Text email #14

Closed jochumdev closed 8 years ago

jochumdev commented 8 years ago

This pull implements Text E-Mails for collective.pwexpiry, which fixes issue #12 and issue #13.

jochumdev commented 8 years ago

I needed msgid's for the translations so they get into the *.po file. Without that i18ndude doesn't work.

frisi commented 8 years ago

@frapell please wait with merging, i'll try to test this on our preview server tomorrow (or next week the latest)

frisi commented 8 years ago

subject and text of the emails get translated, but the variables are not filled in properly @pcdummy .

ie the subject of the email was: ${days} Tage übrig bis Ihr Passwort abläuft

the mapping has to be added to the MessageFactory directly when you pass it to i18n.translate or you have to use i18n.translate('just-a-msgid', 'translation-domain', mapping={...})

@frapell tested on our server and ready for you review now

frapell commented 8 years ago

@frisi @pcdummy Hey guys, I'm not too sure about this solution...

before, you could register your own "notification_email" view for a custom layer in your project, and so, have a completely custom notification email.

With this change, this is pretty much fixed, relying solely on changing the translation for "email_subject", "email_text", and "email_text_expired" for customizing this...

frisi commented 8 years ago

that's a good point @frapell.

@pcdummy got rid of the browser view because

i'm not sure how to get the best out of both versions: translations on the one site, customization on the other.

A

a possible solution would be to register send_notification_email as a multi-adapter (for interface and request - so it can be easily customized for browser-layers) or utiltiy (overrides.zcml or z3c.unconfigure would be needed to register another one) and look it up in collective.pwexpiry.actions.action.BaseExpiration.notification_action:

def notification_action(self, userdata, days_to_expire):
    #getMultiAdapter((IExpiryNotificationMail, self.request))  # self.context.REQUEST would be needed here - a bit hacky
    send_notification_email = getUtility(IExpiryNotificationMail)
    send_notification_email(userdata, days_to_expire)

the corresponding utility registration could look something like this:

<utility 
  component="collective.pwexpiry.utils.send_notification_email"
  provides="collective.pwexpiry.interfaces.IExpiryNotificationMail" />

this way people can provide their own implementations of the script.

B

another route could be to provide a factory for the utility that can be subclassed:

class ExpiryNoticationSender(object):
    implements(IExpiryNotificationMail)

    def send_notification(user, days_to_expire):
        ...
        subject=i18n.translate(
             self.get_subject(user, days), target_language=language)
        ... # same for email_body_text
        api.portal.send_email(recipient=recipient,
                                           subject=subject,
                                           body=msg)

    def get_subject(user, days):
        #override this in subclasses
        return _(u"${name}, your password expires in ${nrdays} days",
                      mapping={'nrdays': days, 'name': user.getProperty('fullname')}
<utility 
  factory="collective.pwexpiry.utils.ExpiryNotificationSender"/>

@frapell: would you merge such an implementation? if yes - A, or B?

if not - please let us know your solution.

frisi commented 8 years ago

still waiting for your feedback @frapell :-)

frapell commented 8 years ago

@frisi @pcdummy Would it be possible to bring the browser view back and make it return plain text?

If not, I think registering send_notification_email as a multi adapter for context and request should be fine for customizing...

frisi commented 8 years ago

bringing back the browserview but dropping the template is a good idea @frapell. we'll have to add the language to the options and return translated strings in the __call__ method. might even be 100% BBB this way ;-)

jochumdev commented 8 years ago

All development hapens now in #15.