Bug 19036

*** Bug 20309 has been marked as a duplicate of this bug. ***

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.

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).

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.

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.

(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

Hi Julian,
it's clear. I'll try to test it again soon.
Thank You.

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?

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 ;)

> 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 ?

(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 :)

Failing QA until the above is responded to.

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

(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) ?

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>

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

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>

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>

This works really nicely, thanks Julian!

Signing off

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)

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>

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>

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

Created attachment 106281 [details] [review]
Bug 19036: DBIC Schema Updates

Dumped schema updates + manual addition of boolean flags

Patchset rescued.. I've added the required QA followups and rebased on master :)

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.

Created attachment 106767 [details] [review]
Bug 19036: (QA follow-up) Fix test plan

(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).

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.

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 :)

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.

(In reply to Katrin Fischer from comment #33)
> 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.

That is NOT a failure.. it's an expected case.. we are testing for the DB constraint.. there's isn't an easy way to hide this noise and it's already there on lots of tests.

Created attachment 107872 [details] [review]
Bug 19036: (QA follow-up) Fix 'out-by-one' error on table

This patch fixes the total row where the column count was out by one.

Fixed the table issue in the attached followup.

As for terminology.. this identifier is only added to 'credits' to the 'debits' terminology discussion it mute at this time.  As for the terminology of 'credit number'.. I think it's fine at the DB level as is.. I'm less sure about how it displays in the UI.  Currently, we're only allowing one 'style' of incrementing number for a set of credit types.. I could imagine there being two distinct sets of running numbers.. one for 'payment reciepts' and a second for 'credit notes', but we don't allow for that here. 

As such, I'd be tempted to go much more generic and just call the column 'Identifier' or 'Unique identifier' or something like that in the UI?

Should we be hiding the 'credit number' column by default in fact... we don't enable the feature by default so why expose the empty field in the table?

I also think the credit number should be added to the 'register details' page that lists transactions too.

(In reply to Martin Renvoize from comment #36)
> (In reply to Katrin Fischer from comment #33)
> > 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.
> 
> That is NOT a failure.. it's an expected case.. we are testing for the DB
> constraint.. there's isn't an easy way to hide this noise and it's already
> there on lots of tests.

I will deal with that on bug 26157.

Created attachment 107875 [details] [review]
Bug 19036: (QA follow-up) Add credit_number handling to POS

This patch adds the display of 'credit_number' as a replacement for the
internal 'accountlines_id' as the transaction identifier on the
register summary page when using cash registers.

(In reply to Jonathan Druart from comment #41)
> (In reply to Martin Renvoize from comment #36)
> > (In reply to Katrin Fischer from comment #33)
> > > 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.
> > 
> > That is NOT a failure.. it's an expected case.. we are testing for the DB
> > constraint.. there's isn't an easy way to hide this noise and it's already
> > there on lots of tests.
> 
> I will deal with that on bug 26157.

Awesome!  Thanks Jonathan

Created attachment 107877 [details] [review]
Bug 19036: (QA follow-up) Add credit_number handling to POS

This patch adds the display of 'credit_number' as a replacement for the
internal 'accountlines_id' as the transaction identifier on the
register summary page when using cash registers.

Test plan
1/ Enable 'UseCashRegisters'
2/ Enable 'EnablePointOfSale'
3/ Enable at least one 'Debit type' to allow 'sale'
4/ Add at least one 'Cash register'
5/ Enable 'AutoCreditNumber'
6/ 'Enable automatic generation of credit number' for the 'Purchase'
credit type
7/ Pay for an item via the Point of Sale page
8/ Navigate to the 'register' page to display the purchase details
9/ Confirm that the 'Transaction' is listed with a number of the form
specified in the 'AutoCreditNumber' preference
10/ Refund an item from this screen
11/ Note that the 'accountlines_id' is used for this refund as we did
not specify the 'Refund' credit type to use the 'AutoCreditNumber'
system
12/ Signoff

Created attachment 108323 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 108324 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 108325 [details] [review]
Bug 19036: (QA follow-up) Use Koha::DateUtils

This patch replaced raw DateTime->now calls with dt_from_string from
Koha::DateUtils

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 108326 [details] [review]
Bug 19036: DBIC Schema Updates

Dumped schema updates + manual addition of boolean flags

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 108327 [details] [review]
Bug 19036: (QA follow-up) Fix test plan

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 108328 [details] [review]
Bug 19036: (QA follow-up) Fix 'out-by-one' error on table

This patch fixes the total row where the column count was out by one.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 108329 [details] [review]
Bug 19036: (QA follow-up) Add credit_number handling to POS

This patch adds the display of 'credit_number' as a replacement for the
internal 'accountlines_id' as the transaction identifier on the
register summary page when using cash registers.

Test plan
1/ Enable 'UseCashRegisters'
2/ Enable 'EnablePointOfSale'
3/ Enable at least one 'Debit type' to allow 'sale'
4/ Add at least one 'Cash register'
5/ Enable 'AutoCreditNumber'
6/ 'Enable automatic generation of credit number' for the 'Purchase'
credit type
7/ Pay for an item via the Point of Sale page
8/ Navigate to the 'register' page to display the purchase details
9/ Confirm that the 'Transaction' is listed with a number of the form
specified in the 'AutoCreditNumber' preference
10/ Refund an item from this screen
11/ Note that the 'accountlines_id' is used for this refund as we did
not specify the 'Refund' credit type to use the 'AutoCreditNumber'
system
12/ Signoff

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

I a little torn still about the terminology, but not enough to block this. I have retested the table layout and the added POS functionality.

Created attachment 108525 [details] [review]
Bug 19036: Add missing test

I've added a missing test but it does not pass. Could you have a look?

Created attachment 108587 [details] [review]
Bug 19036: (follow-up) Improve test output

This patch adds proper detail to the test output for the newly
introduced AutoCreditNumber feature and fixes the final test by adding
the 'interface' and 'amount' parameters to the Koha::Account::Line being
created so that we reach the Exception we are trying to catch and don't
die early.

Fixed the test.. there were two minor issues with the construction meaning that we would not reach the exception (missing interface and amount params in the call to new).

I've also added some more verbose output to the prior tests to clarify their intent at runtime.

Pushed to master for 20.11, thanks to everybody involved!

Created attachment 108733 [details] [review]
Bug 19036: (follow-up) Test credits should be credits

A series of accountlines added to mock credits were being added with a
debit_type_code instead of a valid credit_type_code and so were being
mis-identified by the new credit numbering code trigger in the store
routine.

This patch updates the test to correctly identify the credits as credits

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

The attached followup should fix the related failing test on master.

Follow-up pushed to master!

enhancement will not be backported to 20.05.x

enhancement will not be backported to 20.05.x