Description
Josef Moravec
2016-11-30 11:06:31 UTC
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. Created attachment 99454 [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> 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 |