Bug 19036 - Number payment receipts / payment slips
Summary: Number payment receipts / payment slips
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
: 20309 (view as bug list)
Depends on: 14826
Blocks: 13985
  Show dependency treegraph
 
Reported: 2017-08-04 10:35 UTC by Katrin Fischer
Modified: 2020-07-31 06:05 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19036: Add ability to auto generate a number for account credits (12.88 KB, patch)
2020-02-20 08:48 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19036: Add ability to auto generate a number for account credits (90.08 KB, patch)
2020-02-20 09:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19036: Add ability to auto generate a number for account credits (13.12 KB, patch)
2020-02-20 09:06 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19036: Add ability to auto generate a number for account credits (13.23 KB, patch)
2020-02-26 19:50 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 19036: Add ability to auto generate a number for account credits (13.08 KB, patch)
2020-03-24 12:06 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19036: Add ability to enable credit number for only some credit types (19.31 KB, patch)
2020-03-24 12:06 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19036: Add ability to auto generate a number for account credits (13.14 KB, patch)
2020-03-26 17:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19036: Add ability to enable credit number for only some credit types (19.38 KB, patch)
2020-03-26 17:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19036: Add ability to auto generate a number for account credits (12.88 KB, patch)
2020-06-25 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19036: Add ability to enable credit number for only some credit types (19.38 KB, patch)
2020-06-25 09:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19036: (QA follow-up) Use Koha::DateUtils (2.08 KB, patch)
2020-06-25 09:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19036: DBIC Schema Updates (3.17 KB, patch)
2020-06-25 09:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19036: (QA follow-up) Fix test plan (652 bytes, patch)
2020-07-10 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2017-08-04 10:35:03 UTC
In some areas it's required to sequentially number payment slips / receipts. 

At the moment this is not easily possible in Koha, as we don't have a sequential internal number for the payments, but will hopefully be possible in some point in the future with the work on the Accounts module.
Comment 1 Katrin Fischer 2018-02-28 06:37:07 UTC
*** Bug 20309 has been marked as a duplicate of this bug. ***
Comment 2 Martin Renvoize 2019-07-18 16:15:04 UTC
Do these actually need to be sequential?

At this point we could use the accountlineid for the 'payment' type accountline that's associated with the slip.
Comment 3 Katrin Fischer 2019-10-02 13:20:38 UTC
As I understood this is a legal requirement in some countries, possibly relating to anti-corruption laws, where they have to keep a tight track of all the monetary transactions. I believe the number has to be sequential to avoid problems (like getting asked about missing receipts for missing numbers).
Comment 4 Katrin Fischer 2019-10-02 13:22:48 UTC
Ah, also accountlines_id is on the wrong level I think - we want to number the payments - a payment might include changes to multiple accountlines.
Comment 5 Julian Maurice 2020-02-20 08:48:41 UTC Comment hidden (obsolete)
Comment 6 Julian Maurice 2020-02-20 09:02:59 UTC Comment hidden (obsolete)
Comment 7 Julian Maurice 2020-02-20 09:06:19 UTC Comment hidden (obsolete)
Comment 8 Michal Denar 2020-02-25 20:16:09 UTC
Hi Julian,
I've tested bug. All works great, but I'm not able to add account credit number on print slip. It generate just empty space. But maybe it's my mistake. Can you double-check this part?

Thank you.
Comment 9 Julian Maurice 2020-02-26 06:52:57 UTC
(In reply to Michal Denar from comment #8)
> Hi Julian,
> I've tested bug. All works great, but I'm not able to add account credit
> number on print slip. It generate just empty space. But maybe it's my
> mistake. Can you double-check this part?
> 
> Thank you.

Hi Michal,

You need to edit the ACCOUNT_CREDIT letter ("Print" message type), search for a FOREACH loop like this:

  [% FOREACH account IN accounts %]

And add [% account.credit_number %] inside that loop
Comment 10 Michal Denar 2020-02-26 11:46:11 UTC
Hi Julian,
it's clear. I'll try to test it again soon.
Thank You.
Comment 11 Michal Denar 2020-02-26 19:50:58 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2020-02-28 16:09:38 UTC
Comment on attachment 99676 [details] [review]
Bug 19036: Add ability to auto generate a number for account credits

Review of attachment 99676 [details] [review]:
-----------------------------------------------------------------

Great work Julian, I really wasn't sure what the criteria were for this yet so I'm really pleased to see you've taken it on.

Just a few comments and questions as outlined:

::: Koha/Account/Line.pm
@@ +771,5 @@
> +sub store {
> +    my ($self) = @_;
> +
> +    my $AutoCreditNumber = C4::Context->preference('AutoCreditNumber');
> +    if ($AutoCreditNumber && !$self->in_storage && $self->is_credit && !$self->credit_number) {

Hmm, I slightly wonder if it's a good idea to allow passing credit_number and allowing it to override the AudoCreditNumber setting.. to me if it's set then we should throw an Exception if someone is trying to pass in their own?

Not a failing case, but wanted to raise as a question.

The second question here.. should this apply to ALL credit types, or just a subset of those that actually deal with taking money as apposed to writeoffs and other types of giving credit (like LOST_ITEM_RETURN)?

@@ +773,5 @@
> +
> +    my $AutoCreditNumber = C4::Context->preference('AutoCreditNumber');
> +    if ($AutoCreditNumber && !$self->in_storage && $self->is_credit && !$self->credit_number) {
> +        my $rs = Koha::Database->new->schema->resultset($self->_type);
> +

It feels like this whole block should be inside a transaction.. you're doing a lookup followed by a store using calculated data.. what if another db connection adds a payment line at the same time?
Comment 13 Martin Renvoize 2020-02-28 19:06:27 UTC
Note to self.. I'd like to add support for cash_register_id + timstamp as an option for numbering.. I'll post that as a followup once we've worked through my other comments ;)
Comment 14 Julian Maurice 2020-03-03 09:59:10 UTC
> Hmm, I slightly wonder if it's a good idea to allow passing
> credit_number and allowing it to override the AudoCreditNumber
> setting.. to me if it's set then we should throw an Exception if
> someone is trying to pass in their own?

Since credit_number can only be automatically generated, it does not make sense to check if it's defined or not, so I can remove this check and always autogenerate a value.
I can also throw an exception.
To me, both solutions are acceptable.

I'll let you pick the one you prefer ;)

> The second question here.. should this apply to ALL credit types, or
> just a subset of those that actually deal with taking money as apposed
> to writeoffs and other types of giving credit (like LOST_ITEM_RETURN)?

I don't know, but the answer is probably... it depends :)
Should I add a "Auto generate credit number" parameter in credit types ?
Comment 15 Martin Renvoize 2020-03-03 10:10:28 UTC
(In reply to Julian Maurice from comment #14)
> > Hmm, I slightly wonder if it's a good idea to allow passing
> > credit_number and allowing it to override the AudoCreditNumber
> > setting.. to me if it's set then we should throw an Exception if
> > someone is trying to pass in their own?
> 
> Since credit_number can only be automatically generated, it does not make
> sense to check if it's defined or not, so I can remove this check and always
> autogenerate a value.
> I can also throw an exception.
> To me, both solutions are acceptable.
> 
> I'll let you pick the one you prefer ;)

I like throwing exceptions as it highlights errors when future devs make mistakes.. but at the same time they have a tiny performance impact by calling additional checks so I'm happy without if you'd rather just stop checking for it and add a note in the POD of the store method to highlight we will be ignoring it if passed. Your call, I'll accept either :)

> 
> > The second question here.. should this apply to ALL credit types, or
> > just a subset of those that actually deal with taking money as apposed
> > to write-offs and other types of giving credit (like LOST_ITEM_RETURN)?
> 
> I don't know, but the answer is probably... it depends :)
> Should I add a "Auto generate credit number" parameter in credit types ?

That's not a bad idea at all, and is probably reasonably trivial to add if you're happy to do so, and will keep everbody happy :)
Comment 16 Martin Renvoize 2020-03-23 12:00:54 UTC
Failing QA until the above is responded to.
Comment 17 Julian Maurice 2020-03-23 12:22:27 UTC
I will add the "Auto generate credit number" parameter, and the exception (probably better to have an error rather than a silent overwrite).
Changing status to ASSIGNED
Comment 18 Julian Maurice 2020-03-23 15:11:01 UTC
(In reply to Julian Maurice from comment #17)
> I will add the "Auto generate credit number" parameter

In fact, it's not that simple. "system" credit types cannot be modified :/
Should I add the possibility to edit them but only allow this parameter to be modifiable ? Or should I find another way (another syspref for example) ?
Comment 19 Julian Maurice 2020-03-24 12:06:40 UTC
Created attachment 101549 [details] [review]
Bug 19036: Add ability to auto generate a number for account credits

In some areas it's required to sequentially number payment slips /
receipts.

This patch adds a database column accountlines.credit_number and a
system preference AutoCreditNumber to control how this number will be
generated.  The following options are available:

- Do not automatically generate credit numbers.
  This is the current behaviour and the default syspref value.

- Automatically generate credit numbers in the form <year>-0001 (annual)

- Automatically generate credit numbers in the form
  <branchcode>yyyymm0001 (branchyyyymmincr)
  where <branchcode> is the branch where the user (staff member) is
  logged in

- Automatically generate credit numbers in the form 1, 2, 3
  (incremental)

It also adds a column (hidden by default) in the table under
Transactions tab to display this number.

Test plan:
0. Apply patch, run updatedatabase and update_dbix_class_files
1. Go to Admin » Column settings, and uncheck the 'hidden' box for
   column credit_number in table account-fines. It will be easier for
   testing
2. Create a manual credit for a borrower. Verify in Transactions tab
   that this credit has no number generated
3. Change syspref 'AutoCreditNumber' to 'incremental'
4. Create more manual credits, and verify that the numbers generated are
   1, 2, 3, ...
5. Change syspref 'AutoCreditNumber' to 'annual'
6. Create more manual credits, and verify that the numbers generated are
   '2020-0001', '2020-0002', ...
7. Change syspref to 'AutoCreditNumber' to 'branchyyyymmincr'
8. Create more manual credits, and verify that the numbers generated are
   'BRANCHA2020020001', 'BRANCHA2020020002', ... (assuming you are
   connected to library BRANCHA, and it's February 2020)
9. Set library to another one, say BRANCHB
10. Create more manual credits, and verify that the numbers generated are
    'BRANCHB2020020001', 'BRANCHB2020020002', ...
11. Edit the letter ACCOUNT_CREDIT, and add [% account.credit_number %]
    somewhere. Go back to Transactions tab and click on 'Print' for one
    line that has a credit number. Make sure the number is there.
12. prove t/db_dependent/Koha/Account.t

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 20 Julian Maurice 2020-03-24 12:06:45 UTC
Created attachment 101550 [details] [review]
Bug 19036: Add ability to enable credit number for only some credit types

This feature is disabled by default for all credit types. To enable it
you have to set the syspref AutoCreditNumber to the value of your choice
and then enable the feature for each credit type you want.
System credit types can be modified too (but only this particular field
can be modified)

Also, throw an exception when the feature is enabled and a value for
credit_number has already been set

Updated test plan:
Test plan:
0. Apply patch, run updatedatabase and update_dbix_class_files
1. Go to Admin » Column settings, and uncheck the 'hidden' box for
   column credit_number in table account-fines. It will be easier for
   testing
2. Create a manual credit for a borrower. Verify in Transactions tab
   that this credit has no number generated
3. In Admin » Credit types:
  a. edit the FORGIVEN type and enable credit number generation
  b. create a new type A, check "can be manually added" and "enable
     credit number"
  c. create a new type B, check "can be manually added". Do NOT enable
     credit number
4. Change syspref 'AutoCreditNumber' to 'incremental'
5. Create more manual credits with types CREDIT and B, and verify that
   the numbers are not generated
6. Create more manual credits with types FORGIVEN and A, and verify that
   the numbers generated are 1, 2, 3, ...
7. Change syspref 'AutoCreditNumber' to 'annual'
8. Create more manual credits with types CREDIT and B, and verify that
   the numbers are not generated
9. Create more manual credits with types FORGIVEN and A, and verify that
   the numbers generated are '2020-0001', '2020-0002', ...
10. Change syspref to 'AutoCreditNumber' to 'branchyyyymmincr'
11. Create more manual credits with types CREDIT and B, and verify that
    the numbers are not generated
12. Create more manual credits with types FORGIVEN and A, and verify
    that the numbers generated are 'BRANCHA2020020001',
    'BRANCHA2020020002', ... (assuming you are connected to library
    BRANCHA, and it's February 2020)
13. Set library to another one, say BRANCHB
14. Create more manual credits with types FORGIVEN and A, and verify
    that the numbers generated are 'BRANCHB2020020001',
    'BRANCHB2020020002', ...
15. Edit the letter ACCOUNT_CREDIT, and add [% account.credit_number %]
    somewhere. Go back to Transactions tab and click on 'Print' for one
    line that has a credit number. Make sure the number is there.
16. prove t/db_dependent/Koha/Account.t
Comment 21 Martin Renvoize 2020-03-26 17:03:22 UTC
Created attachment 101896 [details] [review]
Bug 19036: Add ability to auto generate a number for account credits

In some areas it's required to sequentially number payment slips /
receipts.

This patch adds a database column accountlines.credit_number and a
system preference AutoCreditNumber to control how this number will be
generated.  The following options are available:

- Do not automatically generate credit numbers.
  This is the current behaviour and the default syspref value.

- Automatically generate credit numbers in the form <year>-0001 (annual)

- Automatically generate credit numbers in the form
  <branchcode>yyyymm0001 (branchyyyymmincr)
  where <branchcode> is the branch where the user (staff member) is
  logged in

- Automatically generate credit numbers in the form 1, 2, 3
  (incremental)

It also adds a column (hidden by default) in the table under
Transactions tab to display this number.

Test plan:
0. Apply patch, run updatedatabase and update_dbix_class_files
1. Go to Admin » Column settings, and uncheck the 'hidden' box for
   column credit_number in table account-fines. It will be easier for
   testing
2. Create a manual credit for a borrower. Verify in Transactions tab
   that this credit has no number generated
3. Change syspref 'AutoCreditNumber' to 'incremental'
4. Create more manual credits, and verify that the numbers generated are
   1, 2, 3, ...
5. Change syspref 'AutoCreditNumber' to 'annual'
6. Create more manual credits, and verify that the numbers generated are
   '2020-0001', '2020-0002', ...
7. Change syspref to 'AutoCreditNumber' to 'branchyyyymmincr'
8. Create more manual credits, and verify that the numbers generated are
   'BRANCHA2020020001', 'BRANCHA2020020002', ... (assuming you are
   connected to library BRANCHA, and it's February 2020)
9. Set library to another one, say BRANCHB
10. Create more manual credits, and verify that the numbers generated are
    'BRANCHB2020020001', 'BRANCHB2020020002', ...
11. Edit the letter ACCOUNT_CREDIT, and add [% account.credit_number %]
    somewhere. Go back to Transactions tab and click on 'Print' for one
    line that has a credit number. Make sure the number is there.
12. prove t/db_dependent/Koha/Account.t

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2020-03-26 17:03:26 UTC
Created attachment 101897 [details] [review]
Bug 19036: Add ability to enable credit number for only some credit types

This feature is disabled by default for all credit types. To enable it
you have to set the syspref AutoCreditNumber to the value of your choice
and then enable the feature for each credit type you want.
System credit types can be modified too (but only this particular field
can be modified)

Also, throw an exception when the feature is enabled and a value for
credit_number has already been set

Updated test plan:
Test plan:
0. Apply patch, run updatedatabase and update_dbix_class_files
1. Go to Admin » Column settings, and uncheck the 'hidden' box for
   column credit_number in table account-fines. It will be easier for
   testing
2. Create a manual credit for a borrower. Verify in Transactions tab
   that this credit has no number generated
3. In Admin » Credit types:
  a. edit the FORGIVEN type and enable credit number generation
  b. create a new type A, check "can be manually added" and "enable
     credit number"
  c. create a new type B, check "can be manually added". Do NOT enable
     credit number
4. Change syspref 'AutoCreditNumber' to 'incremental'
5. Create more manual credits with types CREDIT and B, and verify that
   the numbers are not generated
6. Create more manual credits with types FORGIVEN and A, and verify that
   the numbers generated are 1, 2, 3, ...
7. Change syspref 'AutoCreditNumber' to 'annual'
8. Create more manual credits with types CREDIT and B, and verify that
   the numbers are not generated
9. Create more manual credits with types FORGIVEN and A, and verify that
   the numbers generated are '2020-0001', '2020-0002', ...
10. Change syspref to 'AutoCreditNumber' to 'branchyyyymmincr'
11. Create more manual credits with types CREDIT and B, and verify that
    the numbers are not generated
12. Create more manual credits with types FORGIVEN and A, and verify
    that the numbers generated are 'BRANCHA2020020001',
    'BRANCHA2020020002', ... (assuming you are connected to library
    BRANCHA, and it's February 2020)
13. Set library to another one, say BRANCHB
14. Create more manual credits with types FORGIVEN and A, and verify
    that the numbers generated are 'BRANCHB2020020001',
    'BRANCHB2020020002', ...
15. Edit the letter ACCOUNT_CREDIT, and add [% account.credit_number %]
    somewhere. Go back to Transactions tab and click on 'Print' for one
    line that has a credit number. Make sure the number is there.
16. prove t/db_dependent/Koha/Account.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2020-03-26 17:03:47 UTC
This works really nicely, thanks Julian!

Signing off
Comment 24 Katrin Fischer 2020-06-22 23:15:37 UTC
There are some failures in QA tools that need to be taken care of. I will try to come back to this for more testing after they are fixed:

 FAIL	t/db_dependent/Koha/Account.t
   FAIL	  forbidden patterns
		forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 1068)

 FAIL	installer/data/mysql/kohastructure.sql
   FAIL	  tinyint_has_boolean_flag
		The new column account_credit_types.credit_number_enabled is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/AccountCreditType.pm), see the SQL12 coding guideline

 FAIL	Koha/Account/Line.pm
   FAIL	  forbidden patterns
		forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 895)
		forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 910)
Comment 25 Martin Renvoize 2020-06-25 09:44:56 UTC
Created attachment 106278 [details] [review]
Bug 19036: Add ability to auto generate a number for account credits

In some areas it's required to sequentially number payment slips /
receipts.

This patch adds a database column accountlines.credit_number and a
system preference AutoCreditNumber to control how this number will be
generated.  The following options are available:

- Do not automatically generate credit numbers.
  This is the current behaviour and the default syspref value.

- Automatically generate credit numbers in the form <year>-0001 (annual)

- Automatically generate credit numbers in the form
  <branchcode>yyyymm0001 (branchyyyymmincr)
  where <branchcode> is the branch where the user (staff member) is
  logged in

- Automatically generate credit numbers in the form 1, 2, 3
  (incremental)

It also adds a column (hidden by default) in the table under
Transactions tab to display this number.

Test plan:
0. Apply patch, run updatedatabase and update_dbix_class_files
1. Go to Admin » Column settings, and uncheck the 'hidden' box for
   column credit_number in table account-fines. It will be easier for
   testing
2. Create a manual credit for a borrower. Verify in Transactions tab
   that this credit has no number generated
3. Change syspref 'AutoCreditNumber' to 'incremental'
4. Create more manual credits, and verify that the numbers generated are
   1, 2, 3, ...
5. Change syspref 'AutoCreditNumber' to 'annual'
6. Create more manual credits, and verify that the numbers generated are
   '2020-0001', '2020-0002', ...
7. Change syspref to 'AutoCreditNumber' to 'branchyyyymmincr'
8. Create more manual credits, and verify that the numbers generated are
   'BRANCHA2020020001', 'BRANCHA2020020002', ... (assuming you are
   connected to library BRANCHA, and it's February 2020)
9. Set library to another one, say BRANCHB
10. Create more manual credits, and verify that the numbers generated are
    'BRANCHB2020020001', 'BRANCHB2020020002', ...
11. Edit the letter ACCOUNT_CREDIT, and add [% account.credit_number %]
    somewhere. Go back to Transactions tab and click on 'Print' for one
    line that has a credit number. Make sure the number is there.
12. prove t/db_dependent/Koha/Account.t

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2020-06-25 09:45:00 UTC
Created attachment 106279 [details] [review]
Bug 19036: Add ability to enable credit number for only some credit types

This feature is disabled by default for all credit types. To enable it
you have to set the syspref AutoCreditNumber to the value of your choice
and then enable the feature for each credit type you want.
System credit types can be modified too (but only this particular field
can be modified)

Also, throw an exception when the feature is enabled and a value for
credit_number has already been set

Updated test plan:
Test plan:
0. Apply patch, run updatedatabase and update_dbix_class_files
1. Go to Admin » Column settings, and uncheck the 'hidden' box for
   column credit_number in table account-fines. It will be easier for
   testing
2. Create a manual credit for a borrower. Verify in Transactions tab
   that this credit has no number generated
3. In Admin » Credit types:
  a. edit the FORGIVEN type and enable credit number generation
  b. create a new type A, check "can be manually added" and "enable
     credit number"
  c. create a new type B, check "can be manually added". Do NOT enable
     credit number
4. Change syspref 'AutoCreditNumber' to 'incremental'
5. Create more manual credits with types CREDIT and B, and verify that
   the numbers are not generated
6. Create more manual credits with types FORGIVEN and A, and verify that
   the numbers generated are 1, 2, 3, ...
7. Change syspref 'AutoCreditNumber' to 'annual'
8. Create more manual credits with types CREDIT and B, and verify that
   the numbers are not generated
9. Create more manual credits with types FORGIVEN and A, and verify that
   the numbers generated are '2020-0001', '2020-0002', ...
10. Change syspref to 'AutoCreditNumber' to 'branchyyyymmincr'
11. Create more manual credits with types CREDIT and B, and verify that
    the numbers are not generated
12. Create more manual credits with types FORGIVEN and A, and verify
    that the numbers generated are 'BRANCHA2020020001',
    'BRANCHA2020020002', ... (assuming you are connected to library
    BRANCHA, and it's February 2020)
13. Set library to another one, say BRANCHB
14. Create more manual credits with types FORGIVEN and A, and verify
    that the numbers generated are 'BRANCHB2020020001',
    'BRANCHB2020020002', ...
15. Edit the letter ACCOUNT_CREDIT, and add [% account.credit_number %]
    somewhere. Go back to Transactions tab and click on 'Print' for one
    line that has a credit number. Make sure the number is there.
16. prove t/db_dependent/Koha/Account.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2020-06-25 09:45:03 UTC
Created attachment 106280 [details] [review]
Bug 19036: (QA follow-up) Use Koha::DateUtils

This patch replaced raw DateTime->now calls with dt_from_string from
Koha::DateUtils
Comment 28 Martin Renvoize 2020-06-25 09:45:07 UTC
Created attachment 106281 [details] [review]
Bug 19036: DBIC Schema Updates

Dumped schema updates + manual addition of boolean flags
Comment 29 Martin Renvoize 2020-06-25 09:45:46 UTC
Patchset rescued.. I've added the required QA followups and rebased on master :)
Comment 30 Katrin Fischer 2020-07-04 23:48:50 UTC
The test file this also changes is currently failing for me on master:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/Account.t
t/db_dependent/Koha/Account.t .. 1/12 Use of uninitialized value in numeric lt (<) at /home/vagrant/kohaclone/Koha/Account/Line.pm line 745.
t/db_dependent/Koha/Account.t .. 3/12 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`accountlines`, CONSTRAINT `accountlines_ibfk_debit_type` FOREIGN KEY (`debit_type_code`) REFERENCES `account_debit_types` (`code`) ON UPDATE CASCADE) [for Statement "INSERT INTO `accountlines` ( `amount`, `amountoutstanding`, `borrowernumber`, `branchcode`, `date`, `debit_type_code`, `description`, `interface`, `issue_id`, `itemnumber`, `manager_id`, `note`, `payment_type`, `timestamp`) VALUES ( ?, ?, ?, ?, NOW(), ?, ?, ?, ?, ?, ?, ?, ?, current_timestamp )" with ParamValues: 0=5, 1=5, 2=199, 3='dgj1I5', 4='failure', 5='type validation failure', 6='commandline', 7=undef, 8=undef, 9=199, 10='this should fail anyway', 11=undef] at /home/vagrant/kohaclone/Koha/Object.pm line 166
t/db_dependent/Koha/Account.t .. 13/12 # Looks like you planned 12 tests but ran 13.
t/db_dependent/Koha/Account.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
All 12 subtests passed 

Test Summary Report
-------------------
t/db_dependent/Koha/Account.t (Wstat: 65280 Tests: 13 Failed: 1)
  Failed test:  13
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 12 tests but ran 13.
Files=1, Tests=13,  8 wallclock secs ( 0.05 usr  0.02 sys +  6.85 cusr  0.92 csys =  7.84 CPU)
Result: FAIL

Moving this to BLOCKED until this is resolved to then continue here.
Comment 31 Martin Renvoize 2020-07-10 10:33:07 UTC
Created attachment 106767 [details] [review]
Bug 19036: (QA follow-up) Fix test plan
Comment 32 Martin Renvoize 2020-07-10 10:35:53 UTC
(In reply to Katrin Fischer from comment #30)
> The test file this also changes is currently failing for me on master:
> 
> kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove
> t/db_dependent/Koha/Account.t
> t/db_dependent/Koha/Account.t .. 1/12 Use of uninitialized value in numeric
> lt (<) at /home/vagrant/kohaclone/Koha/Account/Line.pm line 745.
> t/db_dependent/Koha/Account.t .. 3/12
> DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st
> execute failed: Cannot add or update a child row: a foreign key constraint
> fails (`koha_kohadev`.`accountlines`, CONSTRAINT
> `accountlines_ibfk_debit_type` FOREIGN KEY (`debit_type_code`) REFERENCES
> `account_debit_types` (`code`) ON UPDATE CASCADE) [for Statement "INSERT
> INTO `accountlines` ( `amount`, `amountoutstanding`, `borrowernumber`,
> `branchcode`, `date`, `debit_type_code`, `description`, `interface`,
> `issue_id`, `itemnumber`, `manager_id`, `note`, `payment_type`, `timestamp`)
> VALUES ( ?, ?, ?, ?, NOW(), ?, ?, ?, ?, ?, ?, ?, ?, current_timestamp )"
> with ParamValues: 0=5, 1=5, 2=199, 3='dgj1I5', 4='failure', 5='type
> validation failure', 6='commandline', 7=undef, 8=undef, 9=199, 10='this
> should fail anyway', 11=undef] at /home/vagrant/kohaclone/Koha/Object.pm
> line 166
> t/db_dependent/Koha/Account.t .. 13/12 # Looks like you planned 12 tests but
> ran 13.
> t/db_dependent/Koha/Account.t .. Dubious, test returned 255 (wstat 65280,
> 0xff00)
> All 12 subtests passed 
> 
> Test Summary Report
> -------------------
> t/db_dependent/Koha/Account.t (Wstat: 65280 Tests: 13 Failed: 1)
>   Failed test:  13
>   Non-zero exit status: 255
>   Parse errors: Bad plan.  You planned 12 tests but ran 13.
> Files=1, Tests=13,  8 wallclock secs ( 0.05 usr  0.02 sys +  6.85 cusr  0.92
> csys =  7.84 CPU)
> Result: FAIL
> 
> Moving this to BLOCKED until this is resolved to then continue here.

I think you missed the DB update... The test passes both on master and with this bug for me (though the test plan was out by one, which I've corrected with a follow-up).
Comment 33 Katrin Fischer 2020-07-30 22:41:31 UTC
Hm, it wasn't the missed db update, because as I wrote... I have the same output of master - but it claims all tests successful. Are we provoking the db error intentionally?

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/Account.t
t/db_dependent/Koha/Account.t .. 3/12 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`accountlines`, CONSTRAINT `accountlines_ibfk_debit_type` FOREIGN KEY (`debit_type_code`) REFERENCES `account_debit_types` (`code`) ON UPDATE CASCADE) [for Statement "INSERT INTO `accountlines` ( `amount`, `amountoutstanding`, `borrowernumber`, `branchcode`, `date`, `debit_type_code`, `description`, `interface`, `issue_id`, `itemnumber`, `manager_id`, `note`, `payment_type`, `timestamp`) VALUES ( ?, ?, ?, ?, NOW(), ?, ?, ?, ?, ?, ?, ?, ?, current_timestamp )" with ParamValues: 0=5, 1=5, 2=93, 3='Ro3LQ1AIV', 4='failure', 5='type validation failure', 6='commandline', 7=undef, 8=undef, 9=93, 10='this should fail anyway', 11=undef] at /home/vagrant/kohaclone/Koha/Object.pm line 166
t/db_dependent/Koha/Account.t .. ok     
All tests successful.
Comment 34 Katrin Fischer 2020-07-30 23:05:06 UTC
1) Transactions table

http://localhost:8081/cgi-bin/koha/members/boraccount.pl?borrowernumber=51
Tab: Transactions

The last table row with the total is off by one (display issue).

2) Terminology: credit number versus receipt number versus ?

I wonder if "credit number" is the right term. 
I think for debits we'd probably use invoice number... and for debits I am thinking of receipt number. 

What do the native speakers say?


Everything else works as expected as far as I can tell - I like!

1) should be an easy fix, 2 is a bit more difficult. CC'ing some other librarians in hope for help :)
Comment 35 David Nind 2020-07-31 06:05:33 UTC
My thoughts on terminology, not having worked through the test plan (from an accounting perspective):

- A debit transaction: invoice number for charges and purchases
- A credit transaction: receipt number for payments, credit note for return of goods

For the purposes of having an auto incremented number in a sequence for auditing, then perhaps calling it a transaction number may be better.

This could be used for different debit and credit types. 

On statements or transaction records, having some way to use the preferred term depending on your country, accounting rules or preferences for the transaction type may be useful.