Charge in arrears

[rorepo]

Implemented the core part of the solution for issue "charge minimum in arrears, not in advance #3378"
Overview of the solution:
i) Added a new column giving_due in payment_instructions. To be populated/incremented with the corresponding amount, each time the amount is not charged due to being under the minimum charge threshold
ii) After creation of payday_payment_instructions, the giving_due value for each participant-team combination is updated with the sum of giving_due values for that combination, from payment_instructions.
iii) The 'giving_today' value for each participant is calculated as sum(amount + giving_due) from all records for that participant in payday_payment_instructions
iv) The 'giving_today' value is checked against the MINIMUM_CHARGE threshold before creating card holds if at/above the threshold. Have a doubt here - whether it's the basic amount or the upcharged amount that is to be used in the check
v) In the case of participants who have been charged, all giving_due values in payment_instructions for those participants are reset to 0
vi) In the case of participants who have not been charged, the giving_due value in the current record in payment_instructions is updated to amount + giving_due

Will await comments/suggestions before proceeding further.

Further changes planned:
i) Add a new column giving_due in participants, to cache the total giving_due value for participants
ii) Update the UI to include the above

[chadwhitacre]

!m @rorepo

When you're ready, go ahead and apply the "Review" label.

[chadwhitacre]

4-space indents instead of tabs, please.

[chadwhitacre]

Reformatting for readability. :-)

---

Implemented the core part of the solution for issue "charge minimum in arrears, not in advance #3378"
Overview of the solution:

1. Added a new column giving_due in payment_instructions. To be populated/incremented with the corresponding amount, each time the amount is not charged due to being under the minimum charge threshold
2. After creation of payday_payment_instructions, the giving_due value for each participant-team combination is updated with the sum of giving_due values for that combination, from payment_instructions.
3. The giving_today value for each participant is calculated as sum(amount + giving_due) from all records for that participant in payday_payment_instructions
4. The giving_today value is checked against the MINIMUM_CHARGE threshold before creating card holds if at/above the threshold. Have a doubt here - whether it's the basic amount or the upcharged amount that is to be used in the check
5. In the case of participants who have been charged, all giving_due values in payment_instructions for those participants are reset to 0
6. In the case of participants who have not been charged, the giving_due value in the current record in payment_instructions is updated to amount + giving_due

Will await comments/suggestions before proceeding further.

Further changes planned:

1. Add a new column giving_due in participants, to cache the total giving_due value for participants
2. Update the UI to include the above

[chadwhitacre]

Have a doubt here - whether it's the basic amount or the upcharged amount that is to be used in the check

I think it should be the basic amount, because that will be more natural to people. "I give 50¢/wk, so I'll get charged every 20 weeks," rather than getting charged in the 19th week because the fee puts you over the top. I mean, that's pretty minor. It's implemented as checking the basic amount right now, iirc.

[chadwhitacre]

Wtf?

[chadwhitacre]

We're going to need at least a few tests for this.

[chadwhitacre]

I mean for this PR as a whole. I'm not seeing any tests for the new functionality.

[chadwhitacre]

What's going on with this construction? One does not equal two. :-)

[chadwhitacre]

#3675 (comment):

The '1 = 2' thing is an old trick I've used quite a lot - when I wished to copy the structure (full or part) of some table, without the data. Might be overkill here for just one column, but for one thing, all other table creations in this script are in the form of selects and also, this usage means it will not break if the participant column is modified in payday_payment_instructions/payment_instructions.

[rohitpaulk]

@whit537 - Can you try re-enabling the repo at Travis?

[rorepo]

Have corrected the formatting and the extraneous argument.

The '1 = 2' thing is an old trick I've used quite a lot - when I wished to copy the structure (full or part) of some table, without the data. Might be overkill here for just one column, but for one thing, all other table creations in this script are in the form of selects and also, this usage means it will not break if the participant column is modified in payday_payment_instructions/payment_instructions.

Working on the tests now. Haven't used mock or vcr before, so took a while to figure these out.

[chadwhitacre]

Have corrected the formatting and the extraneous argument.

@rorepo Note that I pushed a couple commits towards this end as well: 22867fd & e087c60.

[chadwhitacre]

Can you try re-enabling the repo at Travis?

It appears to be re-enabled already. O.O

The failure on e087c60 was spurious ("GitHub rate limit exceeded"), so I restarted that build.

[chadwhitacre]

Working on the tests now. Haven't used mock or vcr before, so took a while to figure these out.

Cool. You almost certainly don't need vcr to test this code, and probably not even mock.

[rorepo]

@whit537 - I'm afraid I didn't get your comment about not needing vcr/mock. The way I figured it, I'd have to update test_billing_payday.py, which uses these. I'd have to add new tests as well as update existing ones, because the 'charge-in-arrears' functionality would make some of these invalid e.g. 'test_payday_moves_money', which was for a payment of $6 and which would now not be charged since this is now below the charge threshold.

[chadwhitacre]

@rorepo I guess you're right. :-)

I was just going on a hunch, since you're actually touching code, don't mind me. Sorry! :-)

[rorepo]

Looking at the tests exposed a few bugs as well as the fact that quite a few of the existing tests would have to change.

The key points in tests:

• test_billing_payday.py: test_payday_moves_money needed to be replaced by two new methods - test_payday_moves_money_above_min_charge and test_payday_does_not_move_money_below_min_charge, to test the behaviour when the amount to be charged is >= MINIMUM_CHARGE and < MINIMUM_CHARGE, respectively
• Other tests have needed related changes. Primarily, to increase amount to >= MINIMUM_CHARGE, to make sure the test works as intended

@whit537 I've removed the method park_payment_instructions from payday.py. This had actually been unused - I'd forgotten to remove it before pushing

[chadwhitacre]

The commit log on this PR looks weird. Why are there two of some commits? Why is the 1880 version bump on here?

[chadwhitacre]

I think maybe we need a rebase of this branch to clean up the commits.

[chadwhitacre]

I pulled out the first commit, 13faa2a, into a new PR (#3695). I'm getting bit by #3579 over on #3694, and also had this fix on #3568. Let's take care of that bug, already! :-)

[chadwhitacre]

Rebasing on master got rid of the 1880 version bump. Now for the duplicate commits ...

[rorepo]

Don't quite understand, @whit537. I did do a rebase before merging and I'd assume that's how the 1880 version came in.
Also, which are the duplicated commits you mean? If you mean the code formatting and cleanup, the latest one is what I did following some errors/warnings shown during the rebase about trailing spaces. Also removed an 'import time' I'd put in in between while running test_history.py.

[chadwhitacre]

@rorepo After rebasing on master, here are the commits I'm seeing:

It looks like we have duplicate commits for:

• Preliminary commit - charging in arrears
• Schema changes moved to branch.sql
• Clean up code formatting
• Remove extraneous argument

[chadwhitacre]

Don't quite understand, @whit537.

Neither do I. :-) My git-fu is not that strong.

I think at this point we should rebase to squash the duplicate commits, and then make sure the overall diff is as expected, and then forge ahead. I'm going to rebase ... unless you object?

[chadwhitacre]

$ git rebase -i f887fdd5f8d146d4c2ff5849143fe38059363c3e

... presents me with ...

pick 97ead4e Preliminary commit - charging in arrears                                                         
pick 5b58b70 Schema changes moved to branch.sql                                                               
pick 14045c6 Clean up code formatting                                                                         
pick d9980f5 Remove extraneous argument                                                                       
pick 816776f Preliminary commit - charging in arrears                                                         
pick af42c27 Schema changes moved to branch.sql                                                               
pick 005573e Clean up code formatting                                                                         
pick fe2c70f Remove extraneous argument                                                                       
pick 1b3a373 Refined and added tests                                                                          
pick 98f0f3e Code formatting and cleanup                                                                      

# Rebase f887fdd..98f0f3e onto f887fdd

[rorepo]

You're welcome to proceed, @whit537. My git-fu is almost certainly weaker than yours, in any case :-)
Saw the duplicates. Hadn't checked that far back earlier.

[rorepo]

Good luck :-)

[chadwhitacre]

Thanks! :-)

[chadwhitacre]

@rorepo I looks like we have duplicate commits again ...

[chadwhitacre]

I ran this locally to get your latest commit without the two extra commits I was seeing with feeec33 (2493f71 and 2493f71):

$ git fetch
[...]
$ git cherry-pick feeec33
$ git push --force

[chadwhitacre]

And now we're down to a pyflakes error! :-)

[chadwhitacre]

Aaaaaaaand that's the error you were pointing out at #3675 (comment). :-(

[chadwhitacre]

It's your latest commit, a8a6bc4, that adds the duplicate methods to Participant.

[chadwhitacre]

I've force-pushed a8a6bc4 off the branch, and now we're green again. What was the intention with that commit? The code doesn't seem to match what it says on the tin:

Updated to use current_payment_instructions view
current_payment_instructions view used in place of the new payment_instructions_due table which has now been removed

[chadwhitacre]

feeec33 and a8a6bc4 are identical.

[chadwhitacre]

Once we land this, should we issue refunds for the remaining balances of people that we charged in advance? It would make #3539 easier.

[chadwhitacre]

@rorepo I think we need you to redo your last commit.

[rorepo]

I think where my merges go wrong is when things go to conflict resolution. Either I'm not reading/handling the conflicts right or git is deficient in picking up/highlighting things like duplicated code. But my understanding of git is improving, so hopefully this won't be a perennial problem.

A few thoughts on refunding remaining balances charged in advance:

1. We'd need a threshold for this as well, otherwise we'd end up trying to refund even below the min payout value, say.
2. There's also the aspect that with small balances which would be consumed in a couple of weeks, there may not be much point in doing the refund. So maybe we can link the threshold to this, say, 2 weeks into subscription amount.
3. In any case, I think we should try to eliminate the situation where we charge (in arrears) and end up refunding excess balance right away.
4. Remembering @rohitpaulk's comment a while back, that excess balances as a result of being charged in advance were a relatively low percentage of transaction volumes - what would be the percentage of such balances once we've switched to charging in arrears?

[chadwhitacre]

Good work, @rorepo. :-)

!m @rorepo

[rorepo]

:-) Glad there were no more problems.

What about the refund bit? Will it make sense to start looking at this?

[chadwhitacre]

@rorepo Well, we'll see if there any more problems on Thursday! :-)

I've reticketed the refund as #3763.

!m @rorepo