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

Cap unfunded dues #3876

Closed
wants to merge 11 commits into from
Closed

Cap unfunded dues #3876

wants to merge 11 commits into from

Conversation

rohitpaulk
Copy link
Contributor

@rohitpaulk
Copy link
Contributor Author

Now to change the existing values...

@rohitpaulk
Copy link
Contributor Author

Not ready to merge. I'm seeing weird data values in our DB, got to investigate more...

=> SELECT amount, due, is_funded FROM current_payment_instructions WHERE due > 10 AND amount > 0 AND is_funded;
 amount |  due   | is_funded
--------+--------+-----------
   1.00 |  14.00 | t
  10.00 | 140.00 | t
   5.00 |  70.00 | t
   1.00 |  11.00 | t
   1.00 |  14.00 | t
   1.00 |  14.00 | t
   1.00 |  14.00 | t
   1.00 |  14.00 | t
   5.00 |  70.00 | t
   1.00 |  14.00 | t
   1.00 |  14.00 | t
   1.00 |  14.00 | t
   1.00 |  14.00 | t
   2.25 |  31.50 | t
   8.00 |  80.00 | t
   2.00 |  28.00 | t
   1.00 |  14.00 | t
   1.00 |  14.00 | t
   5.00 |  70.00 | t
   1.00 |  14.00 | t
   1.00 |  11.00 | t
   1.00 |  12.00 | t
   0.75 |  10.50 | t
   1.00 |  14.00 | t
(24 rows)

How do funded payments get to a value greater than the minimum charge???

@rohitpaulk
Copy link
Contributor Author

^ cc: @rorepo

@rohitpaulk
Copy link
Contributor Author

Looks like none of these ~users have CCs attached... Do we not mark is_funded as false when a CC is removed?

@rohitpaulk
Copy link
Contributor Author

Aha. These are people who have never attached a CC. The tips were funded because of their gratipay balance. We used to refresh these values after every Payday (before we moved to charging in arrears and never using one's balance to fund tips).

I guess running Participant.update_giving on all users should correct this problem. Going to try that on a backup...

@@ -27,7 +27,7 @@


with open('sql/payday.sql') as f:
PAYDAY = f.read()
PAYDAY = f.read() % {'minimum_charge': MINIMUM_CHARGE}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ick. We moved away from extra interpolation in ... b045fb1 (#3785). Can we avoid reintroducing that here? I guess that would mean putting minimum_charge in the database somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I'll see if I can get this done in a cleaner fashion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the approach in 2493792, @rohitpaulk?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. A further improvement could be to call it payday_settings, and create it as a temporary table as part of payday.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having it as a permanent table makes sense - If we start to write heavy SQL outside of payday, it could be useful.

@chadwhitacre
Copy link
Contributor

Initial punchlist:

I still need to slow down and read this code to understand what we're doing here ...

@chadwhitacre
Copy link
Contributor

I'm not sure what's up with this Travis failure. I'm pretty sure it's present on master, too ...

@chadwhitacre
Copy link
Contributor

Hoping to read and grok this PR tomorrow.

@chadwhitacre chadwhitacre mentioned this pull request Jan 5, 2016
@chadwhitacre
Copy link
Contributor

Build is failing on master: https://travis-ci.org/gratipay/gratipay.com/builds/98361516. Reticketed as #3885.

@chadwhitacre
Copy link
Contributor

I've kicked Travis post-#3891.

@chadwhitacre
Copy link
Contributor

Looks like there is still some breakage ...

@rohitpaulk
Copy link
Contributor Author

/me kicks Travis again..

@rohitpaulk
Copy link
Contributor Author

@whit537 - What's left here?

@chadwhitacre
Copy link
Contributor

Not much ... looks like I pushed some commits that didn't end up on this branch?

@rohitpaulk
Copy link
Contributor Author

Ohhhhhh

@rohitpaulk
Copy link
Contributor Author

I might've forced pushed :/ Do you have the branch on your local? If so, please push it to a different branch and I'll merge the two manually

@chadwhitacre
Copy link
Contributor

No worries. My commits are still in GitHub: https://github.com/gratipay/gratipay.com/commits/249379279a931a3ece8d29e2e9ec255665c997ad.

@chadwhitacre
Copy link
Contributor

;-)

$ gf
remote: Counting objects: 57, done.
remote: Total 57 (delta 36), reused 36 (delta 36), pack-reused 21
Unpacking objects: 100% (57/57), done.
From github.com:gratipay/gratipay.com
 + 2493792...12057d5 cap-unfunded-dues -> origin/cap-unfunded-dues  (forced update)

@chadwhitacre
Copy link
Contributor

Alright, brought those back. We'll see if Travis is okay with them.

@rohitpaulk
Copy link
Contributor Author

Rebased on master again.

@chadwhitacre
Copy link
Contributor

Rebased on master at ec2c732. Previous head was ad82f60.

@rohitpaulk
Copy link
Contributor Author

The commit log on this is pretty confusing - makes it hard to review. I'm going to squash this down to 2-3 meaningful commits.

@rohitpaulk
Copy link
Contributor Author

I'm seeing three major changes here, the rest is just noise.

  1. Prevent dues from exceeding minimum_charge in payday
  2. Add SQL script to repair old values that are above minimum charge
  3. Add script to recalculate stale cached values

@rohitpaulk
Copy link
Contributor Author

I'm going to cherry pick 1 into a separate PR - and retain 2 and 3 as part of this PR.

@rohitpaulk
Copy link
Contributor Author

Split into #3981 and #3980

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

Successfully merging this pull request may close these issues.

2 participants