Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Send email for Terms notification #4232

Merged
merged 15 commits into from
Jan 6, 2017

Conversation

chadwhitacre
Copy link
Contributor

@chadwhitacre chadwhitacre commented Dec 13, 2016

Includes a script that we can reuse next time we need to send out a transactional email to everyone.

@chadwhitacre chadwhitacre mentioned this pull request Dec 13, 2016
21 tasks
@chadwhitacre
Copy link
Contributor Author

Still in process. I wanna write some tests.

[gratipay] $ run defaults.env local.env send-branch-email
Usage: /Users/whit537/personal/gratipay/gratipay.com/env/bin/send-branch-email [username|all]
[gratipay] $

@chadwhitacre
Copy link
Contributor Author

Ready for review @JessaWitzel @mattbk!

@chadwhitacre
Copy link
Contributor Author

Gosh, I guess I should actually test the mail though. Harumph.

@JessaWitzel
Copy link
Contributor

JessaWitzel commented Dec 15, 2016

Looks good to me! As an exercise, here is how I would have written it. It's interesting how tone differs so much depending on who is writing everything.

Hello!

We are so glad that you have joined us over at Gratipay! We know you do not receive many emails like this from us, and that is intentional. We certainly don't want to spam you because when we do email you we want you to know that it's important, plus that's just annoying.

Today we want to let you know that we have revised our Terms of Service, that all important document that governs your continued usage. If you no longer use your Gratipay account, you can close it here.
If you want to continue to use Gratipay, please learn about our new Terms as soon as you can.

Have a great day!

@clone1018
Copy link
Contributor

Would be awkward to have a username of all :)

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Dec 15, 2016

:-P

The username switch is there for testing, I don't anticipate using this to mail just one participant in general.

@mattbk
Copy link
Contributor

mattbk commented Dec 15, 2016

Please don't wait for a review from me--I am unqualified for this one.

@chadwhitacre
Copy link
Contributor Author

Gonna see about mailing myself with this one ...

@chadwhitacre
Copy link
Contributor Author

That means AWS locally!

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Dec 16, 2016

Hey! I already have AWS keys commented out in my local.env. They're labeled "emails_development AWS user." Let's give that a shot!

@chadwhitacre
Copy link
Contributor Author

Rebased, was 03d3081.

@chadwhitacre
Copy link
Contributor Author

[gratipay] $ run defaults.env local.env queue-branch-email whit537
Okay, just whit537.
1
spotcheck: [email protected] (whit537=1451)
queuing for [email protected] (whit537=1451)
[gratipay] $

Now where did it go? :)

@chadwhitacre
Copy link
Contributor Author

Oh! This just enqueues it, another process has to dequeue it.

@chadwhitacre
Copy link
Contributor Author

Those failures tho ... 😕

@chadwhitacre
Copy link
Contributor Author

Thanks for signing up with Amazon Web Services. Your services may take up to 24 hours to fully activate. If you’re unable to access AWS services after that time, here are a few things you can do to expedite the process:

screen shot 2016-12-16 at 2 01 51 pm

@chadwhitacre
Copy link
Contributor Author

That's trying to sign up for a new account because I don't have the login handy for our existing account (coffee shop).

@chadwhitacre
Copy link
Contributor Author

Both failing on iter_payday_events.

=================================== FAILURES ===================================
_____________________ TestHistory.test_iter_payday_events ______________________
self = <test_history.TestHistory testMethod=test_iter_payday_events>
    def test_iter_payday_events(self):
        now = datetime.now()
        Payday().start().run()
    
        Enterprise = self.make_team(is_approved=True)
        self.obama.set_payment_instruction(Enterprise, '10.00')  # >= MINIMUM_CHARGE!
        for i in range(2):
            with patch.object(Payday, 'fetch_card_holds') as fch:
                fch.return_value = {}
                Payday.start().run()
            self.db.run("""
                    UPDATE paydays
                       SET ts_start = ts_start - interval '1 week'
                         , ts_end = ts_end - interval '1 week';
                    UPDATE payments
                       SET timestamp = "timestamp" - interval '1 week';
                    UPDATE transfers
                       SET timestamp = "timestamp" - interval '1 week';
                """)
    
    
        obama = P('obama')
        picard = P('picard')
    
        assert obama.balance == D('0.00')
        assert picard.balance == D('20.00')
    
        Payday().start()  # to demonstrate that we ignore any open payday?
    
        # Make all events in the same year.
        delta = '%s days' % (364 - (now - datetime(now.year, 1, 1)).days)
        self.db.run("""
                UPDATE paydays
                    SET ts_start = ts_start + interval %(delta)s
                      , ts_end = ts_end + interval %(delta)s;
                UPDATE payments
                    SET timestamp = "timestamp" + interval %(delta)s;
                UPDATE transfers
                    SET timestamp = "timestamp" + interval %(delta)s;
            """, dict(delta=delta))
    
        events = list(iter_payday_events(self.db, picard, now.year))
        assert len(events) == 7
        assert events[0]['kind'] == 'totals'
        assert events[0]['given'] == 0
        assert events[0]['received'] == 20
        assert events[1]['kind'] == 'day-open'
        assert events[1]['payday_number'] == 2
        assert events[2]['balance'] == 20
        assert events[-1]['kind'] == 'day-close'
        assert events[-1]['balance'] == 0
    
        events = list(iter_payday_events(self.db, obama))
        assert events[0]['given'] == 20
>       assert len(events) == 11
E       AssertionError: assert 9 == 11
E        +  where 9 = len([{'given': Decimal('20.00'), 'kind': 'totals', 'received': 0}, {'balance': Decimal('0.00'), 'date': datetime.date(2016...: Decimal('0.61'), 'participant...'10.00'), 'recorder': None, 'balance': Decimal('10.00'), 'ref': None, 'id': 10}, ...])
tests/py/test_history.py:93: AssertionError

@chadwhitacre
Copy link
Contributor Author

screen shot 2017-01-06 at 1 34 27 pm

@chadwhitacre
Copy link
Contributor Author

That was the test message, which I received. Not sure why I'm getting "via amazonses.com" but I'm not going to sweat it.

@chadwhitacre
Copy link
Contributor Author

~ $ queue-branch-email all
Are you ready? [y/N]y
Are you REALLY ready? [y/N]y
... ? [y/N]y
Okay, you asked for it!
5358
spotcheck: []
spotcheck: []
spotcheck: []
spotcheck: []
spotcheck: []
But really actually tho? I mean, ... seriously? [y/N]

@chadwhitacre
Copy link
Contributor Author

If you have an old list that you haven’t emailed to in a while, don’t email to that list through any provider that limits your bounce rates (which includes Amazon SES), unless you’ve verified the state of the addresses (e.g., by checking login activity on your site, purchase history, etc.). Otherwise, you may incur a lot of bounces from old unused email addresses while you try to clean your list, and you risk being blocked by both ISPs and Amazon SES.

https://media.amazonwebservices.com/AWS_Amazon_SES_Best_Practices.pdf

@chadwhitacre
Copy link
Contributor Author

Only send emails to recipients who have interacted with your site recently (for example, within the last 180 days).

Do not use Amazon SES as a way to clean your recipient list.

http://docs.aws.amazon.com/ses/latest/DeveloperGuide/best-practices-recipients.html

@chadwhitacre
Copy link
Contributor Author

DKIM was enabled for our domain but not for the [email protected] address. I've enabled it for the address.

@chadwhitacre
Copy link
Contributor Author

Fixed! 👍

screen shot 2017-01-06 at 2 11 01 pm

@chadwhitacre
Copy link
Contributor Author

But, what if up to this point you haven’t been tracking those things? You have a list of addresses that you have acquired over the years, but you don’t have a mechanism in place to know when each one last interacted with you, or perhaps you don’t have a record of how each of those addresses got into your system in the first place. Maybe you haven’t even been removing addresses that bounce or complain. Ouch! This is going to be a problem!

https://aws.amazon.com//blogs/ses/never-send-to-old-addresses-but-what-if-you-have-to/

@kaguillera
Copy link
Contributor

Just a suggestion

https://pypi.python.org/pypi/validate_email

@chadwhitacre
Copy link
Contributor Author

@kaguillera What are the details of their implementation? How do they determine if an address "really exists"?

@chadwhitacre
Copy link
Contributor Author

Alright, so we definitely need to start doing this. But how much do we need to get done before sending out these emails?

@chadwhitacre
Copy link
Contributor Author

I started poking at AWS SNS for notifications. Supposedly we can use that to POST to an endpoint on our website. We could use that to collect feedback into an email_status_queue and then process that to unverify bad emails. But that's a significant chunk of work and I don't want to block this on that.

@chadwhitacre
Copy link
Contributor Author

This is tough to test because we need a good email to verify it with Gratipay, but then how do make it subsequently bounce in order to test that case?

I tried with https://temp-mail.org/en/ and https://requestb.in/ for the webhook POST. I was able to receive a subscription confirmation message, but no actual SES delivery notifications, let alone bounce/complaint.

@chadwhitacre
Copy link
Contributor Author

We can also configure SNS to dump to SQS instead.

You can configure the Amazon SQS message retention period to a value from 1 minute to 14 days. The default is 4 days. Once the message retention limit is reached, your messages are automatically deleted.

https://aws.amazon.com/sqs/faqs/#limits-restrictions

@mattbk
Copy link
Contributor

mattbk commented Jan 6, 2017

This is tough to test because we need a good email to verify it with Gratipay

You can add it to the database by hand. This is how I set up myself as admin on localhost.

@mattbk
Copy link
Contributor

mattbk commented Jan 6, 2017

http://stackoverflow.com/a/31510470/2152245

@chadwhitacre
Copy link
Contributor Author

"Do Things that Don't Scale" O.o

@chadwhitacre
Copy link
Contributor Author

I created a user in Google Apps, used it to verify an email for a Gratipay test account, and then deleted the user. Google allows restoring users for up to five days, though, so I'm not sure when we'd start getting a hard bounce. Not seeing one yet ...

@chadwhitacre
Copy link
Contributor Author

Seeing it from personal email. Trying via SES now ...

@chadwhitacre
Copy link
Contributor Author

Got it! Bounce notification via HTTPS. Once again I needed to configure for the address and not just the domain (as with DKIM above). Still not seeing it in the SQS queue, though ...

screen shot 2017-01-06 at 4 36 59 pm

@chadwhitacre
Copy link
Contributor Author

P.S. and FTR, the easiest way to test is with a Google Apps "Group," because it can route to an existing account so you don't have to do a password and login dance in order to receive mail. It may also be instantly deleted whereas full user accounts are not.

@chadwhitacre
Copy link
Contributor Author

Based on these docs I redid the link from SNS to SQS on the SQS side.

@chadwhitacre
Copy link
Contributor Author

Blam!

screen shot 2017-01-06 at 5 12 58 pm

@chadwhitacre
Copy link
Contributor Author

Alright, so here's what I propose:

  • We update the participants query to only select those who have signed in (session_expires) or show up in payments or exchanges within the past 180 days.
  • We mail all of them, tracking bounces in SQS.
  • We manually retrieve bad addresses from the queue, and drop them to unverified in the database.
  • We reticket proper bounce handling with a webhook back to https://gratipay.com/.

Given that it is now after 5pm on Friday, it looks like we are going to be sending next week after all. :-)

Will move to a new PR since this is closed ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants