Send confirmation email #4335

chadwhitacre · 2017-02-14T20:15:35Z

Part of #4305.

Requirements

(7) and (8) from the workflow:

(7) receive email
(8) click link

Todo

chadwhitacre · 2017-02-20T16:04:00Z

Rebased, was f3e657f.

chadwhitacre · 2017-02-20T20:09:25Z

Gosh, this would be easier to implement if we kept it to two steps:

add email
claim package

But that makes it harder for users.

chadwhitacre · 2017-02-20T20:09:42Z

The natural thing for a user is to claim the package and add the email in one fell swoop.

chadwhitacre · 2017-02-21T15:05:45Z

Our email infrastructure is pretty squishy. I started really getting into it yesterday, and I'm trying to decide how deep to go.

chadwhitacre · 2017-03-03T20:43:31Z

Rebased.

chadwhitacre · 2017-03-03T20:43:48Z

I started really getting into it yesterday, and I'm trying to decide how deep to go.

Went pretty deep! 😁

chadwhitacre · 2017-03-22T17:29:39Z

Alright, what the heck am I doing here?

chadwhitacre · 2017-03-22T17:34:21Z

What's the end result here? What behavior?

/me reviews ticket and parent ...

chadwhitacre · 2017-03-22T17:50:16Z

chadwhitacre · 2017-03-22T17:50:48Z

(I updated the ticket description with requirements.)

chadwhitacre · 2017-03-22T18:05:22Z

Blorg. Right. Gotta think through adding email vs. mailing before adding. Current infrastructure assumes ... wait, does it? Assume we're only ever sending to participant.email_address? But then how does adding non-primary emails work?

chadwhitacre · 2017-03-22T18:06:13Z

Ah ... is the To specified in the context in that case?

chadwhitacre · 2017-03-22T18:10:44Z

Yeah, that's it.

chadwhitacre · 2017-03-22T18:13:14Z

Existing verification_notice should be good enough for the package claiming case as well.

chadwhitacre · 2017-03-22T18:45:20Z

We can mostly use the existing add_email flow. The caveat is that we should send a mail even when the email already exists.

You can't raise it with a 200. The class works in concert with error.spt.

- use LocalizedErrorResponse to deduplicate error messages - don't include email address in error message to reduce spoofing surface

Only available with the new `start-verification` action.

chadwhitacre · 2017-04-03T12:53:09Z

Rebased, was de1a0a3.

kaguillera · 2017-04-12T21:09:21Z

LGTM...off to the second reviewr

dowski

Hopefully some of this is helpful. Some of it is certainly my ignorance of Gratipay code/context/conventions.

dowski · 2017-04-13T03:43:23Z

gratipay/models/participant/email.py

+        and returns ``None``.
+        """
+        if not packages:
+            return


Is this a normal condition? If not, consider logging or throwing an exception. Seems odd to start a package claim on an empty list of packages.

Done in 2c2a3a5. 👍

dowski · 2017-04-13T03:45:31Z

gratipay/models/participant/email.py

+        """
+        if not packages:
+            return
+        VALUES, values = [], []


Is this Gratipay convention, to use CAPS for format strings? Normally I'd expect that to be a constant.

I had named it VALUES to harmonize with VALUES in the SQL statement, but I see your point. Addressed in 2c2a3a5.

dowski · 2017-04-13T03:46:20Z

gratipay/models/participant/email.py

+        for p in packages:
+            VALUES.append('(%s, %s)')
+            values += [nonce, p.id]
+        VALUES = ', '.join(VALUES)


It seems tricky to switch types of a variable from line to line. Maybe that's the Java coder in me though. ;-)

Addressed in 2c2a3a5. For large inputs the ', '.join idiom is more performant than string concatenation due to Python implementation details, but our inputs here are not large (order of magnitude 100 packages max to be claimed at once).

FWIW, I didn't have a problem with .join(), it was just my internal type checker twitching at VALUES starting as a list of strings and then getting reassigned to a string.

P.S. @kaguillera thinks this is the Java coder in you. 😛

dowski · 2017-04-13T03:51:56Z

tests/py/test_email.py

-        headers = {b'HTTP_ACCEPT_LANGUAGE': b'en'}
-        return f('/~alice/emails/modify.json', data, auth_as=user, **headers)
+
+        # Hack to work around Aspen test client limitations.


What limitation?

Comment verbosified in 136591a.

dowski · 2017-04-13T03:59:14Z

emails/verification.spt

-     ('<b><a href="https://gratipay.com/~{0}/">{0}</a></b>'|safe).format(username)) }}
+{% if new_email_verified %}
+{{ ngettext( "We've received a request to connect the {package_name} npm package to the "
+             "{username} account on Gratipay. Sound familiar?"


What if it doesn't sound familiar?

What if it does, for that matter? I don't really know what I would do if I received an email like this.

Am I supposed to click the package name?

Down on line 32 there's a "Yes, proceed!" button. Here's what it looks like:

Ah, woops - didn't see that! Thanks!

dowski · 2017-04-13T04:02:07Z

gratipay/models/participant/email.py

+            VALUES.append('(%s, %s)')
+            values += [nonce, p.id]
+        VALUES = ', '.join(VALUES)
+        c.run('INSERT INTO claims (nonce, package_id) VALUES ' + VALUES, values)


I'm generally unfamiliar with the Gratipay DB environment. Does anything exist to clean up claims that lay around for a long time?

No! Reticketed as #4408.

Should fix upstream some day. :/

dowski · 2017-04-13T15:31:37Z

gratipay/models/participant/email.py

        for p in packages:
-            VALUES.append('(%s, %s)')
+            extra_sql += ' (%s, %s)'


Looks like maybe this isn't going to be delimited by commas anymore?

" (%s, %s) (%s, %s) (%s, %s)"

Maybe that's OK for Postgres though?

Good point. The few tests I ran passed. Lessee here ...

Yay CI! :-)

def execute(self, query, vars=None): self.Record = None > return super(NamedTupleCursor, self).execute(query, vars) E ProgrammingError: syntax error at or near "(" E LINE 1: ...LUES ('208503b2-9283-4bc8-ab51-6ed60c28c9a3', 30) ('208503b2... E

https://travis-ci.org/gratipay/gratipay.com/builds/221772022#L2879-L2884

Fixing ...

You okay with 9926985?

chadwhitacre · 2017-04-13T18:56:54Z

With gratipay/inside.gratipay.com#1038 (comment) you should be cleared to merge, @dowski. Got a green button?

chadwhitacre · 2017-04-14T12:34:39Z

Woo-hoo! 💃

Includes: #4335 #4397 #4398 #4404 #4422 #4406 #4410 #4411 #4423

chadwhitacre changed the base branch from master to project/claim-packages February 14, 2017 20:15

This was referenced Feb 14, 2017

— #4326

Closed

Integrate initial package claiming PRs #4305

Merged

chadwhitacre force-pushed the project/claim-packages branch 2 times, most recently from bdd20ac to 9ea1b0c Compare February 20, 2017 16:00

chadwhitacre force-pushed the project/claim-packages-send branch 4 times, most recently from 53a7f29 to 91dcc5a Compare February 20, 2017 19:33

chadwhitacre mentioned this pull request Feb 21, 2017

Improve email subsystem #4344

Merged

1 task

chadwhitacre force-pushed the project/claim-packages branch from 9ea1b0c to 4ac1d86 Compare March 3, 2017 19:52

chadwhitacre force-pushed the project/claim-packages-send branch from a9f9dfe to a487ef5 Compare March 3, 2017 20:00

chadwhitacre force-pushed the project/claim-packages-send branch from a487ef5 to ba41e6d Compare March 17, 2017 14:18

chadwhitacre force-pushed the project/claim-packages branch from 4ac1d86 to 11a436f Compare March 17, 2017 14:19

chadwhitacre force-pushed the project/claim-packages-send branch from ba41e6d to b81cb7d Compare March 22, 2017 18:45

chadwhitacre added 11 commits April 3, 2017 08:52

Beef up test suite around start_email_verification

0c91a13

Move cursor creation up a level

ca9a6b6

Add *packages param to start_email_verification

a694122

Clear out old claims on resend

e66262f

Remove claims when emails removed

437f4a7

LazyResponse -> LocalizedErrorResponse

86ccce4

You can't raise it with a 200. The class works in concert with error.spt.

Clean up error handling

2e979de

- use LocalizedErrorResponse to deduplicate error messages - don't include email address in error message to reduce spoofing surface

Expose packages arg in the modify.json endpoint

8799c69

Only available with the new `start-verification` action.

Update ttw tests

8eba761

Start standardizing on dash in email template name

53b7fb9

Extend verification template for package claiming

01c837f

chadwhitacre force-pushed the project/claim-packages-send branch from de1a0a3 to 01c837f Compare April 3, 2017 12:53

chadwhitacre mentioned this pull request Apr 12, 2017

🗻 Make it easier for companies to fund open source gratipay/inside.gratipay.com#987

Closed

9 tasks

dowski suggested changes Apr 13, 2017

View reviewed changes

chadwhitacre added 2 commits April 13, 2017 11:09

Give more details about Aspen workaround

136591a

Should fix upstream some day. :/

Make somewhat clever function a little clearer

2c2a3a5

dowski reviewed Apr 13, 2017

View reviewed changes

Fix regression in start_package_claims

9926985

chadwhitacre mentioned this pull request Apr 13, 2017

Prune expired email verifications #4408

Closed

dowski approved these changes Apr 13, 2017

View reviewed changes

dowski merged commit baf8248 into project/claim-packages Apr 14, 2017

chadwhitacre deleted the project/claim-packages-send branch April 25, 2017 00:28

chadwhitacre added a commit that referenced this pull request Apr 28, 2017

Checkpoint 2

771c40f

Includes: #4335 #4397 #4398 #4404 #4422 #4406 #4410 #4411 #4423

chadwhitacre added a commit that referenced this pull request Apr 28, 2017

Checkpoint 2

eb5921f

Includes: #4335 #4397 #4398 #4404 #4422 #4406 #4410 #4411 #4423

chadwhitacre added a commit that referenced this pull request May 5, 2017

Checkpoint 2

9898a38

Includes: #4335 #4397 #4398 #4404 #4422 #4406 #4410 #4411 #4423

chadwhitacre added a commit that referenced this pull request May 9, 2017

Checkpoint 2

24559cb

Includes: #4335 #4397 #4398 #4404 #4422 #4406 #4410 #4411 #4423

Send confirmation email #4335

Send confirmation email #4335

Conversation

chadwhitacre commented Feb 14, 2017 • edited Loading

Requirements

Todo

chadwhitacre commented Feb 20, 2017

chadwhitacre commented Feb 20, 2017

chadwhitacre commented Feb 20, 2017

chadwhitacre commented Feb 21, 2017

chadwhitacre commented Mar 3, 2017

chadwhitacre commented Mar 3, 2017

chadwhitacre commented Mar 22, 2017

chadwhitacre commented Mar 22, 2017

chadwhitacre commented Mar 22, 2017

chadwhitacre commented Mar 22, 2017

chadwhitacre commented Mar 22, 2017

chadwhitacre commented Mar 22, 2017 • edited Loading

chadwhitacre commented Mar 22, 2017

chadwhitacre commented Mar 22, 2017

chadwhitacre commented Mar 22, 2017

chadwhitacre commented Apr 3, 2017

kaguillera commented Apr 12, 2017

dowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadwhitacre commented Apr 13, 2017

chadwhitacre commented Apr 14, 2017

chadwhitacre commented Feb 14, 2017 •

edited

Loading

chadwhitacre commented Mar 22, 2017 •

edited

Loading