Bring back takes in payday #4102

chadwhitacre · 2016-07-29T11:17:04Z

Remake of #4025.

TODO

~~include members in MassPay; see get_ready_payout_routes_by_network~~ → Refactor test_billing_exchanges #4113
rebase once bring back the takes UI #4077 lands
refactor process_distribution for clarity—Bring back takes in payday #4102 (comment) —started in Bring back takes in payday #4102 (comment) —done in 51af227
fix order of calls (and test!)—Bring back takes in payday #4102 (comment) —started in 84f4b31 and d971d40, now:
- explain test intention—Bring back takes in payday #4102 (comment) —done at Bring back takes in payday #4102 (comment)
- reconsider test placement—Bring back takes in payday #4102 (comment) —done at Bring back takes in payday #4102 (comment)
- obviated by rebased commits 0ed1d31 and 51af227
allow owners to join teams—Bring back takes in payday #4102 (comment) —done in 3ac7fee

chadwhitacre · 2016-07-29T11:20:45Z

Okay! I rebased #4025 on #4077. GitHub wouldn't let me reopen #4025 for whatever reason, so here we are. This is still work in progress.

chadwhitacre · 2016-08-01T14:10:19Z

Made some progress on this on the bus this morning. Rebased and pushed.

chadwhitacre · 2016-08-24T11:57:45Z

Alright, getting back to this. I'm looking at test_take_over_during_payin and finding some complication related to take_over, an interaction between clearing takes and checking for identities.

chadwhitacre · 2016-08-24T17:41:50Z

This is ready for review, @Nashe @aandis @rohitpaulk! I split off a new PR for MassPay (#4111).

chadwhitacre · 2016-08-25T16:12:55Z

Found a bug while working on #4111. Withdrawing Review and fixing here ...

chadwhitacre · 2016-08-25T16:36:19Z

Okay, fixed. Ready for review again! :-)

ghost · 2016-08-25T22:31:46Z

I don't have a lot of things to say since I'm not familiar with this part of the code—but nothing shocked me. Waiting for a second approval?

chadwhitacre · 2016-08-26T12:34:28Z

Thanks @Nashe.

@rohitpaulk Would you be able to spare any cycles this week to look this over? This and #4111 are the last two major PRs needed to land payouts for Team Gratipay!

rohitpaulk · 2016-08-31T16:27:14Z

tests/py/test_billing_payday.py

+        self.enterprise.set_take_for(self.picard, D('0.01'), self.picard)
+        with pytest.raises(NotAllowed):
+            self.enterprise.set_take_for(self.picard, 50, self.picard)
+        return  # XXX allow owners to join teams!


Hmm, what is happening here? What is the limitation that we're facing in allowing owners to join teams? And if we are going to do it in the future, do we have a vague idea of how we'd do it? My main concern here is whether we're making a decision that will make it harder to re-introduce this functionality in the future. (we'll build more stuff that assumes this is not going to happen, and then that'll be harder to reverse)

I guess the fundamental issue here is that we don't allow a payment sink to be tied to a team (we drain funds to the owner's bank account) - however - We could still process takes to the owner, right? And in the future when we do allow linking bank accounts to teams - the only piece that will change is process_remainders.

I think this is also nice in terms of the payroll functionality - we want an owner to be able to set 'their' take - and have the rest go to the team. It won't make a different right now in terms of where the money goes - but teams could use that data to decide how to split money internally. For example, if @whit537 was the owner of Gratipay - he could set a take and have the rest flow to the team. And once a month, he could look at the takes history and decide how much to move to his personal account and how much to keep in the team's operations account.

I know @kaguillera and I talked about this, and I thought I recorded somewhere that we thought it was okay to allow owners to join teams. Looking for that ...

Found it!

#4077 (comment)

Ready for review in 3ac7fee.

rohitpaulk · 2016-08-31T16:28:52Z

FIrst pass done, only real concern is #4102 (comment) - the rest are just code-style/tests stuff that I'll clear myself in a while

chadwhitacre · 2016-08-31T16:38:28Z

@rohitpaulk Awesome! I've updated the todo and will look into these right after lunch ... 🍗

rohitpaulk · 2016-08-31T16:38:39Z

I like how all my comments have magically appeared as to-do items on the description of this ticket 😉

chadwhitacre · 2016-08-31T16:39:39Z

Let's communicate if/what we're working on to avoid stepping on each others' toes. I'm working on putting food into my head. :)

rohitpaulk · 2016-08-31T16:43:13Z

:) I'm working on " fix order of calls (and test!)" right now

rohitpaulk · 2016-08-31T17:14:12Z

Done with that, now looking at "refactor process_distribution for clarity"

chadwhitacre · 2016-08-31T17:15:55Z

refactor process_distribution for clarity

Should we rename it back to process_take since we're not really using "distribution" elsewhere in the code?

chadwhitacre · 2016-08-31T17:25:52Z

tests/py/test_billing_payday.py

+            self.payday.update_balances(cursor)
+
+        assert P('alice').balance == D('100')
+        assert P('picard').balance == D('400') # Owner receives whatever is left


Is this test expected to fail without 84f4b31? That was my expectation, but when I manually revert that commit locally this test still passes.

Also: Let's find the right place for this in the test suite. Where it is now it should have a name like test_pt_ for both conceptual grouping and easy filtering with py.test -k test_pt_. Maybe it would work better up in the TestPayday class?

Is this test expected to fail without 84f4b31? That was my expectation, but when I manually revert that commit locally this test still passes.

I see how this is confusing - the first test is only testing process_remainder in isolation, and I think it'd be clearer if we just remove process_takes from there.

The order should only matter in the new test that I wrote (that tests the interaction between those two).

Maybe it would work better up in the TestPayday class?

Yep, agreed - on it

chadwhitacre · 2016-08-31T17:30:58Z

Ducking out for gratipay/inside.gratipay.com#792, bbiab ...

rohitpaulk · 2016-08-31T18:00:22Z

Decided that the extra 'isolated' test wasn't worth it, so merged both into one. Since both process_remainder and process_takes are part of payin(), I placed the test in the TestPayin section.

rohitpaulk · 2016-08-31T18:00:31Z

Now to squash these commits a bit..

rohitpaulk · 2016-08-31T18:02:46Z

Done.

rohitpaulk · 2016-08-31T18:24:05Z

51af227 is what I could come up with to fix "refactor process_distribution for clarity"

Why I think it is better:

Unlike balance which goes from zero to positive and back during a payday, available was not created with the intention of being a variable during payday. available is a property on the team, and only the owner should be able to change it - we were piggybacking additional functionality on this column by decreasing it on every take.
Less comparison logic to read through - UPDATE payday_teams SET available_today = LEAST(available, balance); captures the idea more clearly than the two previous comparison statements we had.

rohitpaulk · 2016-08-31T18:28:11Z

Off for tonight, will check back tomorrow morning! 😴

chadwhitacre · 2016-08-31T19:23:10Z

Back from gratipay/inside.gratipay.com#792, catching up ...

chadwhitacre · 2016-09-01T00:17:52Z

allow owners to join teams—#4102 (comment)

Looking at this ...

chadwhitacre · 2016-09-01T11:03:59Z

I'm good to merge if you're good with 241bc51 and 3ac7fee, @rohitpaulk.

chadwhitacre · 2016-09-01T13:32:56Z

🎉 Woo-hoo! 💃

This was referenced Jul 29, 2016

bring back takes in payday #4025

Closed

Bring back takes for Team Gratipay #3994

Closed

chadwhitacre force-pushed the takes-payday branch from 854f948 to afdfd75 Compare August 1, 2016 13:59

chadwhitacre added this to the Bring Back Take-What-You-Want for Team Gratipay milestone Aug 3, 2016

chadwhitacre force-pushed the takes-payday branch from afdfd75 to fc87663 Compare August 16, 2016 21:11

chadwhitacre force-pushed the takes-payday branch 8 times, most recently from e4cd2f6 to b3c050e Compare August 24, 2016 17:38

chadwhitacre mentioned this pull request Aug 24, 2016

Include team members in MassPay #4111

Merged

3 tasks

chadwhitacre added the Review label Aug 24, 2016

chadwhitacre removed the Review label Aug 25, 2016

chadwhitacre force-pushed the takes-payday branch 2 times, most recently from 06f3998 to 66d1023 Compare August 25, 2016 16:26

chadwhitacre mentioned this pull request Aug 25, 2016

Refactor test_billing_exchanges #4113

Merged

1 task

chadwhitacre changed the title ~~bring back takes in payday~~ Bring back takes in payday Aug 25, 2016

chadwhitacre added the Review label Aug 25, 2016

rohitpaulk self-assigned this Aug 29, 2016

rohitpaulk reviewed Aug 31, 2016
View reviewed changes

chadwhitacre reviewed Aug 31, 2016
View reviewed changes

Increase surface area of process_remainder test

0ed1d31

rohitpaulk force-pushed the takes-payday branch from 174e8fc to 0ed1d31 Compare August 31, 2016 18:02

Simplify takes logic by adding available_today

51af227

Minor formatting cleanups

241bc51

chadwhitacre force-pushed the takes-payday branch from 0b88b4d to b71a39f Compare September 1, 2016 01:01

Allow owners to add themselves

3ac7fee

chadwhitacre force-pushed the takes-payday branch from b71a39f to 3ac7fee Compare September 1, 2016 01:12

chadwhitacre mentioned this pull request Sep 1, 2016

run Gratipay 222 gratipay/inside.gratipay.com#800

Closed

rohitpaulk merged commit 8c49b4b into master Sep 1, 2016

rohitpaulk deleted the takes-payday branch September 1, 2016 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back takes in payday #4102

Bring back takes in payday #4102

chadwhitacre commented Jul 29, 2016 •

edited

Loading

chadwhitacre commented Jul 29, 2016

chadwhitacre commented Aug 1, 2016

chadwhitacre commented Aug 24, 2016

chadwhitacre commented Aug 24, 2016

chadwhitacre commented Aug 25, 2016

chadwhitacre commented Aug 25, 2016

ghost commented Aug 25, 2016

chadwhitacre commented Aug 26, 2016

rohitpaulk Aug 31, 2016

chadwhitacre Sep 1, 2016

chadwhitacre Sep 1, 2016

chadwhitacre Sep 1, 2016 •

edited

Loading

rohitpaulk commented Aug 31, 2016

chadwhitacre commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

chadwhitacre commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

chadwhitacre commented Aug 31, 2016

chadwhitacre Aug 31, 2016 •

edited

Loading

rohitpaulk Aug 31, 2016

chadwhitacre commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

chadwhitacre commented Aug 31, 2016

chadwhitacre commented Sep 1, 2016

chadwhitacre commented Sep 1, 2016

chadwhitacre commented Sep 1, 2016

Bring back takes in payday #4102

Bring back takes in payday #4102

Conversation

chadwhitacre commented Jul 29, 2016 • edited Loading

TODO

chadwhitacre commented Jul 29, 2016

chadwhitacre commented Aug 1, 2016

chadwhitacre commented Aug 24, 2016

chadwhitacre commented Aug 24, 2016

chadwhitacre commented Aug 25, 2016

chadwhitacre commented Aug 25, 2016

ghost commented Aug 25, 2016

chadwhitacre commented Aug 26, 2016

rohitpaulk Aug 31, 2016

Choose a reason for hiding this comment

chadwhitacre Sep 1, 2016

Choose a reason for hiding this comment

chadwhitacre Sep 1, 2016

Choose a reason for hiding this comment

chadwhitacre Sep 1, 2016 • edited Loading

Choose a reason for hiding this comment

rohitpaulk commented Aug 31, 2016

chadwhitacre commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

chadwhitacre commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

chadwhitacre commented Aug 31, 2016

chadwhitacre Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

rohitpaulk Aug 31, 2016

Choose a reason for hiding this comment

chadwhitacre commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

rohitpaulk commented Aug 31, 2016

chadwhitacre commented Aug 31, 2016

chadwhitacre commented Sep 1, 2016

chadwhitacre commented Sep 1, 2016

chadwhitacre commented Sep 1, 2016

chadwhitacre commented Jul 29, 2016 •

edited

Loading

chadwhitacre Sep 1, 2016 •

edited

Loading

chadwhitacre Aug 31, 2016 •

edited

Loading