Bug 18786 introduced a dedicated payment_type field to the database and an authorized value for it's population.. we should move the SIP payment type appendages from accounttype into this AV list.
Created attachment 88619 [details] [review] Bug 22610: Update SIP payment accounttypes This DB update updates the Pay00, Pay01 and Pay02 accounttypes to all be 'Pay' and adds the corresponding payment_types into the authorised_values
Created attachment 88620 [details] [review] Bug 22610: Update SIP2 to use payment_type The SIP2 interface has long used a set of accounttypes to denote payment types. Now we have an authorised list of payment_types and a payment_type field in accountlines this patch alters the logic to use it.
Created attachment 89460 [details] [review] Bug 22610: Update SIP payment accounttypes This DB update updates the Pay00, Pay01 and Pay02 accounttypes to all be 'Pay' and adds the corresponding payment_types into the authorised_values
Created attachment 89461 [details] [review] Bug 22610: Update SIP2 to use payment_type The SIP2 interface has long used a set of accounttypes to denote payment types. Now we have an authorised list of payment_types and a payment_type field in accountlines this patch alters the logic to use it.
Freshly rebased. Test Plan: 1) Check for presence of Pay01, Pay02 and Pay03 accounttypes in your database 2) Apply first patch and notice those occurrences have all changed to 'Pay' and an appropriate 'payment_type' has been set to catch the '01', '02' and '03' meanings 3) Apply second patch 4) Do some payments via SIP2 and note no new Pay01 etc accounttypes are created and instead payment types are populated for the appropriate lines.
Created attachment 89470 [details] [review] Bug 22610: (follow-up) Catch non-standard SIP codes
Created attachment 89471 [details] [review] Bug 22610: (follow-up) Catch non-standard SIP codes
I think libraries are going to miss these: [% CASE 'Pay00' %]Payment (cash via SIP2) [% CASE 'Pay01' %]Payment (VISA via SIP2) [% CASE 'Pay02' %]Payment (credit card via SIP2) Right now, *near as I can tell* we are only ever going to get "Cash via SIP2" in the interface with this patch, is that correct? I tried with the following sip emulator settings: I'd expect this to be Credit Card in the interface: kohadev-koha@b46acddf5bb7:/kohadevbox/koha$ perl misc/sip_cli_emulator.pl --sip_user=term1 --sip_pass=term1 --patron=2 --item='12' --fee-amount=15.00 --fee-type=03 -m fee_paid -a 127.0.0.1 -p 6001 -l MAIN I'd expect this to be VISA in the interface: kohadev-koha@b46acddf5bb7:/kohadevbox/koha$ perl misc/sip_cli_emulator.pl --sip_user=term1 --sip_pass=term1 --patron=2 --item='12' --fee-amount=15.00 --fee-type=02 -m fee_paid -a 127.0.0.1 -p 6001 -l MAIN This one is correct, the default 01, with Cash: kohadev-koha@b46acddf5bb7:/kohadevbox/koha$ perl misc/sip_cli_emulator.pl --sip_user=term1 --sip_pass=term1 --patron=2 --item='12' --fee-amount=15.00 -m fee_paid -a 127.0.0.1 -p 6001 -l MAIN Is there something I need to change in my setup here to make this work the way libraries will expect? Cheers, Liz
actually, I'm wrong about the numbers, but we're still not getting the right SIP fee type for the different codes. The emulator defaults to 01, which is VISA, but it says Cash in the description.
(In reply to Liz Rea from comment #8) > I think libraries are going to miss these: > > [% CASE 'Pay00' %]Payment (cash via SIP2) > [% CASE 'Pay01' %]Payment (VISA via SIP2) > [% CASE 'Pay02' %]Payment (credit card via SIP2) > Will they really miss the above? We've not gotten right of the data, only moved it into the more appropriate field 'payment_type' as apposed to lumping it in with 'accounttype'. That field is pretty much always displayed next to the accounttype column so I don't understand what they'll miss? > Right now, *near as I can tell* we are only ever going to get "Cash via > SIP2" in the interface with this patch, is that correct? I tried with the > following sip emulator settings: I'm looking into this now as well as writing the unit tests. Thanks for testing :)
Right, I've gotten to the bottom of this.. it's tester error :).. man SIP is 'fun' to test. So, I tested with: perl misc/sip_cli_emulator.pl --sip_user=staff --sip_pass=staff --patron=dir --fee-identifier=697 --fee-amount=0.50 --payment-type=02 -m fee_paid -a 127.0.0.1 -p 6001 -l MAIN Note the 'payment-type' as apposed to 'fee-type' used in your version.. to be entirely honest I have no idea what fee-type is used for here.. I would suggest it's not for payment but for creation of new fee's only? Anywho.. the code does appear to work as expected (defaulting to 'Cash' when no payment-type is specified, but using the payment type if one is). Just about to upload a Unit test now too. Thanks again for testing Liz :)
Created attachment 89492 [details] [review] Bug 22610: Update SIP payment accounttypes This DB update updates the Pay00, Pay01 and Pay02 accounttypes to all be 'Pay' and adds the corresponding payment_types into the authorised_values
Created attachment 89493 [details] [review] Bug 22610: Update SIP2 to use payment_type The SIP2 interface has long used a set of accounttypes to denote payment types. Now we have an authorised list of payment_types and a payment_type field in accountlines this patch alters the logic to use it.
Created attachment 89494 [details] [review] Bug 22610: (follow-up) Catch non-standard SIP codes
Created attachment 89495 [details] [review] Bug 22610: (follow-up) Correction to accounts tests
Created attachment 89496 [details] [review] Bug 22610: (QA follow-up) Add unit tests for FeePayment
Created attachment 89498 [details] [review] Bug 22610: Update SIP payment accounttypes This DB update updates the Pay00, Pay01 and Pay02 accounttypes to all be 'Pay' and adds the corresponding payment_types into the authorised_values Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 89499 [details] [review] Bug 22610: Update SIP2 to use payment_type The SIP2 interface has long used a set of accounttypes to denote payment types. Now we have an authorised list of payment_types and a payment_type field in accountlines this patch alters the logic to use it. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 89500 [details] [review] Bug 22610: (follow-up) Catch non-standard SIP codes Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 89501 [details] [review] Bug 22610: (follow-up) Correction to accounts tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 89502 [details] [review] Bug 22610: (QA follow-up) Add unit tests for FeePayment Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 89503 [details] [review] Bug 22610: (QA follow-up) Make upgrade query more accurate Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
awesome, I'm happy if y'all are happy :)
Is it really useful to check for the others PayXX (in the updatedatabase entry)?
Why not switch the sip_type (00, 01, 02) to something meaningful? Also, we are loosing the translation...
(In reply to Jonathan Druart from comment #24) >Is it really useful to check for the others PayXX (in the updatedatabase entry)? (In reply to Jonathan Druart from comment #25) > Why not switch the sip_type (00, 01, 02) to something meaningful? The types come directly from the SIP messages. That's part of the point of doing this, it allows for SIP extensions and types we are not currently handling by allowing users to add them as authorized values of the for SIPXX where the XX is the code they've found to be in use. It allows them to add a meaningful description of the type of payment. (For example, if 3M decided to have a unit that accepted paypal and assigned if 05 arbitrarily). > > Also, we are loosing the translation... Yup, I was sort of hoping no-one would mind this at the moment (none of the payment_types are translatable as it stands). This was meant to be a step towards bug 17702 (or perhaps I'll add another step between this and that bug) where I intend to make split accounttypes into credit and debit types and also move payment_types out of the authorized value system and into their own specialised table. I intend to make all of that translatable.
(In reply to Martin Renvoize from comment #26) > (In reply to Jonathan Druart from comment #24) > > Also, we are loosing the translation... > > Yup, I was sort of hoping no-one would mind this at the moment (none of the > payment_types are translatable as it stands). This was meant to be a step > towards bug 17702 (or perhaps I'll add another step between this and that > bug) where I intend to make split accounttypes into credit and debit types > and also move payment_types out of the authorized value system and into > their own specialised table. I intend to make all of that translatable. As it is a regression I think it should be fixed here. Why not simply keep (and adjust) the lines in the templates? (if authorised_values eq "Payment (cash via SIP2)" then display "Payment (cash via SIP2)")
Good idea, that would work in the interim. Thanks for the suggestion Jonathan.. sometimes I just can't see the wood for the trees!
Created attachment 91638 [details] [review] Bug 22610: Update SIP payment accounttypes This DB update updates the Pay00, Pay01 and Pay02 accounttypes to all be 'Pay' and adds the corresponding payment_types into the authorised_values Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 91639 [details] [review] Bug 22610: Update SIP2 to use payment_type The SIP2 interface has long used a set of accounttypes to denote payment types. Now we have an authorised list of payment_types and a payment_type field in accountlines this patch alters the logic to use it. Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 91640 [details] [review] Bug 22610: (follow-up) Catch non-standard SIP codes Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 91641 [details] [review] Bug 22610: (follow-up) Correction to accounts tests Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 91642 [details] [review] Bug 22610: (QA follow-up) Add unit tests for FeePayment Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 91643 [details] [review] Bug 22610: (QA follow-up) Make upgrade query more accurate Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work! Pushed to master for 19.11.00
Enhancement not pushed to 19.05.x