Bug 21756

Summary: Deprecate C4::Accounts::manualinvoice (use Koha::Account->add_debit instead)
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: josef.moravec, katrin.fischer, martin.renvoize, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21721    
Bug Blocks: 21683    
Attachments: Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21747: Update members/manivoice to use add_debit
Bug 21747: Add deprecation warning to manualinvoice
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21747: Update members/manivoice to use add_debit
Bug 21747: Add deprecation warning to manualinvoice
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Update members/manivoice to use add_debit
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Update members/manivoice to use add_debit
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Update members/manivoice to use add_debit
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'
Bug 21756: (QA follow-up) Fixes for rebase
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets
Bug 21756: (QA follow-up) Fix Circulation.t
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Update members/manivoice to use add_debit
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'
Bug 21756: (QA follow-up) Fixes for rebase
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets
Bug 21756: (QA follow-up) Fix Circulation.t
Bug 21756: (follow-up) Add test for deprecation warning
Bug 21756: (follow-up) Replace newly introduced manualinvoice call
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets
Bug 21756: (QA follow-up) Fix Circulation.t
Bug 21756: (follow-up) Add test for deprecation warning
Bug 21756: (follow-up) Replace newly introduced manualinvoice call
Bug 21756: (QA follow-up) Add `Account Management Fee` mapping
Bug 21756: (QA follow-up) Add offset_types to database
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets
Bug 21756: (QA follow-up) Fix Circulation.t
Bug 21756: (follow-up) Add test for deprecation warning
Bug 21756: (follow-up) Replace newly introduced manualinvoice call
Bug 21756: (QA follow-up) Add `Account Fee` mapping
Bug 21756: (QA follow-up) Add offset_types to database
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets
Bug 21756: (QA follow-up) Fix Circulation.t
Bug 21756: (follow-up) Add test for deprecation warning
Bug 21756: (follow-up) Replace newly introduced manualinvoice call
Bug 21756: (QA follow-up) Add `Account Fee` mapping
Bug 21756: (QA follow-up) Add offset_types to database
Bug 21756: (QA follow-up) Add missing offset_types to installer file
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets
Bug 21756: (QA follow-up) Fix Circulation.t
Bug 21756: (follow-up) Add test for deprecation warning
Bug 21756: (follow-up) Replace newly introduced manualinvoice call
Bug 21756: (QA follow-up) Add `Account Fee` mapping
Bug 21756: (QA follow-up) Add offset_types to database
Bug 21756: (QA follow-up) Add offest types to installer file
Bug 21756: Remove use of manualinvoice from Koha::Patron
Bug 21756: Remove use of manualinvoice from Koha::Hold
Bug 21756: Replace manualinvoice with add_debit in tests
Bug 21756: Add deprecation warning to manualinvoice
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets
Bug 21756: (QA follow-up) Fix Circulation.t
Bug 21756: (follow-up) Add test for deprecation warning
Bug 21756: (follow-up) Replace newly introduced manualinvoice call
Bug 21756: (QA follow-up) Add `Account Fee` mapping
Bug 21756: (QA follow-up) Add offset_types to database
Bug 21756: (QA follow-up) Add offest types to installer file

Description Martin Renvoize 2018-11-02 12:23:22 UTC
All account handling should go via the Koha::Account class.
Comment 1 Martin Renvoize 2018-12-17 08:58:21 UTC
Created attachment 83257 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 2 Martin Renvoize 2018-12-17 08:58:24 UTC
Created attachment 83258 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 3 Martin Renvoize 2018-12-17 08:58:27 UTC
Created attachment 83259 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 4 Martin Renvoize 2018-12-17 08:58:30 UTC
Created attachment 83260 [details] [review]
Bug 21747: Update members/manivoice to use add_debit
Comment 5 Martin Renvoize 2018-12-17 08:58:33 UTC
Created attachment 83261 [details] [review]
Bug 21747: Add deprecation warning to manualinvoice
Comment 6 Martin Renvoize 2018-12-17 09:00:01 UTC
Created attachment 83262 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 7 Martin Renvoize 2018-12-17 09:00:06 UTC
Created attachment 83263 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 8 Martin Renvoize 2018-12-17 09:00:10 UTC
Created attachment 83264 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 9 Martin Renvoize 2018-12-17 09:00:14 UTC
Created attachment 83265 [details] [review]
Bug 21747: Update members/manivoice to use add_debit
Comment 10 Martin Renvoize 2018-12-17 09:00:17 UTC
Created attachment 83266 [details] [review]
Bug 21747: Add deprecation warning to manualinvoice
Comment 11 Martin Renvoize 2018-12-17 09:02:19 UTC
Created attachment 83267 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 12 Martin Renvoize 2018-12-17 09:02:22 UTC
Created attachment 83268 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 13 Martin Renvoize 2018-12-17 09:02:25 UTC
Created attachment 83269 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 14 Martin Renvoize 2018-12-17 09:02:29 UTC
Created attachment 83270 [details] [review]
Bug 21756: Update members/manivoice to use add_debit
Comment 15 Martin Renvoize 2018-12-17 09:02:32 UTC
Created attachment 83271 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice
Comment 16 Martin Renvoize 2018-12-17 09:04:53 UTC
Test Plan

1) Apply patches and run the test suit to check for regressions in tests
2) Use the manual invoice page to add a new invoice, this should work
3) As above, but use a non-existent item number and check that you get a suitable warning still.
Comment 17 Josef Moravec 2019-01-22 07:22:34 UTC
Need rebase due to bug 21915.

Also qa tools have some complains:

 FAIL	Koha/Patron.pm
   FAIL	  pod
		*** ERROR: Spurious text after =cut  in file Koha/Patron.pm

 FAIL	members/maninvoice.pl
   FAIL	  valid
			(Missing operator before $error?)
		Global symbol "$template" requires explicit package name (did you forget to declare "my $template"?) 
		Scalar found where operator expected 
		Can't redeclare "my" in "my" 
		members/maninvoice.pl has too many errors.
		Global symbol "$cookie" requires explicit package name (did you forget to declare "my $cookie"?) 
		Global symbol "$loggedinuser" requires explicit package name (did you forget to declare "my $loggedinuser"?) 
		Global symbol "$error" requires explicit package name (did you forget to declare "my $error"?) 
		"my" variable $input masks earlier declaration in same scope 
		Type of arg 1 to Try::Tiny::catch must be block or sub {} (not reference constructor) 
		syntax error
Comment 18 Martin Renvoize 2019-02-04 16:18:18 UTC
Created attachment 84729 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 19 Martin Renvoize 2019-02-04 16:18:21 UTC
Created attachment 84730 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 20 Martin Renvoize 2019-02-04 16:18:24 UTC
Created attachment 84731 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 21 Martin Renvoize 2019-02-04 16:18:27 UTC
Created attachment 84732 [details] [review]
Bug 21756: Update members/manivoice to use add_debit
Comment 22 Martin Renvoize 2019-02-04 16:18:30 UTC
Created attachment 84733 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice
Comment 23 Martin Renvoize 2019-02-04 16:19:12 UTC
Rebased on master
Comment 24 Josef Moravec 2019-02-18 07:04:20 UTC
I am still getting this fron QA tools:

 FAIL	members/maninvoice.pl
   FAIL	  valid
			(Missing operator before $error?)
		Global symbol "$template" requires explicit package name (did you forget to declare "my $template"?) 
		Scalar found where operator expected 
		Can't redeclare "my" in "my" 
		members/maninvoice.pl has too many errors.
		Global symbol "$cookie" requires explicit package name (did you forget to declare "my $cookie"?) 
		Global symbol "$loggedinuser" requires explicit package name (did you forget to declare "my $loggedinuser"?) 
		Global symbol "$error" requires explicit package name (did you forget to declare "my $error"?) 
		"my" variable $input masks earlier declaration in same scope 
		Type of arg 1 to Try::Tiny::catch must be block or sub {} (not reference constructor) 
		syntax error

Also test don't pass:
t/db_dependent/Circulation/NoIssuesChargeGuarantees.t .. 1/6 
#   Failed test 'Got correct offset type'
#   at t/db_dependent/Circulation/NoIssuesChargeGuarantees.t line 81.
#          got: 'Lost Item'
#     expected: 'Manual Debit'

#   Failed test 'No tests run for subtest "auto_too_much_oweing | OPACFineNoRenewalsBlockAutoRenew"'
#   at t/db_dependent/Circulation.t line 703.
Can't use string ("FU") as a subroutine ref while "strict refs" in use at t/db_dependent/Circulation.t line 668.
Comment 25 Martin Renvoize 2019-02-20 11:29:39 UTC
Created attachment 85320 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 26 Martin Renvoize 2019-02-20 11:29:42 UTC
Created attachment 85321 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 27 Martin Renvoize 2019-02-20 11:29:45 UTC
Created attachment 85322 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 28 Martin Renvoize 2019-02-20 11:29:48 UTC
Created attachment 85323 [details] [review]
Bug 21756: Update members/manivoice to use add_debit
Comment 29 Martin Renvoize 2019-02-20 11:29:51 UTC
Created attachment 85324 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice
Comment 30 Martin Renvoize 2019-02-20 11:29:54 UTC
Created attachment 85325 [details] [review]
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2019-02-20 11:29:57 UTC
Created attachment 85326 [details] [review]
Bug 21756: (QA follow-up) Fixes for rebase

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2019-02-20 11:30:00 UTC
Created attachment 85327 [details] [review]
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2019-02-20 11:31:27 UTC
Sorted the issues with a series of follow-ups and I ran the whole test suit against it all again to check everything passed.  Thanks for the QA effort Josef, back to you :)
Comment 34 Tomás Cohen Arazi 2019-02-20 12:31:15 UTC
\o/
Comment 35 Josef Moravec 2019-02-20 13:19:41 UTC
Created attachment 85350 [details] [review]
Bug 21756: (QA follow-up) Fix Circulation.t
Comment 36 Josef Moravec 2019-02-20 15:35:24 UTC
I am sorry, but the tests are still not passing:

t/db_dependent/Koha/Patrons.t .. 10/33 DBD::mysql::st execute failed: Field 'type' doesn't have a default value [for Statement "INSERT INTO `account_offsets` ( `amount`, `debit_id`) VALUES ( ?, ? )" with ParamValues: 0=5.000000, 1='68'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
    # No tests run!

#   Failed test 'No tests run for subtest "add_enrolment_fee_if_needed"'
#   at t/db_dependent/Koha/Patrons.t line 473.

t/db_dependent/Koha/Holds.t .. DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t` FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` ( `amount`, `debit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=42, 1='69', 2='Hold Expired'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
        # Looks like you planned 4 tests but ran 3.

    #   Failed test 'charge_cancel_fee parameter'
    #   at t/db_dependent/Koha/Holds.t line 134.


I think that some new values should be added to account_offset_types...
Comment 37 Katrin Fischer 2019-02-20 19:05:31 UTC
(In reply to Josef Moravec from comment #36)
> I am sorry, but the tests are still not passing:
> 
> t/db_dependent/Koha/Patrons.t .. 10/33 DBD::mysql::st execute failed: Field
> 'type' doesn't have a default value [for Statement "INSERT INTO
> `account_offsets` ( `amount`, `debit_id`) VALUES ( ?, ? )" with ParamValues:
> 0=5.000000, 1='68'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
>     # No tests run!
> 
> #   Failed test 'No tests run for subtest "add_enrolment_fee_if_needed"'
> #   at t/db_dependent/Koha/Patrons.t line 473.
> 
> t/db_dependent/Koha/Holds.t .. DBD::mysql::st execute failed: Cannot add or
> update a child row: a foreign key constraint fails
> (`koha_kohadev`.`account_offsets`, CONSTRAINT `account_offsets_ibfk_t`
> FOREIGN KEY (`type`) REFERENCES `account_offset_types` (`type`) ON DELETE
> CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `account_offsets` (
> `amount`, `debit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=42,
> 1='69', 2='Hold Expired'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line
> 1832.
>         # Looks like you planned 4 tests but ran 3.
> 
>     #   Failed test 'charge_cancel_fee parameter'
>     #   at t/db_dependent/Koha/Holds.t line 134.
> 
> 
> I think that some new values should be added to account_offset_types...

The tests pass for me - I wonder what the difference could be? Just applied, reset_all and ran them. The only issue I see is lots of warnings in Circulation.t, but I tihnk there was another bug for those.
Comment 38 Martin Renvoize 2019-02-20 19:43:40 UTC
I'm going to take another look at this again in the morning with fresh eyes. The final follow-up fix you've added Josef highlights a logical error to me... Feels like accounttype should not be getting set after the add_debit call at all.. seems like it's cheating the test result.  Anywho, I'll investigate.. bet it's another thing that crept in with a rebase.
Comment 39 Martin Renvoize 2019-02-21 08:11:27 UTC
Having looked again this morning, I'm OK with the accounttype being altered in the test there.. it mimics the behaviour of the FU -> F transition that happens in C4::Overdues.

So.. I'm re-running the full test suit one last time in a totally fresh koha-testing-docker to see if I can replicate your failures.. I'll report back once the test run has completed.
Comment 40 Martin Renvoize 2019-02-21 08:15:40 UTC
Ack.. thought I'd double check I'd caught all counts of manualinvoice and it seems a few new ones have been introduced since I wrote this patchset.. bear with my whilst I do yet another followup!
Comment 41 Martin Renvoize 2019-02-21 08:49:48 UTC
Created attachment 85420 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 42 Martin Renvoize 2019-02-21 08:49:51 UTC
Created attachment 85421 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 43 Martin Renvoize 2019-02-21 08:49:55 UTC
Created attachment 85422 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 44 Martin Renvoize 2019-02-21 08:49:58 UTC
Created attachment 85423 [details] [review]
Bug 21756: Update members/manivoice to use add_debit
Comment 45 Martin Renvoize 2019-02-21 08:50:01 UTC
Created attachment 85424 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice
Comment 46 Martin Renvoize 2019-02-21 08:50:05 UTC
Created attachment 85425 [details] [review]
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2019-02-21 08:50:09 UTC
Created attachment 85426 [details] [review]
Bug 21756: (QA follow-up) Fixes for rebase

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2019-02-21 08:50:12 UTC
Created attachment 85427 [details] [review]
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2019-02-21 08:50:15 UTC
Created attachment 85428 [details] [review]
Bug 21756: (QA follow-up) Fix Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2019-02-21 08:50:19 UTC
Created attachment 85429 [details] [review]
Bug 21756: (follow-up) Add test for deprecation warning

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2019-02-21 08:50:22 UTC
Created attachment 85430 [details] [review]
Bug 21756: (follow-up) Replace newly introduced manualinvoice call

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize 2019-02-21 08:53:30 UTC
Yup, I can't replicate that test failure at all.. sorry Josef.. any chance you could have another go or help me replicate it.
Comment 53 Martin Renvoize 2019-02-22 11:08:43 UTC
Created attachment 85505 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 54 Martin Renvoize 2019-02-22 11:08:46 UTC
Created attachment 85506 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 55 Martin Renvoize 2019-02-22 11:08:50 UTC
Created attachment 85507 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 56 Martin Renvoize 2019-02-22 11:08:53 UTC
Created attachment 85508 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice
Comment 57 Martin Renvoize 2019-02-22 11:08:56 UTC
Created attachment 85509 [details] [review]
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Martin Renvoize 2019-02-22 11:08:59 UTC
Created attachment 85510 [details] [review]
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 59 Martin Renvoize 2019-02-22 11:09:03 UTC
Created attachment 85511 [details] [review]
Bug 21756: (QA follow-up) Fix Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 60 Martin Renvoize 2019-02-22 11:09:07 UTC
Created attachment 85512 [details] [review]
Bug 21756: (follow-up) Add test for deprecation warning

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 61 Martin Renvoize 2019-02-22 11:09:10 UTC
Created attachment 85513 [details] [review]
Bug 21756: (follow-up) Replace newly introduced manualinvoice call

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2019-02-22 11:09:14 UTC
Created attachment 85514 [details] [review]
Bug 21756: (QA follow-up) Add `Account Management Fee` mapping

Required for add_enrolement_fee_if_needed within Koha::Patron

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2019-02-22 11:09:17 UTC
Created attachment 85515 [details] [review]
Bug 21756: (QA follow-up) Add offset_types to database

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 64 Josef Moravec 2019-02-25 11:56:49 UTC
Sorry, still not perfect...

t/db_dependent/Koha/Patrons.t .......................... 12/33 DBD::mysql::st execute failed: Data too long for column 'type' at row 1 [for Statement "INSERT INTO `account_offsets` ( `amount`, `debit_id`, `type`) VALUES ( ?, ?, ? )" with ParamValues: 0=5.000000, 1='408', 2='Account Management Fee'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
    # No tests run!

#   Failed test 'No tests run for subtest "add_enrolment_fee_if_needed"'
#   at t/db_dependent/Koha/Patrons.t line 473.
DBIx::Class::Storage::DBI::_dbh_execute(): Data too long for column 'type' at row 1 at /home/vagrant/kohaclone/Koha/Object.pm line 155
# Looks like your test exited with 255 just after 14.

'type' is of type VARCHAR (16), so 'Account Management Fee' is too long here

also one type here:
 FAIL	t/db_dependent/Accounts.t
   FAIL	  spelling
		 recieved  ==> received
Comment 65 Martin Renvoize 2019-02-25 18:22:16 UTC
Created attachment 85638 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 66 Martin Renvoize 2019-02-25 18:22:20 UTC
Created attachment 85639 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 67 Martin Renvoize 2019-02-25 18:22:23 UTC
Created attachment 85640 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 68 Martin Renvoize 2019-02-25 18:22:27 UTC
Created attachment 85641 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice
Comment 69 Martin Renvoize 2019-02-25 18:22:30 UTC
Created attachment 85642 [details] [review]
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 70 Martin Renvoize 2019-02-25 18:22:34 UTC
Created attachment 85643 [details] [review]
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2019-02-25 18:22:37 UTC
Created attachment 85644 [details] [review]
Bug 21756: (QA follow-up) Fix Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 72 Martin Renvoize 2019-02-25 18:22:41 UTC
Created attachment 85645 [details] [review]
Bug 21756: (follow-up) Add test for deprecation warning

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 73 Martin Renvoize 2019-02-25 18:22:44 UTC
Created attachment 85646 [details] [review]
Bug 21756: (follow-up) Replace newly introduced manualinvoice call

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 74 Martin Renvoize 2019-02-25 18:22:48 UTC
Created attachment 85647 [details] [review]
Bug 21756: (QA follow-up) Add `Account Fee` mapping

Required for add_enrolement_fee_if_needed within Koha::Patron

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 75 Martin Renvoize 2019-02-25 18:22:51 UTC
Created attachment 85648 [details] [review]
Bug 21756: (QA follow-up) Add offset_types to database

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2019-02-25 18:24:47 UTC
Dopey mistake corrected.
Comment 77 Josef Moravec 2019-02-25 18:45:01 UTC
Created attachment 85654 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 78 Josef Moravec 2019-02-25 18:45:05 UTC
Created attachment 85655 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 79 Josef Moravec 2019-02-25 18:45:09 UTC
Created attachment 85656 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 80 Josef Moravec 2019-02-25 18:45:13 UTC
Created attachment 85657 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice

https://bugs.koha-community.org/show_bug.cgi?id=21756

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 81 Josef Moravec 2019-02-25 18:45:17 UTC
Created attachment 85658 [details] [review]
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 82 Josef Moravec 2019-02-25 18:45:21 UTC
Created attachment 85659 [details] [review]
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 83 Josef Moravec 2019-02-25 18:45:25 UTC
Created attachment 85660 [details] [review]
Bug 21756: (QA follow-up) Fix Circulation.t

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 84 Josef Moravec 2019-02-25 18:45:29 UTC
Created attachment 85661 [details] [review]
Bug 21756: (follow-up) Add test for deprecation warning

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 85 Josef Moravec 2019-02-25 18:45:33 UTC
Created attachment 85662 [details] [review]
Bug 21756: (follow-up) Replace newly introduced manualinvoice call

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 86 Josef Moravec 2019-02-25 18:45:36 UTC
Created attachment 85663 [details] [review]
Bug 21756: (QA follow-up) Add `Account Fee` mapping

Required for add_enrolement_fee_if_needed within Koha::Patron

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 87 Josef Moravec 2019-02-25 18:45:41 UTC
Created attachment 85664 [details] [review]
Bug 21756: (QA follow-up) Add offset_types to database

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 88 Josef Moravec 2019-02-25 18:45:45 UTC
Created attachment 85665 [details] [review]
Bug 21756: (QA follow-up) Add missing offset_types to installer file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 89 Martin Renvoize 2019-03-07 14:06:55 UTC
Created attachment 86265 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.
Comment 90 Martin Renvoize 2019-03-07 14:06:58 UTC
Created attachment 86266 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.
Comment 91 Martin Renvoize 2019-03-07 14:07:02 UTC
Created attachment 86267 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests
Comment 92 Martin Renvoize 2019-03-07 14:07:06 UTC
Created attachment 86268 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice
Comment 93 Martin Renvoize 2019-03-07 14:07:09 UTC
Created attachment 86269 [details] [review]
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 94 Martin Renvoize 2019-03-07 14:07:13 UTC
Created attachment 86270 [details] [review]
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 95 Martin Renvoize 2019-03-07 14:07:17 UTC
Created attachment 86271 [details] [review]
Bug 21756: (QA follow-up) Fix Circulation.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 96 Martin Renvoize 2019-03-07 14:07:20 UTC
Created attachment 86272 [details] [review]
Bug 21756: (follow-up) Add test for deprecation warning

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 97 Martin Renvoize 2019-03-07 14:07:24 UTC
Created attachment 86273 [details] [review]
Bug 21756: (follow-up) Replace newly introduced manualinvoice call

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 98 Martin Renvoize 2019-03-07 14:07:28 UTC
Created attachment 86274 [details] [review]
Bug 21756: (QA follow-up) Add `Account Fee` mapping

Required for add_enrolement_fee_if_needed within Koha::Patron

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 99 Martin Renvoize 2019-03-07 14:07:31 UTC
Created attachment 86275 [details] [review]
Bug 21756: (QA follow-up) Add offset_types to database

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 100 Martin Renvoize 2019-03-07 14:07:35 UTC
Created attachment 86276 [details] [review]
Bug 21756: (QA follow-up) Add offest types to installer file

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 101 Martin Renvoize 2019-03-07 14:08:54 UTC
Freshly rebased
Comment 102 Nick Clemens 2019-03-07 20:58:43 UTC
Looks like we lost the sign off lines on the latest patches. I also only see Josef, did these skip SO and go straight to QA?
Comment 103 Martin Renvoize 2019-03-07 21:09:09 UTC
Ooh, I thought Tomas was doing a final QA as both Josef and I worked on it and in effect signed off each others work.
Comment 104 Nick Clemens 2019-03-07 21:25:59 UTC
Setting to SO for final QA pass
Comment 105 Kyle M Hall 2019-03-11 13:32:33 UTC
Created attachment 86441 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Patron

Known Side Effect: Prior to this patch enrolment fees were not
recorded in the FinesLog. After this patch, if the FinesLog is
enabled then the 'action' will be recorded as `create_account`.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 106 Kyle M Hall 2019-03-11 13:32:53 UTC
Created attachment 86442 [details] [review]
Bug 21756: Remove use of manualinvoice from Koha::Hold

Known Side Effect: Prior to this patch hold cancellation fees
were not recorded in the FinesLog. After this patch, if the
FinesLog is enabled then the 'action' will be recorded as
`create_hold_expiration`.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 107 Kyle M Hall 2019-03-11 13:32:56 UTC
Created attachment 86443 [details] [review]
Bug 21756: Replace manualinvoice with add_debit in tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 108 Kyle M Hall 2019-03-11 13:33:00 UTC
Created attachment 86444 [details] [review]
Bug 21756: Add deprecation warning to manualinvoice

https://bugs.koha-community.org/show_bug.cgi?id=21756

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 109 Kyle M Hall 2019-03-11 13:33:03 UTC
Created attachment 86445 [details] [review]
Bug 21756: (QA follow-up) Fix for 'Spurious text after =cut'

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 110 Kyle M Hall 2019-03-11 13:33:07 UTC
Created attachment 86446 [details] [review]
Bug 21756: (QA follow-up) Correct the test for 'Lost Item' offsets

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 111 Kyle M Hall 2019-03-11 13:33:10 UTC
Created attachment 86447 [details] [review]
Bug 21756: (QA follow-up) Fix Circulation.t

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 112 Kyle M Hall 2019-03-11 13:33:14 UTC
Created attachment 86448 [details] [review]
Bug 21756: (follow-up) Add test for deprecation warning

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 113 Kyle M Hall 2019-03-11 13:33:17 UTC
Created attachment 86449 [details] [review]
Bug 21756: (follow-up) Replace newly introduced manualinvoice call

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 114 Kyle M Hall 2019-03-11 13:33:20 UTC
Created attachment 86450 [details] [review]
Bug 21756: (QA follow-up) Add `Account Fee` mapping

Required for add_enrolement_fee_if_needed within Koha::Patron

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 115 Kyle M Hall 2019-03-11 13:33:24 UTC
Created attachment 86451 [details] [review]
Bug 21756: (QA follow-up) Add offset_types to database

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 116 Kyle M Hall 2019-03-11 13:33:27 UTC
Created attachment 86452 [details] [review]
Bug 21756: (QA follow-up) Add offest types to installer file

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 117 Nick Clemens 2019-03-16 13:54:36 UTC
Awesome work all!

Pushed to master for 19.05
Comment 118 Martin Renvoize 2019-03-19 11:06:12 UTC
Enhancement will not be backported to 18.11.x series.