Cap unfunded dues #3981

rohitpaulk · 2016-04-05T07:02:08Z

This is a redo of #3876 with a cleaner commit log.

chadwhitacre · 2016-04-05T21:24:01Z

sql/branch.sql

@@ -0,0 +1,3 @@
+BEGIN;
+    UPDATE payment_instructions SET due = 0 WHERE due > 9.41;


This means that anyone with dues and a failing credit card is getting a big reset. I think that means we're leaving a little money on the table. An alternative would be to reset due to 9.41, and then if/when they fix their card, we'd charge them that much instead of having to build back up. Right?

Was that the original intention at #3876 (diff)?

We aren't otherwise resetting to zero after a card fails. Maybe we should make an effort to avoid doing that in the repair step here?

Aha, you're right. That was the original intention, yes - But even that diff is just an approximation.

UPDATE payment_instructions SET due = floor(9.41/(amount)) * amount WHERE due > 9.41;

^ That assumes that the user didn't change the tip amount. For example - let's say the user had a 5 dollar tip amount.

Payday 1

Charged: 0
Due: $5

Payday 2

Attempted to charge, failed
Due: $10 (our old code does this)

Expected behaviour after our script runs is that the due is reset to $5.

What happens if the user changes the tip amount after Payday 2 though? If they changed the tip amount to $1, let's say - we'll end up setting the due to floor(9.41/(amount)), i.e. $9

Given the two options - I think the resetting to $0 option would be safer though...

Hmm, maybe we could use #3851 to truly solve this?

Let's check back here once I've identified the issue there

Okay.

For the fifth, amount = 0, meaning due should be reset to zero.

Done in cc1d5f1

What happens if the user changes the tip amount after Payday 2 though?

I think that we should be able to isolate cases where this has happened.. And if we're looking to get this out the door before #3851 is done - the best way to go about this is reset the dues to $0 for only those users. For users who did not change the tip amount at all, the original UPDATE statement should work just fine

Agreed. How many users are affected if we go this route? I suspect it's not very many ...

In total - it'd be a lot of people (654, to be exact). But after isolating the ones that we can't properly repair without #3851, it might not be many... Time to run some queries.

SELECT COUNT(*) FROM payment_instructions WHERE due > 9.41; #=> 654 SELECT COUNT(*) FROM current_payment_instructions WHERE due > 9.41; -- Should be the same as above #=> 654 SELECT COUNT(*) FROM current_payment_instructions cpi WHERE due > 9.41 AND ( SELECT COUNT(*) -- Tips with a different amount exist. FROM payment_instructions pi WHERE cpi.participant = pi.participant AND cpi.team = pi.team AND amount != cpi.amount ) > 0; #=> 34

But after isolating the ones that we can't properly repair without #3851, it might not be many...

So that's just 34 people out of 654. I'm going to modify the script to set dues to zero for those 34 people (because we run the risk of overcharging them if we go with an approximation). For the rest of the users, we'll go with the original UPDATE query that sets the due to the highest multiple of the tip amount that is < the minimum charge

rohitpaulk · 2016-04-08T12:32:50Z

Rebased on master.

chadwhitacre · 2016-04-08T12:46:48Z

@rohitpaulk With #3981 (comment), should we take this out of Review for now? Sounds like you want to do a little more digging before we merge this, ya?

rohitpaulk · 2016-04-08T13:01:24Z

Ran 7a10ee9 on a recent backup, here's the output.

➜  gratipay.com git:(new-cap-unfunded-dues) ✗ psql gratipay < sql/branch.sql
BEGIN
UPDATE 13
UPDATE 32
UPDATE 620
COMMIT

chadwhitacre · 2016-04-08T13:24:10Z

sql/branch.sql

+BEGIN;
+    -- Tips that have been set to zero should have zero dues.
+
+    UPDATE payment_instructions SET due = 0 WHERE amount = 0 AND due != 0;


P.S. Do we do this normally? When I drop my tip to zero, does my due get reset as well?

@whit537 - Yes, https://github.com/gratipay/gratipay.com/blob/master/gratipay/models/participant.py#L856

chadwhitacre · 2016-04-08T13:40:34Z

Alright, so how does this interact with the straggler at #3980 (comment)?

chadwhitacre · 2016-04-08T13:42:26Z

Will this accidentally reset their due to zero before we've given them enough time to respond?

rohitpaulk · 2016-04-08T13:43:20Z

Will this accidentally reset their due to zero before we've given them enough time to respond?

That's a good question... Let me check

rohitpaulk · 2016-04-08T13:45:45Z

The particular user - they haven't changed their tip, so we'll be resetting to the multiple below 9.41... which we don't want to do. I guess I should modify the script to exclude people with working credit cards then. On it.

chadwhitacre · 2016-04-08T13:46:51Z

The particular user - they haven't changed their tip, so we'll be resetting to the multiple below 9.41

Confirmed.

I guess I should modify the script to exclude people with working credit cards then.

👍

chadwhitacre · 2016-04-08T13:47:47Z

The payment instruction in question has false is_funded. I would expect true if they had a working card, no?

rohitpaulk · 2016-04-08T13:49:26Z

The payment instruction in question has false is_funded. I would expect true if they had a working card, no?

Hmm, yes...

chadwhitacre · 2016-04-08T13:54:41Z

sql/branch.sql

+    -- For the rest of the payment_instructions, we set due to the highest multiple of the
+    -- tip amount that is lower than the minimum charge.
+
+    UPDATE payment_instructions SET due = floor(9.41/(amount)) * amount WHERE due > 9.41;


I'm not sure about this.

Shouldn't it be the lowest multiple that is higher?

What about people who had two tips set up? We compute minimum ~~due~~ charge based on all payment instructions together (and "tip" sure is easier to type than "payment instruction" ;).

Shouldn't it be the lowest multiple that is higher?

Ah, yes. True. We charge their CC the day they exceed the minimum charge.

What about people who had two tips set up? We compute minimum due based on all payment instructions together

Hmm, this is starting to get tricky..

The only cases where we can properly repair are:

They have only one tip set up

They've never changed the tip amount.

Really I suppose we'd want to replay paydays and payment_instructions backwards (via the events table?) until the point at which we charged them and their card failed.

Hmm, this is starting to get tricky..
Really I suppose we'd want to replay

Well, we've got 620 folks in this boat, at roughly $10 each, so call it $6,000 we'd be leaving on the table if we hard-reset those 620 to zero. Some—many? most?—of those folks will never fix their card and start giving again.

Raises the question of whether dues shouldn't expire after a while anyway. If I forget about Gratipay and come back a year later and re-up, I'd be surprised if I got hit with $10 that had been waiting there for a year. Should we expire dues after 30 (or 60, or 90) days?

Yep, brings us back to #3981 (comment) I guess

Alright, I think we cut bait. Meaning, reset everyone back to zero. We have so much to work on, I think our time is better spent on other things. Unless you really want to keep going down this rabbit hole?

Unless you really want to keep going down this rabbit hole?

Not really...

I'm going to force push this branch to the original state.

rohitpaulk · 2016-04-08T13:55:15Z

The last time we checked, is_funded was true. What could've changed?

rohitpaulk · 2016-04-08T13:56:47Z

@whit537 - I just ran the query and it still is true, can you double check?

chadwhitacre · 2016-04-08T13:56:57Z

What could've changed?

Sorry, I'm working from an old backup. It's true on prod.

rohitpaulk · 2016-04-08T13:57:02Z

SELECT * FROM payment_instructions WHERE participant = 'xxx'; is what I ran

chadwhitacre · 2016-04-08T13:57:12Z

Yeah, my bad, sorry.

rohitpaulk · 2016-04-08T14:25:45Z

Output before 29400fe -

BEGIN
UPDATE 13
UPDATE 652
COMMIT

Output after 29400fe -

BEGIN
UPDATE 13
UPDATE 648
COMMIT

chadwhitacre · 2016-04-08T14:28:47Z

Hmm ... how does that related to is_funded?

=> select count(*) from payment_instructions WHERE due > 9.41 and not is_funded;
┌───────┐
│ count │
├───────┤
│   652 │
└───────┘
(1 row)

=> select count(*) from payment_instructions WHERE due > 9.41 and is_funded;
┌───────┐
│ count │
├───────┤
│     2 │
└───────┘
(1 row)

rohitpaulk · 2016-04-08T14:30:04Z

My backup is a day old, let me try getting a new one..

chadwhitacre · 2016-04-08T14:33:07Z

The 652 / 2 counts are from prod.

rohitpaulk · 2016-04-08T14:49:48Z

gratipay=# select count(*) from payment_instructions WHERE due > 9.41 and not is_funded;
 count
-------
   652
(1 row)

gratipay=# select count(*) from payment_instructions WHERE due > 9.41 and is_funded;
 count
-------
     2
(1 row)

BEGIN
UPDATE 13
UPDATE 651
COMMIT

chadwhitacre · 2016-04-08T14:50:13Z

Perfect. Let's do it!

rohitpaulk · 2016-04-08T14:52:12Z

Perfect. Let's do it!

@whit537 - Something I'm missing? 652 != 651?

chadwhitacre · 2016-04-08T14:52:57Z

@rohitpaulk Yeah, that's okay. One of the two is part of the 13. ;-)

rohitpaulk · 2016-04-08T14:53:41Z

Aha, yes gotcha. :)

rohitpaulk · 2016-04-08T14:54:16Z

Verified.

gratipay=# UPDATE payment_instructions SET due = 0 WHERE amount = 0 AND due != 0;
UPDATE 13

gratipay=# select count(*) from payment_instructions WHERE due > 9.41 and not is_funded;
 count
-------
   651
(1 row)

rohitpaulk added the Review label Apr 5, 2016

This was referenced Apr 5, 2016

Cap unfunded dues #3876

Closed

Add team receiving page #3873

Merged

chadwhitacre mentioned this pull request Apr 5, 2016

Radar 53 gratipay/inside.gratipay.com#558

Closed

chadwhitacre reviewed Apr 5, 2016
View reviewed changes

chadwhitacre mentioned this pull request Apr 5, 2016

Add script to fix stale cached values #3980

Closed

rohitpaulk added 3 commits April 8, 2016 18:02

Test for adding dues only if valid CC exists

d0e79ca

Only increment dues if valid CC exists

5c1e98d

Add SQL script to fix old values

af1e6a2

rohitpaulk force-pushed the new-cap-unfunded-dues branch from 61fbb2a to af1e6a2 Compare April 8, 2016 12:32

rohitpaulk removed the Review label Apr 8, 2016

rohitpaulk added the Review label Apr 8, 2016

chadwhitacre reviewed Apr 8, 2016
View reviewed changes

rohitpaulk removed the Review label Apr 8, 2016

chadwhitacre reviewed Apr 8, 2016
View reviewed changes

rohitpaulk force-pushed the new-cap-unfunded-dues branch from 7a10ee9 to cc1d5f1 Compare April 8, 2016 14:17

Add SQL statement to set due to zero if amount is 0

7d4f9de

rohitpaulk force-pushed the new-cap-unfunded-dues branch from cc1d5f1 to 7d4f9de Compare April 8, 2016 14:21

Only reset dues for people with failing CCs

29400fe

chadwhitacre added the Review label Apr 8, 2016

chadwhitacre merged commit 19862c0 into master Apr 8, 2016

chadwhitacre deleted the new-cap-unfunded-dues branch April 8, 2016 14:50

chadwhitacre mentioned this pull request Apr 8, 2016

avoid increasing due beyond payin minimum when cards fail #3818

Closed

		@@ -0,0 +1,3 @@
		BEGIN;
		UPDATE payment_instructions SET due = 0 WHERE due > 9.41;

Cap unfunded dues #3981

Cap unfunded dues #3981

Conversation

rohitpaulk commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Payday 1

Payday 2

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

rohitpaulk commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadwhitacre commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

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

rohitpaulk commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

chadwhitacre commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016

rohitpaulk commented Apr 8, 2016