[OPEN-ILS-DEV] SIP2 Fee Payment Code

[Jason Stephenson]

Quoting Mike Rylander <mrylander at gmail.com>:

> On Thu, May 12, 2011 at 4:04 PM, Jason Stephenson  
> <jstephenson at mvlc.org> wrote:

[I'm deleting irrelevant bits of discussion.]

>> Payments are applied in a very rudimentary fashion, though if the self check
>> sends a transaction id that it perhaps got earlier from the server, it will
>> try to apply the payment to the corresponding transaction for the patron.
>>
>
> The way you're applying payments to unidentified transactions is
> more-or-less in line with what the staff client does when it all
> transactions are checked in the billing interface.  With transaction
> id support, the client can simulate specific bill payment, so that's
> good.  I'm a little concerned about piggy-backing this on a generic
> find-all-with-balance call, since it could certainly be done with a
> direct lookup, but that's an optimization for later, perhaps, assuming
> identified transactions are common enough to support that codepath.

OK. A good suggestion. I'll work on that in the coming days/weeks.  
Think I'll expand it to also look up the fee id, dunno which it should  
try first if they're both present, greater than 0, and not equal to  
each other, but it'll try one, then the other, and only fail in that  
case if neither matches a bill on the patron.


>
>> The code still needs more testing and perhaps more logging. I also need to
>> add the ability to process credit card payments. Currently it is hard coded
>> to handle all payments as cash.
>>
>
> Right ... is there a way for a generic SIP client to pass this
> information, or will we likely be looking at an EG-user (SIP client
> user) setting, or SIP config file setting, or something else entirely?
>  Or will this require some specialized extensions to 36/37?

I assume you're talking about the credit card payments. In looking at  
it some more, the SC sends over a payment type, but it doesn't seem to  
send over credit card information for billing, at least not according  
to the standard SIP2 protocol reference. I guess the assumption is  
that the SC handles the credit card authorization, and merely informs  
the ACS if the payment was successful. In that case, I'm not sure what  
to do with a credit card payment, so until I learn a bit more or  
someone provides some direction (whichever comes first) I'll leave the  
code entering a cash_payment regardless of what payment type the SC  
sends over.


>
> A couple minor style nits: please declare your for[each] loop vars
> inline, and please cuddle your braces! ;)

NP. I'll take of that today.


>
> There are also some very idiomatic things going on in the Transaction
> subclass, but I can't blame those on you -- the surrounding (peer)
> modules do the same things (like slice assignments).
>
> What I would /really/ like to see is lots of inline comments
> explaining the assumptions in do_fee_payment().  For instance, the
> 'else' of the outermost 'if' block is magical and could use explicit
> documentation, as could IMO the central foreach loop.

Yes, commenting the code is something that I need to do. As I revise  
it in light of your suggestions, I'll add comments to explain things.



>
> One last thought before I set this aside for now, I wonder if it
> wouldn't be best to batch up all sub-payments to submit them in one
> go.  This way, if one payment fails they all fail, and you don't have
> to deal with partial refunds or other strange issues -- just kick it
> all back to the SIP client to handle atomically.

Yes, another excellent suggestion. I'll change the payment creation to  
be done in a batch as I work on your other suggestions regarding the  
transaction_id, etc.


>
>> If anyone wants to play with it and try it on their self checks you can get
>> the code from the following git repository:
>>
>> git://git.mvlcstaff.org/jason/ILS
>>
>> You will want the SIP23738 branch. (HTTP URL should work as well for
>> cloning/merging.)
>>
>> You will also want to get the SIPServer code from the following repository:
>>
>> git://git.mvlcstaff.org/jason/SIPServer
>>
>> Here, you will want the fixes branch. This branch contains fixes to the
>> SIPServer code from github so that checksums work properly with clients, and
>> so that the Fee Paid message is correctly parsed by the server.
>
> I've not looked at this at all ... and won't be the best to do so.  Anyone?

I have created a new branch on my SIPServer public repo called MVLC.  
This contains a merge of all the code changes that we're running here  
for anyone who wants to see what we're using for our SIPServer. This  
includes the fixes branch and an extra patron fields branch that  
Thomas Berezansky created. (I believe there might be a Launchpad  
wishlist item for the EG side of the latter code.)

Naturally, it's all licensed under the GPL 2 or later, though I think  
the headers in my SIPServer branch might need updating.

>
>>
>> The above code has all been tested on a trunk installation of Evergreen so
>> far, but it *should* merge and work with a 2.1 or 2.0 branch, since I don't
>> think there have been any changes to SIP in the mean time.
>>
>
> As far as the authoritative repo, it won't be able to go into anything
> but trunk since we're in beta (aka feature freeze) for 2.1 and we
> haven't branched 2.2 yet.  Of course that shouldn't stop anyone from
> testing it if they feel so inclined.

Right. I think the code will apply cleaning back to 2.0, possibly back  
to 1.6, if anyone wants the features, but I make no guarantees. It  
might work, and it might kill kittens.



> Thanks again, Jason.
>
> --
> Mike Rylander

Thanks, Mike, for your helpful suggestions. I'll send another email to  
the list when I have implemented and tested them.

Jason Stephenson
Merrimack Valley Library Consortium