Bug 22610 - SIP Payment Types should be moved out of accountype
Summary: SIP Payment Types should be moved out of accountype
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 23049 18786
Blocks: 17702
  Show dependency treegraph
 
Reported: 2019-03-29 16:37 UTC by Martin Renvoize
Modified: 2019-06-05 11:39 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22610: Update SIP payment accounttypes (1.87 KB, patch)
2019-04-24 13:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: Update SIP2 to use payment_type (6.61 KB, patch)
2019-04-24 13:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: Update SIP payment accounttypes (1.87 KB, patch)
2019-05-08 11:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: Update SIP2 to use payment_type (6.39 KB, patch)
2019-05-08 11:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: (follow-up) Catch non-standard SIP codes (1.69 KB, patch)
2019-05-08 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: (follow-up) Catch non-standard SIP codes (1.73 KB, patch)
2019-05-08 11:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: Update SIP payment accounttypes (1.87 KB, patch)
2019-05-09 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: Update SIP2 to use payment_type (6.39 KB, patch)
2019-05-09 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: (follow-up) Catch non-standard SIP codes (1.73 KB, patch)
2019-05-09 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: (follow-up) Correction to accounts tests (1.96 KB, patch)
2019-05-09 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: (QA follow-up) Add unit tests for FeePayment (2.67 KB, patch)
2019-05-09 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22610: Update SIP payment accounttypes (1.94 KB, patch)
2019-05-09 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22610: Update SIP2 to use payment_type (6.46 KB, patch)
2019-05-09 11:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22610: (follow-up) Catch non-standard SIP codes (1.79 KB, patch)
2019-05-09 11:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22610: (follow-up) Correction to accounts tests (2.03 KB, patch)
2019-05-09 11:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22610: (QA follow-up) Add unit tests for FeePayment (2.74 KB, patch)
2019-05-09 11:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22610: (QA follow-up) Make upgrade query more accurate (1.07 KB, patch)
2019-05-09 11:06 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-03-29 16:37:56 UTC
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.
Comment 1 Martin Renvoize 2019-04-24 13:25:38 UTC
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
Comment 2 Martin Renvoize 2019-04-24 13:25:41 UTC
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.
Comment 3 Martin Renvoize 2019-05-08 11:19:39 UTC
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
Comment 4 Martin Renvoize 2019-05-08 11:19:42 UTC
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.
Comment 5 Martin Renvoize 2019-05-08 11:23:03 UTC
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.
Comment 6 Martin Renvoize 2019-05-08 11:53:49 UTC
Created attachment 89470 [details] [review]
Bug 22610: (follow-up) Catch non-standard SIP codes
Comment 7 Martin Renvoize 2019-05-08 11:58:11 UTC
Created attachment 89471 [details] [review]
Bug 22610: (follow-up) Catch non-standard SIP codes
Comment 8 Liz Rea 2019-05-08 19:33:50 UTC
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
Comment 9 Liz Rea 2019-05-08 19:35:13 UTC
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.
Comment 10 Martin Renvoize 2019-05-09 08:10:13 UTC
(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 :)
Comment 11 Martin Renvoize 2019-05-09 10:33:45 UTC
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 :)
Comment 12 Martin Renvoize 2019-05-09 10:37:33 UTC
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
Comment 13 Martin Renvoize 2019-05-09 10:37:35 UTC
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.
Comment 14 Martin Renvoize 2019-05-09 10:37:38 UTC
Created attachment 89494 [details] [review]
Bug 22610: (follow-up) Catch non-standard SIP codes
Comment 15 Martin Renvoize 2019-05-09 10:37:41 UTC
Created attachment 89495 [details] [review]
Bug 22610: (follow-up) Correction to accounts tests
Comment 16 Martin Renvoize 2019-05-09 10:37:44 UTC
Created attachment 89496 [details] [review]
Bug 22610: (QA follow-up) Add unit tests for FeePayment
Comment 17 Kyle M Hall 2019-05-09 11:05:28 UTC
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>
Comment 18 Kyle M Hall 2019-05-09 11:06:14 UTC
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>
Comment 19 Kyle M Hall 2019-05-09 11:06:17 UTC
Created attachment 89500 [details] [review]
Bug 22610: (follow-up) Catch non-standard SIP codes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2019-05-09 11:06:19 UTC
Created attachment 89501 [details] [review]
Bug 22610: (follow-up) Correction to accounts tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2019-05-09 11:06:21 UTC
Created attachment 89502 [details] [review]
Bug 22610: (QA follow-up) Add unit tests for FeePayment

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2019-05-09 11:06:24 UTC
Created attachment 89503 [details] [review]
Bug 22610: (QA follow-up) Make upgrade query more accurate

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Liz Rea 2019-05-09 14:10:04 UTC
awesome, I'm happy if y'all are happy :)
Comment 24 Jonathan Druart 2019-05-09 16:43:40 UTC
Is it really useful to check for the others PayXX (in the updatedatabase entry)?
Comment 25 Jonathan Druart 2019-05-09 16:47:52 UTC
Why not switch the sip_type (00, 01, 02) to something meaningful?

Also, we are loosing the translation...
Comment 26 Martin Renvoize 2019-05-10 07:17:46 UTC
(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.
Comment 27 Jonathan Druart 2019-05-10 13:02:49 UTC
(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)")
Comment 28 Martin Renvoize 2019-05-14 13:53:08 UTC
Good idea, that would work in the interim. Thanks for the suggestion Jonathan.. sometimes I just can't see the wood for the trees!