Created attachment 57856 [details] [review]
Bug 17702: Account types configuration - db changes

Test plan:
1) Read the diff and confirm that it does make sense
2) Comment what does not make sense ;)

Created attachment 57857 [details] [review]
Bug 17702: Account types configuration - Manual invoice and credit

Created attachment 57858 [details] [review]
Bug 17702: Account types configuration - Admin page

Test plan:
1) Go to admin home, note there is new Account types page in Patrons and
circulation section
2) Go to any other admin page and confirm there is link to Account types
in admin menu as welll
3) Go to Account types page
4) You should see two datatables, one for debit types and one for
credit types, ensure the datatables are working corectly
5) Try to create, edit and delete some debit types, note, that some of
them can't be deleted - they are neede for Koha internally
6) Do the same with credit types
7) Try to add some default amount to internal debits - like Acccount
management fee, New card, ...
8) Go to Patron fines page and play with account type select, when you
change thy account type, the description and amount should change as you
set it on the admin page
9) Try to add some fees and ensure they are inserted corectly
10) Pay the inserted fees
11) Try to add manual credit and ensure it is added corectly

I attached my current work, but stay with ASSIGNED status till I write tests ;)

Created attachment 58066 [details] [review]
Bug 17702: Add tests

Test plan:
  prove t/db_dependent/Koha/AccountTypes.t

Hi Josef,
When I tried to add a new debit type, I got the following error:
DBIx::Class::Storage::DBI::_dbh_execute(): Table 'koha_mykoha.account_debit_types' doesn't exist at /home/ckirby/koha/Koha/Objects.pm line 83

(In reply to Chris Kirby from comment #6)
> Hi Josef,
> When I tried to add a new debit type, I got the following error:
> DBIx::Class::Storage::DBI::_dbh_execute(): Table
> 'koha_mykoha.account_debit_types' doesn't exist at
> /home/ckirby/koha/Koha/Objects.pm line 83

Did you upgrade your db schema? I forget to mention it in test plan, sorry.

(In reply to comment #7), 
Hi Josef, thanks for the tip to upgrade the db schema.

When one creates a "manual invoice" for a patron using one of the created debit types, both the debit type code and the description are entered into the "Description of charges." However, for the pre-existing debit types, the debit type description is entered twice in the "description of charges".
Is there any reason for this difference?

When one creates a credit type, there is no field for default amount. Is there any reason for leaving a default amount out of the credit type but keeping it in the debit?

(In reply to Josef Moravec from comment #1)
> Created attachment 57856 [details] [review] [review]
> Bug 17702: Account types configuration - db changes
> 
> Test plan:
> 1) Read the diff and confirm that it does make sense
> 2) Comment what does not make sense ;)

According to db changes, is it possible to extend type_code from only 5 chars to (let's say) 10 chars? It would be very useful for some libraries having a longer list of payments not to be so cryptic. For instance, one of them (Czech only - sorry):
mvs	50|MVS
tA4	3|Tisk a kopie A4
t2sA4	4|Tisk a kopie A4 oboustranně
toA4	6|Tisk a kopie A4 + obrázek
tbA4	15|Celobarevná A4
t20A4	2|Tisk a kopie A4 nad 20 kusů
s20A4	3|Tisk a kopie A4 oboustranně nad 20 kusů
b20A4	10|Tisk a kopie A4 barevně nad 20 kusů
tA3	5|Tisk a kopie A3
t2sA3	7|Tisk a kopie A3 oboustranně
toA3	10|Tisk a kopie A3 + obrázek
tbA3	25|Celobarevná A3
t20A3	3|Tisk a kopie A3 nad 20 kusů
s20A3	5|Tisk a kopie A3 oboustranně nad 20 kusů
b20A3	20|Tisk a kopie A3 barevně nad 20 kusů
sken	7|skenování
eursl	3|eurosložka
papsl	5|papírová složka
taskm	3|taška malá
taskv	5|taška velká
prodj	?|prodej
plack	?|placky
eshop	?|e-shop
poukaz	110|Poukaz na registraci
REZER	10|Rezervace (ručně)
pspec	2|Papír speciál

(In reply to Chris Kirby from comment #8)
> (In reply to comment #7), 
> Hi Josef, thanks for the tip to upgrade the db schema.
> 
> When one creates a "manual invoice" for a patron using one of the created
> debit types, both the debit type code and the description are entered into
> the "Description of charges." However, for the pre-existing debit types, the
> debit type description is entered twice in the "description of charges".
> Is there any reason for this difference?

It's a bug, I am going to work on this a bit more...


> When one creates a credit type, there is no field for default amount. Is
> there any reason for leaving a default amount out of the credit type but
> keeping it in the debit?

Yes, the reason is, the when you are creating manual invoice, you often know the price of some kind of things, but you can't pretend which amount are people going to pay, or so...

(In reply to Radek Šiman (R-Bit Technology, s.r.o.) from comment #9)
> (In reply to Josef Moravec from comment #1)
> > Created attachment 57856 [details] [review] [review] [review]
> > Bug 17702: Account types configuration - db changes
> > 
> > Test plan:
> > 1) Read the diff and confirm that it does make sense
> > 2) Comment what does not make sense ;)
> 
> According to db changes, is it possible to extend type_code from only 5
> chars to (let's say) 10 chars? It would be very useful for some libraries
> having a longer list of payments not to be so cryptic. For instance, one of
> them (Czech only - sorry):

I have no problem with this.

Created attachment 85874 [details] [review]
Bug 17702: Account types configuration - db changes

Test plan:
1) Read the diff and confirm that it does make sense
2) Comment what does not make sense ;)

Created attachment 85875 [details] [review]
Bug 17702: Account types configuration - Manual invoice and credit

Created attachment 85876 [details] [review]
Bug 17702: Account types configuration - Admin page

Test plan:
1) Go to admin home, note there is new Account types page in Patrons and
circulation section
2) Go to any other admin page and confirm there is link to Account types
in admin menu as welll
3) Go to Account types page
4) You should see two datatables, one for debit types and one for
credit types, ensure the datatables are working corectly
5) Try to create, edit and delete some debit types, note, that some of
them can't be deleted - they are neede for Koha internally
6) Do the same with credit types
7) Try to add some default amount to internal debits - like Acccount
management fee, New card, ...
8) Go to Patron fines page and play with account type select, when you
change thy account type, the description and amount should change as you
set it on the admin page
9) Try to add some fees and ensure they are inserted corectly
10) Pay the inserted fees
11) Try to add manual credit and ensure it is added corectly

Created attachment 85877 [details] [review]
Bug 17702: Add tests

Test plan:
  prove t/db_dependent/Koha/AccountTypes.t

Hi Martin,
it's ready for testing? 

Thank You.

I tried test this patch but there're some problem with depencies.

Applying: Bug 22511: Update tests
Applying: Bug 22511: Update void method to use status
Applying: Bug 22511: Update UI to use accountlines.status
error: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/bootstrap/en/includes/account-table.inc).
error: could not build fake ancestor
Patch failed at 0001 Bug 22511: Update UI to use accountlines.status

Sorry, this one isn't ready at all for testing yet I'm afraid. I've been working through the tree of bugs above it and may will split this one up a little further too once a few more of those foundations are closer to being in.

Don't worry, I've certainly not forgotten this one though, we have lots of customers very eager to see it too.

Hi Martin,
we want this feature too. So, I'm ready for testing. Just set status if follow-up will be ready.

Thank you.

Probably the best way to get this one moving again at this point is for someone to follow the dependencies down from bug 22200. I think I've added reasonable test plans to each of the bugs now so they should be reasonably clear.

I intend to get back onto this bug in earnest very shortly, but I have my doubts that it'll make it into 19.05 at this stage.

The combination of bug 23049 and bug 23805 resolves 90% of this enhancement report.  What is missing is the UI for account credits.

I chose not to implement the UI for account credit definitions as part of bug 23805 as at the time of writing all credit types are hard coded in Koha.

I do wonder if we still require the 'PAYMENT_TYPE' authorized value and corresponding accountlines field however. It feels like that should be merged into account_credit_types and then a UI exposed much like the account_debit_type management page introduced in but 23049.

As such, I'm leaving this bug open awaiting those next steps.

Created attachment 98564 [details] [review]
Bug 17702: Add UI to manage account credit types

It is the same as for debit types.

It adds a new column `archived` in table `account_credit_types` that has
the same purpose than `account_debit_types.archived`

Test plan:
0. Apply patch && run updatedatabase && update_dbix_class_files
1. Go to Admin » Credit types
2. Add a new credit type, give it a code and a description and check
   'Can be manually added'
3. Go to a patron's accounting section, 'Create manual credit' tab
4. Verify that the new credit type appears
5. Return to Admin » Credit types and archive the credit type
6. Verify that the new credit type is not available anymore in 'Create
   manual credit'
7. Restore the credit type and verify that it is available again
8. Create a manual credit with the new credit type
9. Go to Reports » Cash register and make sure you can find the
   transaction by filtering on transaction type
10. Edit the new credit type and set some library limitations, make sure
    that the credit type doesn't appear if you're connected to a library
    you didn't selected, and that it appears if you're connected to a
    library you selected.

Note to QA team:
The change in Koha/Account.pm, I added it because otherwise Koha died
when adding a manual credit with a custom type.
In that case, offset type will default to 'Manual Credit'. I'm not sure
if that is the best thing to do. I'm open to suggestions :)

Not sure if it should still be BLOCKED, or if it is the right bug for this patch. Martin, can you take a look please ?

Thanks for taking this on Julian, 

I'll just reverse the dependencies and we can unblock this one, it's great to see a new face taking an interest in this work.

I'll give it a test later today :)

Created attachment 98659 [details] [review]
Bug 17702: Add UI to manage account credit types

It is the same as for debit types.

It adds a new column `archived` in table `account_credit_types` that has
the same purpose than `account_debit_types.archived`

Test plan:
0. Apply patch && run updatedatabase && update_dbix_class_files
1. Go to Admin » Credit types
2. Add a new credit type, give it a code and a description and check
   'Can be manually added'
3. Go to a patron's accounting section, 'Create manual credit' tab
4. Verify that the new credit type appears
5. Return to Admin » Credit types and archive the credit type
6. Verify that the new credit type is not available anymore in 'Create
   manual credit'
7. Restore the credit type and verify that it is available again
8. Create a manual credit with the new credit type
9. Go to Reports » Cash register and make sure you can find the
   transaction by filtering on transaction type
10. Edit the new credit type and set some library limitations, make sure
    that the credit type doesn't appear if you're connected to a library
    you didn't selected, and that it appears if you're connected to a
    library you selected.

Note to QA team:
The change in Koha/Account.pm, I added it because otherwise Koha died
when adding a manual credit with a custom type.
In that case, offset type will default to 'Manual Credit'. I'm not sure
if that is the best thing to do. I'm open to suggestions :)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 98660 [details] [review]
Bug 17702: (QA follow-up) Add new field to kohastructure

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Thanks for taking this on Julian, it's all looking good to me, signing off.

Created attachment 99306 [details] [review]
Bug 17702: Add UI to manage account credit types

It is the same as for debit types.

It adds a new column `archived` in table `account_credit_types` that has
the same purpose than `account_debit_types.archived`

Test plan:
0. Apply patch && run updatedatabase && update_dbix_class_files
1. Go to Admin » Credit types
2. Add a new credit type, give it a code and a description and check
   'Can be manually added'
3. Go to a patron's accounting section, 'Create manual credit' tab
4. Verify that the new credit type appears
5. Return to Admin » Credit types and archive the credit type
6. Verify that the new credit type is not available anymore in 'Create
   manual credit'
7. Restore the credit type and verify that it is available again
8. Create a manual credit with the new credit type
9. Go to Reports » Cash register and make sure you can find the
   transaction by filtering on transaction type
10. Edit the new credit type and set some library limitations, make sure
    that the credit type doesn't appear if you're connected to a library
    you didn't selected, and that it appears if you're connected to a
    library you selected.

Note to QA team:
The change in Koha/Account.pm, I added it because otherwise Koha died
when adding a manual credit with a custom type.
In that case, offset type will default to 'Manual Credit'. I'm not sure
if that is the best thing to do. I'm open to suggestions :)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 99307 [details] [review]
Bug 17702: (QA follow-up) Add new field to kohastructure

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Nice work everyone!

Pushed to master for 20.05

Created attachment 99322 [details] [review]
Bug 17702: (RM follow-up) Remove use of I18N tt

This patch removes the use of the I18N TT plugin in introduced for all
strings in the new template. We're not yet ready to use this plugin for
all strings as we need to further investigate performance for such a
move and perhaps add template compilation.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

I did investigate performance at the time of bug 15395. See https://gitlab.com/jajm/time-i18n.

Apologies for the incorrect patch attachment.

(In reply to Martin Renvoize from comment #31)
> Created attachment 99322 [details] [review] [review]
> Bug 17702: (RM follow-up) Remove use of I18N tt
> 
> This patch removes the use of the I18N TT plugin in introduced for all
> strings in the new template. We're not yet ready to use this plugin for
> all strings as we need to further investigate performance for such a
> move and perhaps add template compilation.
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

I somehow missed that performance comparison.. I'll have a look, thanks for bringing it up..

Jonathan is looking to work on a mass replacement of existing strings to move us over to this standard as a mass change and also update the QA scripts and xt test for filters to prevent the false failures we were seeing on this patch.

Thanks Julian..

enhancement not backported to 19.11.x