Account debits types should be properly recorded in their own table with a purpose-built interface for working upon them. This would pave the way for extensions to the cash management side of Koha, allowing for more granular definitions of charges for a point of sale system and giving an obvious location to add features like recording tax rates on different charges.
What about translation?
I intend on including translation in the patch I'll be submitting here ;) The point is.. I have a number of ways I need to extend the account debit functionality to add a point of sale mechanism to Koha. This is simply foundational work for that as leaving these credits as authorized values preclude them from being extended in any meaningful way. It is my firm opinion that Authorized Values should not just be used as a catch-all.
Created attachment 93513 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Update database to insert existing values
Created attachment 93514 [details] [review] Bug 23049: Add branch limitations tables
Created attachment 93515 [details] [review] Bug 23049: Update DBIC Classes for branch limitations
Created attachment 93516 [details] [review] Bug 23049: Add CHECK constraint to accountlines
Created attachment 93518 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality.
Created attachment 93519 [details] [review] Bug 23049: Update maninvoice to reference debit types
Created attachment 93520 [details] [review] Bug 23049: Use debit_type in staff client views
Created attachment 93521 [details] [review] Bug 23049: Update existing accounttype values
Created attachment 93522 [details] [review] Bug 23049: Split system and local types Test plan 1) Apply patches up to this point 2) View the new debit types management page and comment whether the split table view is clearer/better.
Created attachment 93523 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t
I discussed translations with Katrin and she preferred I do a dependant bug rather than role it in here.. that's my next target so this patchset is ready to test.
Created attachment 93529 [details] [review] Bug 23049: Use a filtered table instead of two tables Test Plan * Apply this patch and comment on new page design.
Not a critical point, but column name "Account type code" is different in the creation/modification form where it's "Account debit type code". The "Account type code" is supposed to accept 16 letters, but blocks after 5. If I use letters, currency symbols or comma for the "Default amount", the data is set to 0.00 after saving : I think it could be better if we could have a control field (don't know if it the correct term) that only accept numbers and the autorized decimal separator. Other behaviors seemed good to me, the design is clear and the options (filter / search) efficient !
(In reply to Séverine Queune from comment #15) > Not a critical point, but column name "Account type code" is different in > the creation/modification form where it's "Account debit type code". > > The "Account type code" is supposed to accept 16 letters, but blocks after 5. Actually, it should be 80 based on bug 23539 > If I use letters, currency symbols or comma for the "Default amount", the > data is set to 0.00 after saving : I think it could be better if we could > have a control field (don't know if it the correct term) that only accept > numbers and the autorized decimal separator. Agreed, an html5 pattern like ^\d+\.\d{2}$ should do it!
Created attachment 93557 [details] [review] Bug 23049: (follow-up) Impliment feedback
Created attachment 93564 [details] [review] Bug 23049: (follow-up) Impliment feedback
Created attachment 93565 [details] [review] Bug 23049: (follow-up) Remove MANUAL_INV from AV
Thankyou for the fast feedback Séverine, I believe I've resolved those issues now with the followup :).
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93567 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Update database to insert existing values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93568 [details] [review] Bug 23049: Add branch limitations tables Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93569 [details] [review] Bug 23049: Update DBIC Classes for branch limitations Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93570 [details] [review] Bug 23049: Add CHECK constraint to accountlines Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93571 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93572 [details] [review] Bug 23049: Update maninvoice to reference debit types Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93573 [details] [review] Bug 23049: Use debit_type in staff client views Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93574 [details] [review] Bug 23049: Update existing accounttype values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93575 [details] [review] Bug 23049: Split system and local types Test plan 1) Apply patches up to this point 2) View the new debit types management page and comment whether the split table view is clearer/better. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93576 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93577 [details] [review] Bug 23049: Use a filtered table instead of two tables Test Plan * Apply this patch and comment on new page design. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93578 [details] [review] Bug 23049: (follow-up) Impliment feedback Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93579 [details] [review] Bug 23049: (follow-up) Remove MANUAL_INV from AV Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Just found myself a whole area of code I overlooked.. putting back to assigned to reflect I have a bit more work to do before we're ready for another round of testing.
Created attachment 93960 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add ac_debit_types_branches table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Migrate MANUAL_INV values * Remove MANUAL_INV references * Migrate accounttype values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93961 [details] [review] Bug 23049: Add CHECK constraint to accountlines Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93962 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93963 [details] [review] Bug 23049: Filter out system types by default Test Plan 1) Navigate to the debit types management page added in previous patches 2) Note that system types are now filtered out by default 3) Click on the new unfilter button to include system types 4) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93964 [details] [review] Bug 23049: Update maninvoice to reference debit types Test plan: 1) Naviage to a patron record and select the accounts > manual invoice tab 2) Add some manual invoices and confirm they are working as expected 3) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93965 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93966 [details] [review] Bug 23049: Update existing code to use debit_type * Update C4::Accounts::chargelostitem * Update C4::Accounts::manualinvoice * Update C4::Circulation::_FixOverduesOnReturn * Update C4::Circulation::_FixAccountForLostAndReturned * Update C4::Overdues::UpdateFine * Update C4::Overdues::GetFine * Update C4::Overdues::GetOverduesForBranch * Update Koha::Account->pay * Update Koha::Account->add_debit * Update Koha::Account->non_issues_charges * Update Koha::Account::Line->apply * Update Koha::Account::Line->adjust * Update controller scripts * Update reports scripts * Update tests Test Plan 1) Run the test suit and ensure everything still passes 2) Test reports/cash_register_stats still works 3) Test that adding manual invoices still works 4) Test that making payments still works 5) Test that lost item fee handling still works 6) Test that invoice printing still works 7) Test that the sco still works Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93967 [details] [review] Bug 23049: Update 'Res' to 'RESERVE' for consistency This patch updates all cases where debit_type_code was 'Res' to 'RESERVE' which is more informative and consistent with the new style guidelines. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93968 [details] [review] Bug 23049: Capitalise 'rent' calls to add_debit This works towards making type codes consistent Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93969 [details] [review] Bug 23049: Update 'PF' to 'PROCESSING' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93970 [details] [review] Bug 23049: Capitalise type passed to add_debit for account Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93972 [details] [review] Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency This patch updates all cases where debit_type_code was 'HE' to 'RESERVE_EXPIRED' which is more informative and consistent with the new style guidelines. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93973 [details] [review] Bug 23049: Capitalise type passed to add_debit for lost_item Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93974 [details] [review] Bug 23049: Remove manual_debit as it's never called Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93975 [details] [review] Bug 23049: Capitalise type passed to add_debit for overdue Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93976 [details] [review] Bug 23049: 'N' -> 'NEW_CARD' Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93977 [details] [review] Bug 23049: Drop type lookup as it's now a foreign key Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93978 [details] [review] Bug 23049: Update API Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93979 [details] [review] Bug 23049: (follow-up) Fix manual debits via add_debit Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
OK Trying to be brave, start QA
INSERT IGNORE INTO account_debit_types ( code, default_amount, description, can_be_added_manually, is_system ) SELECT SUBSTR(authorised_value, 1, 64), lib, authorised_value, 1, 0 This does not match somehow. Default amount filled by lib ?
NAME ac_debit_types_branches Names for databases, tables, columns, and indexes can be up to 64 characters long. Alias names can be up to 256 characters long. Why not account_debit_types_branches instead of some abbreviated form. Making developers look for a table a bit longer?
Moving authorized values to new table: SUBSTR(authorised_value, 1, 64) This may be a source of trouble, at least theoretically. Why not switch from the 64 to 80 chars? `accounttype` varchar(80) default NULL, `debit_type_code` varchar(64) default NULL, `payment_type` varchar(80) default NULL, -- optional authorised value PAYMENT_TYPE This looks a bit inconsistent too? Why not give the table an autoincrement id as PK and do not duplicate the code everywhere? Accountlines could be large, less space?
FAIL Koha/Account/DebitType.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage SKIP spelling FAIL valid Inconsistent hierarchy during C3 merge of class 'Koha::Account::DebitType': Koha::Account::DebitType, current merge results [ ] merging failed on 'Koha::Object' FAIL Koha/Account/DebitTypes.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'object_class' SKIP spelling OK valid Not sure what exactly triggered the first one.
Thanks for taking a look Marcel, I'll get onto the feedback today... all of it seems sane to me so far.
This is a quite fundamental one: UPDATE accountlines SET debit_type_code = accounttype, accounttype = NULL WHERE accounttype IN (SELECT code from account_debit_types) Hmm. I do not really like this at first glance (and second too ;). I would prefer one account type and not a separate debit code. If I would be scrolling through account lines, I think it is weird to have to look at two columns to determine the type of this transaction. But might understand this choice from the perspective to keep this patch set a bit smaller and narrow it down to the old MANUAL_INV stuff. So, my problem actually goes a bit beyond this report too and is also about the design of accounts. We should have added an accounts type table in the first place. Now we are doing it for a subset of our account types. And I am wondering if this step will bring us closer to that goal of integrating these types or will just hinder us along the way. I am inclined to think that we should not separate the two now. And I understand that this creates new problems. The account types would be partially controlled by the debit codes list. So we could not (yet) put a FK on it. But it might be a better starting point for controlling the other account types? Since this is an important design choice, I would suggest to discuss this on the dev list or within the QA team, or dev meeting? We need some more consensus on that decision.
(In reply to Marcel de Rooy from comment #58) > Moving authorized values to new table: > SUBSTR(authorised_value, 1, 64) > This may be a source of trouble, at least theoretically. Why not switch from > the 64 to 80 chars? > > `accounttype` varchar(80) default NULL, > `debit_type_code` varchar(64) default NULL, > `payment_type` varchar(80) default NULL, -- optional authorised value > PAYMENT_TYPE > This looks a bit inconsistent too? > > Why not give the table an autoincrement id as PK and do not duplicate the > code everywhere? Accountlines could be large, less space? I stuck to codes here to continue the pattern for simple reporting and also to allow for simple translations of the core system defaults where a code is significantly simpler to remember for translators. (Discussed with Katrin at hackfest regarding the translation comment, she was very pleased that we could keep the existing translations in the form of the include files).
(In reply to Marcel de Rooy from comment #59) > FAIL Koha/Account/DebitType.pm > OK critic > OK forbidden patterns > OK git manipulation > OK pod > OK pod coverage > SKIP spelling > FAIL valid > Inconsistent hierarchy during C3 merge of class > 'Koha::Account::DebitType': > Koha::Account::DebitType, > current merge results [ > ] > merging failed on 'Koha::Object' > > FAIL Koha/Account/DebitTypes.pm > OK critic > OK forbidden patterns > OK git manipulation > OK pod > FAIL pod coverage > POD is missing for 'object_class' > SKIP spelling > OK valid > > Not sure what exactly triggered the first one. The first one is the use of Koha::Objects::Limit::Library as a base class. Tomas has been looking into how to quiet this warning as we believe it's just noise at the moment.
(In reply to Marcel de Rooy from comment #61) > This is a quite fundamental one: > > UPDATE accountlines SET debit_type_code = accounttype, accounttype = NULL > WHERE accounttype IN (SELECT code from account_debit_types) > > Hmm. I do not really like this at first glance (and second too ;). I would > prefer one account type and not a separate debit code. If I would be > scrolling through account lines, I think it is weird to have to look at two > columns to determine the type of this transaction. Actually, as it currently stands you already have to look at two columns to ascertain whether the accountline is a debit or credit, so having these two fields next to each other is clearer in my opinion. I would like to rename accounttype to credit_type but didn't want to do that here and make the bug even bigger. > But might understand this choice from the perspective to keep this patch set > a bit smaller and narrow it down to the old MANUAL_INV stuff. Correct, I was trying to make this patch a bit more manageable. Bug 17702 became inpenetrably large by doing it all in one hit. > > So, my problem actually goes a bit beyond this report too and is also about > the design of accounts. We should have added an accounts type table in the > first place. Now we are doing it for a subset of our account types. And I am > wondering if this step will bring us closer to that goal of integrating > these types or will just hinder us along the way. I am inclined to think > that we should not separate the two now. > And I understand that this creates new problems. The account types would be > partially controlled by the debit codes list. So we could not (yet) put a FK > on it. But it might be a better starting point for controlling the other > account types? The aim was to have a similar treatment for credit_types in a second bug.. although presently I see no use case for user-definable credit_types at this time. I strongly believe one of the major strengths of this bug is that we're introducing better DB design including FK constraints. One cannot point a single field at multiple tables for such constraints at this time and I don't believe there is any plan for such support in SQL. > > Since this is an important design choice, I would suggest to discuss this on > the dev list or within the QA team, or dev meeting? We need some more > consensus on that decision. I'm happy to attempt something else if you can come up with any suggestions, but I don't really want this to go back to discussion again (this is just a reimplementation of bug 17702 which I believe has already been discussed). I've already saught support for this from those most closely in touch with accounts and thats how I arrived at this design (Tomas, Kyle, Josef).
Created attachment 93989 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add ac_debit_types_branches table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Migrate MANUAL_INV values * Remove MANUAL_INV references * Migrate accounttype values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93990 [details] [review] Bug 23049: Add CHECK constraint to accountlines Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93991 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93992 [details] [review] Bug 23049: Filter out system types by default Test Plan 1) Navigate to the debit types management page added in previous patches 2) Note that system types are now filtered out by default 3) Click on the new unfilter button to include system types 4) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93993 [details] [review] Bug 23049: Update maninvoice to reference debit types Test plan: 1) Naviage to a patron record and select the accounts > manual invoice tab 2) Add some manual invoices and confirm they are working as expected 3) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93994 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93995 [details] [review] Bug 23049: Update existing code to use debit_type * Update C4::Accounts::chargelostitem * Update C4::Accounts::manualinvoice * Update C4::Circulation::_FixOverduesOnReturn * Update C4::Circulation::_FixAccountForLostAndReturned * Update C4::Overdues::UpdateFine * Update C4::Overdues::GetFine * Update C4::Overdues::GetOverduesForBranch * Update Koha::Account->pay * Update Koha::Account->add_debit * Update Koha::Account->non_issues_charges * Update Koha::Account::Line->apply * Update Koha::Account::Line->adjust * Update controller scripts * Update reports scripts * Update tests Test Plan 1) Run the test suit and ensure everything still passes 2) Test reports/cash_register_stats still works 3) Test that adding manual invoices still works 4) Test that making payments still works 5) Test that lost item fee handling still works 6) Test that invoice printing still works 7) Test that the sco still works Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93996 [details] [review] Bug 23049: Update debit_type_code 'Res' to 'RESERVE' This patch updates all cases where debit_type_code was 'Res' to 'RESERVE' which is more informative and consistent with the new style guidelines. The patch also ensure all calls to add_debit with a type or 'reserve' now pass the capitalised code instead. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Capitalise reserve
Created attachment 93997 [details] [review] Bug 23049: Capitalise type passed to add_debit for `rent` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93998 [details] [review] Bug 23049: Update 'PF' to 'PROCESSING' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93999 [details] [review] Bug 23049: Capitalise type passed to add_debit for `account` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94000 [details] [review] Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency This patch updates all cases where debit_type_code was 'HE' to 'RESERVE_EXPIRED' which is more informative and consistent with the new style guidelines. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94001 [details] [review] Bug 23049: Capitalise type passed to add_debit for lost_item Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94002 [details] [review] Bug 23049: Remove manual_debit as it's never called Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94003 [details] [review] Bug 23049: Capitalise type passed to add_debit for `overdue` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94004 [details] [review] Bug 23049: Update 'N' to 'NEW_CARD' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94005 [details] [review] Bug 23049: Drop type lookup as it's now a foreign key Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94006 [details] [review] Bug 23049: Update API Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94007 [details] [review] Bug 23049: More thorough param checking
Created attachment 94008 [details] [review] Bug 23064: (follow-up) VARCHAR(80) + ac -> account
(In reply to Marcel de Rooy from comment #61) > This is a quite fundamental one: > > UPDATE accountlines SET debit_type_code = accounttype, accounttype = NULL > WHERE accounttype IN (SELECT code from account_debit_types) > > Hmm. I do not really like this at first glance (and second too ;). I would > prefer one account type and not a separate debit code. If I would be > scrolling through account lines, I think it is weird to have to look at two > columns to determine the type of this transaction. > But might understand this choice from the perspective to keep this patch set > a bit smaller and narrow it down to the old MANUAL_INV stuff. > > So, my problem actually goes a bit beyond this report too and is also about > the design of accounts. We should have added an accounts type table in the > first place. Now we are doing it for a subset of our account types. And I am > wondering if this step will bring us closer to that goal of integrating > these types or will just hinder us along the way. I am inclined to think > that we should not separate the two now. > And I understand that this creates new problems. The account types would be > partially controlled by the debit codes list. So we could not (yet) put a FK > on it. But it might be a better starting point for controlling the other > account types? > > Since this is an important design choice, I would suggest to discuss this on > the dev list or within the QA team, or dev meeting? We need some more > consensus on that decision. I agree that having one set of values pulled from a dedicated table and having the other set of values pulled from Authorised Values is a less than preferable situation. I *do* like having separate fields for the debit and credit type because A) it makes it crystal clear if an account line is a credit or a debit and B) it allows for FK constraints. I think the solution is to follow this bug quickly with a second bug that moves credit types into a dedicated table as well. As long as those come together quickly ( definitely within the same release ), I believe this will be a big step forward!
Created attachment 94073 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add ac_debit_types_branches table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Migrate MANUAL_INV values * Remove MANUAL_INV references * Migrate accounttype values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94074 [details] [review] Bug 23049: Add CHECK constraint to accountlines Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94075 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94076 [details] [review] Bug 23049: Filter out system types by default Test Plan 1) Navigate to the debit types management page added in previous patches 2) Note that system types are now filtered out by default 3) Click on the new unfilter button to include system types 4) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94077 [details] [review] Bug 23049: Update maninvoice to reference debit types Test plan: 1) Naviage to a patron record and select the accounts > manual invoice tab 2) Add some manual invoices and confirm they are working as expected 3) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94078 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94079 [details] [review] Bug 23049: Update existing code to use debit_type * Update C4::Accounts::chargelostitem * Update C4::Accounts::manualinvoice * Update C4::Circulation::_FixOverduesOnReturn * Update C4::Circulation::_FixAccountForLostAndReturned * Update C4::Overdues::UpdateFine * Update C4::Overdues::GetFine * Update C4::Overdues::GetOverduesForBranch * Update Koha::Account->pay * Update Koha::Account->add_debit * Update Koha::Account->non_issues_charges * Update Koha::Account::Line->apply * Update Koha::Account::Line->adjust * Update controller scripts * Update reports scripts * Update tests Test Plan 1) Run the test suit and ensure everything still passes 2) Test reports/cash_register_stats still works 3) Test that adding manual invoices still works 4) Test that making payments still works 5) Test that lost item fee handling still works 6) Test that invoice printing still works 7) Test that the sco still works Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94080 [details] [review] Bug 23049: Update debit_type_code 'Res' to 'RESERVE' This patch updates all cases where debit_type_code was 'Res' to 'RESERVE' which is more informative and consistent with the new style guidelines. The patch also ensure all calls to add_debit with a type or 'reserve' now pass the capitalised code instead. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Capitalise reserve
Created attachment 94081 [details] [review] Bug 23049: Capitalise type passed to add_debit for `rent` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94082 [details] [review] Bug 23049: Update 'PF' to 'PROCESSING' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94083 [details] [review] Bug 23049: Capitalise type passed to add_debit for `account` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94084 [details] [review] Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency This patch updates all cases where debit_type_code was 'HE' to 'RESERVE_EXPIRED' which is more informative and consistent with the new style guidelines. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94085 [details] [review] Bug 23049: Capitalise type passed to add_debit for lost_item Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94086 [details] [review] Bug 23049: Remove manual_debit as it's never called Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94087 [details] [review] Bug 23049: Capitalise type passed to add_debit for `overdue` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94088 [details] [review] Bug 23049: Update 'N' to 'NEW_CARD' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94089 [details] [review] Bug 23049: Drop type lookup as it's now a foreign key Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94090 [details] [review] Bug 23049: Update API Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94091 [details] [review] Bug 23049: More thorough param checking
Created attachment 94092 [details] [review] Bug 23049: (follow-up) VARCHAR(80) + ac -> account
Created attachment 94093 [details] [review] Bug 23049: (follow-up) Correction to includes
Created attachment 94111 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add ac_debit_types_branches table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Migrate MANUAL_INV values * Remove MANUAL_INV references * Migrate accounttype values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94112 [details] [review] Bug 23049: Add CHECK constraint to accountlines Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94113 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94114 [details] [review] Bug 23049: Filter out system types by default Test Plan 1) Navigate to the debit types management page added in previous patches 2) Note that system types are now filtered out by default 3) Click on the new unfilter button to include system types 4) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94115 [details] [review] Bug 23049: Update maninvoice to reference debit types Test plan: 1) Naviage to a patron record and select the accounts > manual invoice tab 2) Add some manual invoices and confirm they are working as expected 3) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94116 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94117 [details] [review] Bug 23049: Update existing code to use debit_type * Update C4::Accounts::chargelostitem * Update C4::Accounts::manualinvoice * Update C4::Circulation::_FixOverduesOnReturn * Update C4::Circulation::_FixAccountForLostAndReturned * Update C4::Overdues::UpdateFine * Update C4::Overdues::GetFine * Update C4::Overdues::GetOverduesForBranch * Update Koha::Account->pay * Update Koha::Account->add_debit * Update Koha::Account->non_issues_charges * Update Koha::Account::Line->apply * Update Koha::Account::Line->adjust * Update controller scripts * Update reports scripts * Update tests Test Plan 1) Run the test suit and ensure everything still passes 2) Test reports/cash_register_stats still works 3) Test that adding manual invoices still works 4) Test that making payments still works 5) Test that lost item fee handling still works 6) Test that invoice printing still works 7) Test that the sco still works Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94118 [details] [review] Bug 23049: Update debit_type_code 'Res' to 'RESERVE' This patch updates all cases where debit_type_code was 'Res' to 'RESERVE' which is more informative and consistent with the new style guidelines. The patch also ensure all calls to add_debit with a type or 'reserve' now pass the capitalised code instead. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Capitalise reserve
Created attachment 94119 [details] [review] Bug 23049: Capitalise type passed to add_debit for `rent` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94120 [details] [review] Bug 23049: Update 'PF' to 'PROCESSING' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94121 [details] [review] Bug 23049: Capitalise type passed to add_debit for `account` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94122 [details] [review] Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency This patch updates all cases where debit_type_code was 'HE' to 'RESERVE_EXPIRED' which is more informative and consistent with the new style guidelines. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94123 [details] [review] Bug 23049: Capitalise type passed to add_debit for lost_item Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94124 [details] [review] Bug 23049: Remove manual_debit as it's never called Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94125 [details] [review] Bug 23049: Capitalise type passed to add_debit for `overdue` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94126 [details] [review] Bug 23049: Update 'N' to 'NEW_CARD' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94127 [details] [review] Bug 23049: Drop type lookup as it's now a foreign key Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94128 [details] [review] Bug 23049: Update API Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94129 [details] [review] Bug 23049: More thorough param checking
Created attachment 94130 [details] [review] Bug 23049: (follow-up) VARCHAR(80) + ac -> account
Created attachment 94131 [details] [review] Bug 23049: (follow-up) Correction to includes
Created attachment 94132 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add ac_debit_types_branches table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Migrate MANUAL_INV values * Remove MANUAL_INV references * Migrate accounttype values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94133 [details] [review] Bug 23049: Add CHECK constraint to accountlines Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94134 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94135 [details] [review] Bug 23049: Filter out system types by default Test Plan 1) Navigate to the debit types management page added in previous patches 2) Note that system types are now filtered out by default 3) Click on the new unfilter button to include system types 4) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94136 [details] [review] Bug 23049: Update maninvoice to reference debit types Test plan: 1) Naviage to a patron record and select the accounts > manual invoice tab 2) Add some manual invoices and confirm they are working as expected 3) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94137 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94138 [details] [review] Bug 23049: Update existing code to use debit_type * Update C4::Accounts::chargelostitem * Update C4::Accounts::manualinvoice * Update C4::Circulation::_FixOverduesOnReturn * Update C4::Circulation::_FixAccountForLostAndReturned * Update C4::Overdues::UpdateFine * Update C4::Overdues::GetFine * Update C4::Overdues::GetOverduesForBranch * Update Koha::Account->pay * Update Koha::Account->add_debit * Update Koha::Account->non_issues_charges * Update Koha::Account::Line->apply * Update Koha::Account::Line->adjust * Update controller scripts * Update reports scripts * Update tests Test Plan 1) Run the test suit and ensure everything still passes 2) Test reports/cash_register_stats still works 3) Test that adding manual invoices still works 4) Test that making payments still works 5) Test that lost item fee handling still works 6) Test that invoice printing still works 7) Test that the sco still works Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94139 [details] [review] Bug 23049: Update debit_type_code 'Res' to 'RESERVE' This patch updates all cases where debit_type_code was 'Res' to 'RESERVE' which is more informative and consistent with the new style guidelines. The patch also ensure all calls to add_debit with a type or 'reserve' now pass the capitalised code instead. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Capitalise reserve
Created attachment 94140 [details] [review] Bug 23049: Capitalise type passed to add_debit for `rent` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94141 [details] [review] Bug 23049: Update 'PF' to 'PROCESSING' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94142 [details] [review] Bug 23049: Capitalise type passed to add_debit for `account` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94143 [details] [review] Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency This patch updates all cases where debit_type_code was 'HE' to 'RESERVE_EXPIRED' which is more informative and consistent with the new style guidelines. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94144 [details] [review] Bug 23049: Capitalise type passed to add_debit for lost_item Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94145 [details] [review] Bug 23049: Remove manual_debit as it's never called Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94146 [details] [review] Bug 23049: Capitalise type passed to add_debit for `overdue` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94147 [details] [review] Bug 23049: Update 'N' to 'NEW_CARD' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94148 [details] [review] Bug 23049: Drop type lookup as it's now a foreign key Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94149 [details] [review] Bug 23049: Update API Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94150 [details] [review] Bug 23049: More thorough param checking
Created attachment 94151 [details] [review] Bug 23049: (follow-up) VARCHAR(80) + ac -> account
Created attachment 94152 [details] [review] Bug 23049: (follow-up) Correction to includes
Created attachment 94165 [details] [review] Bug 23049: (follow-up) Handle unexpected types
DEV atomic update: bug_23049_debit.perl DBD::mysql::db do failed: Table 'koha_kohadev.account_credit_types' doesn't exist [for Statement " INSERT IGNORE INTO account_credit_types ( code, description, can_be_added_manually, default_amount, is_system ) SELECT SUBSTR(accounttype, 1, 80), "Unexpected type found during upgrade", 1, NULL, 0 FROM accountlines WHERE amount > 0 "] at (eval 1419) line 109. Upgrade to XXX done (Bug 23049 - Add account debit_types)
Created attachment 94307 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add ac_debit_types_branches table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Migrate MANUAL_INV values * Remove MANUAL_INV references * Migrate accounttype values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94308 [details] [review] Bug 23049: Add CHECK constraint to accountlines Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94309 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94310 [details] [review] Bug 23049: Filter out system types by default Test Plan 1) Navigate to the debit types management page added in previous patches 2) Note that system types are now filtered out by default 3) Click on the new unfilter button to include system types 4) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94311 [details] [review] Bug 23049: Update maninvoice to reference debit types Test plan: 1) Naviage to a patron record and select the accounts > manual invoice tab 2) Add some manual invoices and confirm they are working as expected 3) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94312 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94313 [details] [review] Bug 23049: Update existing code to use debit_type * Update C4::Accounts::chargelostitem * Update C4::Accounts::manualinvoice * Update C4::Circulation::_FixOverduesOnReturn * Update C4::Circulation::_FixAccountForLostAndReturned * Update C4::Overdues::UpdateFine * Update C4::Overdues::GetFine * Update C4::Overdues::GetOverduesForBranch * Update Koha::Account->pay * Update Koha::Account->add_debit * Update Koha::Account->non_issues_charges * Update Koha::Account::Line->apply * Update Koha::Account::Line->adjust * Update controller scripts * Update reports scripts * Update tests Test Plan 1) Run the test suit and ensure everything still passes 2) Test reports/cash_register_stats still works 3) Test that adding manual invoices still works 4) Test that making payments still works 5) Test that lost item fee handling still works 6) Test that invoice printing still works 7) Test that the sco still works Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94314 [details] [review] Bug 23049: Update debit_type_code 'Res' to 'RESERVE' This patch updates all cases where debit_type_code was 'Res' to 'RESERVE' which is more informative and consistent with the new style guidelines. The patch also ensure all calls to add_debit with a type or 'reserve' now pass the capitalised code instead. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Capitalise reserve
Created attachment 94315 [details] [review] Bug 23049: Capitalise type passed to add_debit for `rent` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94316 [details] [review] Bug 23049: Update 'PF' to 'PROCESSING' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94317 [details] [review] Bug 23049: Capitalise type passed to add_debit for `account` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94318 [details] [review] Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency This patch updates all cases where debit_type_code was 'HE' to 'RESERVE_EXPIRED' which is more informative and consistent with the new style guidelines. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94319 [details] [review] Bug 23049: Capitalise type passed to add_debit for lost_item Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94320 [details] [review] Bug 23049: Remove manual_debit as it's never called Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94321 [details] [review] Bug 23049: Capitalise type passed to add_debit for `overdue` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94323 [details] [review] Bug 23049: Update 'N' to 'NEW_CARD' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94324 [details] [review] Bug 23049: Drop type lookup as it's now a foreign key Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94325 [details] [review] Bug 23049: Update API Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94326 [details] [review] Bug 23049: More thorough param checking
Created attachment 94327 [details] [review] Bug 23049: (follow-up) VARCHAR(80) + ac -> account
Created attachment 94328 [details] [review] Bug 23049: (follow-up) Correction to includes
Created attachment 94329 [details] [review] Bug 23049: (follow-up) Handle unexpected types
Created attachment 94331 [details] [review] Bug 23049: Add debit_type * Add account_debit_types table * Add ac_debit_types_branches table * Add account_debit_types defaults * Add Koha::Account::DebitType and Koha::Account::DebitTypes * Prevent deletion of defaults * Migrate MANUAL_INV values * Remove MANUAL_INV references * Migrate accounttype values Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94332 [details] [review] Bug 23049: Add CHECK constraint to accountlines Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94333 [details] [review] Bug 23049: Account types configuration - Admin page Test plan: 1) Go to admin home, note there is new Debit types page in Accounting section 2) Go to any other admin page and confirm there is link to Debit types in the admin menu as well 3) Go to Debit types page 4) You should see a datatable listing existing debit types, ensure they are working as expected. 5) Try to create, edit and delete some debit types. Note: Some debit types cannot be deleted as they are needed for koha functionality. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94334 [details] [review] Bug 23049: Filter out system types by default Test Plan 1) Navigate to the debit types management page added in previous patches 2) Note that system types are now filtered out by default 3) Click on the new unfilter button to include system types 4) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94335 [details] [review] Bug 23049: Update maninvoice to reference debit types Test plan: 1) Naviage to a patron record and select the accounts > manual invoice tab 2) Add some manual invoices and confirm they are working as expected 3) Signoff Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94336 [details] [review] Bug 23049: Add tests Test plan: prove t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/Koha/Account/DebitTypes.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94337 [details] [review] Bug 23049: Update existing code to use debit_type * Update C4::Accounts::chargelostitem * Update C4::Accounts::manualinvoice * Update C4::Circulation::_FixOverduesOnReturn * Update C4::Circulation::_FixAccountForLostAndReturned * Update C4::Overdues::UpdateFine * Update C4::Overdues::GetFine * Update C4::Overdues::GetOverduesForBranch * Update Koha::Account->pay * Update Koha::Account->add_debit * Update Koha::Account->non_issues_charges * Update Koha::Account::Line->apply * Update Koha::Account::Line->adjust * Update controller scripts * Update reports scripts * Update tests Test Plan 1) Run the test suit and ensure everything still passes 2) Test reports/cash_register_stats still works 3) Test that adding manual invoices still works 4) Test that making payments still works 5) Test that lost item fee handling still works 6) Test that invoice printing still works 7) Test that the sco still works Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94338 [details] [review] Bug 23049: Update debit_type_code 'Res' to 'RESERVE' This patch updates all cases where debit_type_code was 'Res' to 'RESERVE' which is more informative and consistent with the new style guidelines. The patch also ensure all calls to add_debit with a type or 'reserve' now pass the capitalised code instead. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Capitalise reserve
Created attachment 94339 [details] [review] Bug 23049: Capitalise type passed to add_debit for `rent` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94340 [details] [review] Bug 23049: Update 'PF' to 'PROCESSING' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94341 [details] [review] Bug 23049: Capitalise type passed to add_debit for `account` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94342 [details] [review] Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency This patch updates all cases where debit_type_code was 'HE' to 'RESERVE_EXPIRED' which is more informative and consistent with the new style guidelines. Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94343 [details] [review] Bug 23049: Capitalise type passed to add_debit for lost_item Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94344 [details] [review] Bug 23049: Remove manual_debit as it's never called Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94345 [details] [review] Bug 23049: Capitalise type passed to add_debit for `overdue` Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94346 [details] [review] Bug 23049: Update 'N' to 'NEW_CARD' for consistency Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94347 [details] [review] Bug 23049: Drop type lookup as it's now a foreign key Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94348 [details] [review] Bug 23049: Update API Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 94349 [details] [review] Bug 23049: More thorough param checking
Created attachment 94350 [details] [review] Bug 23049: (follow-up) VARCHAR(80) + ac -> account
Created attachment 94351 [details] [review] Bug 23049: (follow-up) Correction to includes
Created attachment 94352 [details] [review] Bug 23049: (follow-up) Handle unexpected types
(In reply to Kyle M Hall from comment #150) > DEV atomic update: bug_23049_debit.perl > DBD::mysql::db do failed: Table 'koha_kohadev.account_credit_types' doesn't > exist [for Statement " > INSERT IGNORE INTO account_credit_types ( > code, > description, > can_be_added_manually, > default_amount, > is_system > ) > SELECT > SUBSTR(accounttype, 1, 80), > "Unexpected type found during upgrade", > 1, > NULL, > 0 > FROM > accountlines > WHERE > amount > 0 > "] at (eval 1419) line 109. > Upgrade to XXX done (Bug 23049 - Add account debit_types) Rebase mistake.. should be better now :)
I've pushed a public branch to keep this rebased rather than throw yet more patches in here: https://github.com/PTFS-Europe/koha/tree/bug_23049_inv
Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|m.de.rooy@rijksmuseum.nl |kyle@bywatersolutions.com Whats happening here ?
Still working on this one, Martin. Dont change QA contact without communication please, especially within such a short time frame.
Tested on top of commit 480434bbf4a32750c5e47a3600b6a386d9732296 with your branch. Thanks for your hard work. I have noted comments 64 and 85 (thx Kyle). I made my preference clear, and continued on the current stand. Comments below are in order of time noted, not priority. User experience: During startup of admin/debit_types.pl I am seeing the 15 records for a short moment and end up with 3 records (filtered). It would be nice to see them all and only allow editing of the non-system types ? Same when saving a record (no blocker, but does not look good). When you do not want to show the records, do not fetch them at all? I saw that I now have a debit type F (Unexpected type found during upgrade). Obviously this had to do with old fines stuff. I understand that bug 22521 should have dealt with those, but somehow I still had such a record in accountlines. I am wondering if this will happen to more people and if we should anticipate on that by giving this F another description than unexpected ;) $dbh->do( qq{ INSERT IGNORE INTO account_debit_types ( code, description, can_be_added_manually, default_amount, is_system ) SELECT SUBSTR(accounttype, 1, 80), "Unexpected type found during upgrade", 1, NULL, 0 FROM accountlines WHERE amount > 0 } ); No need for the SUBSTR anymore. Please add a DISTINCT accounttype to this query to eliminate a lof of ignored inserts. And a question: If this type is unexpected, why do we enable Add manually ? sub UpdateFine You touch the following line, obviously only changing the type codes. But this is a condition for finding the overdues. Why type M in the first place, and why not yet another debit code? debit_type_code => [ 'OVERDUE', 'M' ], This needs fixing somehow but I agree that it should be done on its own report.. sub GetFine + WHERE debit_type_code LIKE 'OVERDUE' Shouldnt that be = ? flexability [a.o. :) ] No, not so flexible. sub adjust - my $account_type = $self->accounttype; [..] + my $debit_type_code = $self->debit_type_code; As I understand, the only allowed code is now overdue so debit. But this change does not look very solid to me. Hopefully, we will not adjust too much ;) Copier Fees Database: | Copier Fees | Copier Fees | 1 | 0.250000 | 0 | -INSERT INTO `authorised_values` (category, authorised_value, lib) VALUES ('MANUAL_INV','Copier Fees','.25'); git grep 'Copier Fees' installer/data/mysql/es-ES/optional/auth_val.sql:INSERT INTO `authorised_values` (category, authorised_value, lib) VALUES ('MANUAL_INV','Copier Fees','.25'); Oops, you should remove that one above. Since we previously installed Copier Fees as a manual invoice, I think you should do now too. So add it too account_debit_types.sql. And Copier Fees should also have a new code. Now the description and code are the same. It is inconsistent with the other debit types. You also changed lots of other codes, or not? Does this hold for some other debit types too that were formerly hardcoded? Stuff like sundry etc. ? Which actually is a horrible category.. + [%- SWITCH account.debit_type_code -%] + [%- CASE 'ACCOUNT' -%]Account creation fee + [%- CASE 'ACCOUNT_RENEW' -%]Account renewal fee + [%- CASE 'LOST' -%]Lost item + [%- CASE 'M' -%]Sundry + [%- CASE 'NEW_CARD' -%]New card + [%- CASE 'OVERDUE' -%]Fine + [%- CASE 'PROCESSING' -%]Lost item processing fee + [%- CASE 'RENT' -%]Rental fee + [%- CASE 'RENT_DAILY' -%]Daily rental fee + [%- CASE 'RENT_RENEW' -%]Renewal of rental item + [%- CASE 'RENT_DAILY_RENEW' -%]Rewewal of daily rental item + [%- CASE 'RESERVE' -%]Hold fee + [%- CASE 'RESERVE_EXPIRED' -%]Hold waiting too long + [%- CASE -%][% account.debit_type.description | html %] + [%- END -%] Note that koha-tmpl/intranet-tmpl/prog/en/includes/accounts.inc contains debit code M for sundry while account_debit_types.sql install M as Manual fee ! This is a BLOCKER. Please correct. At the credit side, I am seeing code FORW (and FOR and W and WO !) and having doubts about it, leaving it outside the scope for now.. Submitting another comment for the failing tests.
[FAIL] t/db_dependent/Accounts.t [OK] t/db_dependent/Circulation.t [OK] t/db_dependent/Circulation/NoIssuesChargeGuarantees.t [OK] t/db_dependent/Circulation/Returns.t [OK] t/db_dependent/Circulation/issue.t [FAIL] t/db_dependent/Koha/Account.t [OK] t/db_dependent/Circulation/issue.t [OK] t/db_dependent/Koha/Account/DebitType.t [OK] t/db_dependent/Koha/Account/DebitTypes.t [FAIL] t/db_dependent/Koha/Account/Lines.t [OK] t/db_dependent/Koha/Patron.t [OK] t/db_dependent/Reserves.t [OK] t/db_dependent/SIP/Transaction.t [FAIL] t/db_dependent/api/v1/patrons_accounts.t t/db_dependent/api/v1/patrons_accounts.t .. DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_master`.`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`, `manager_id`) VALUES ( ?, ?, ?, ?, NOW(), ?, ?, ?, ? )" with ParamValues: 0=50, 1=50, 2=2930, 3='wfeFkt_9', 4='N', 5='A description', 6='test', 7=2930] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. => Looks like the description is copied to the debit code ? The other tests stumble over: DBIx::Class::Storage::DBI::_dbh_execute(): CONSTRAINT `accountlines_check_type` failed for `koha_master`.`accountlines` at /usr/share/koha/Koha/Object.pm line 156
I saw that I still had data that conflicted with accounttype IS NOT NULL OR debit_type_code IS NOT NULL; But when I removed that data and ran Accounts.t again, it still fails.
(In reply to Marcel de Rooy from comment #201) > I saw that I still had data that conflicted with accounttype IS NOT NULL OR > debit_type_code IS NOT NULL; > > But when I removed that data and ran Accounts.t again, it still fails. So, we should make sure that at upgrade time we no longer have data that does not meet the new constraint. And still something wrong in the test ?? I leave it here for now.
(In reply to Marcel de Rooy from comment #199) > Tested on top of commit 480434bbf4a32750c5e47a3600b6a386d9732296 with your > branch. > > Thanks for your hard work. > I have noted comments 64 and 85 (thx Kyle). I made my preference clear, and > continued on the current stand. > > Comments below are in order of time noted, not priority. > > User experience: > During startup of admin/debit_types.pl I am seeing the 15 records for a > short moment and end up with 3 records (filtered). I agree, I not a fan of the "flash of unstyled content" effect this has but as yet I've not managed to work around it.. I'll have another try. > It would be nice to see them all and only allow editing of the non-system > types ? This is actually how I had it originally but updated the approach after feedback from Nick and Severine. Perhaps just sorted the system and archived ones to the bottom would be the best approach.. though I liked them being interleaved so one could see why you could not create a new type with a code that matched a system debit type. > Same when saving a record (no blocker, but does not look good). When you do > not want to show the records, do not fetch them at all? > I saw that I now have a debit type F (Unexpected type found during upgrade). I will take a look at adding some additional handling into the db update to catch more cases of unrecognised types.. 'F' is obviously missed in certain cases in previous bugs and should be caught and converted to proper 'OVERDUE' types in my opinion. (I'm going to apply the patchset to a number of clones of customers from various versions and try to spot some patterns to see what other types may be being missed). > Obviously this had to do with old fines stuff. I understand that bug 22521 > should have dealt with those, but somehow I still had such a record in > accountlines. I am wondering if this will happen to more people and if we > should anticipate on that by giving this F another description than > unexpected ;) > > $dbh->do( > qq{ > INSERT IGNORE INTO account_debit_types ( > code, > description, > can_be_added_manually, > default_amount, > is_system > ) > SELECT > SUBSTR(accounttype, 1, 80), > "Unexpected type found during upgrade", > 1, > NULL, > 0 > FROM > accountlines > WHERE > amount > 0 > } > ); > No need for the SUBSTR anymore. Please add a DISTINCT accounttype to this > query to eliminate a lof of ignored inserts. Good catch, I'll update the update statement > And a question: If this type is unexpected, why do we enable Add manually ? Also a good catch, thanks :). > > sub UpdateFine > You touch the following line, obviously only changing the type codes. But > this is a condition for finding the overdues. Why type M in the first place, > and why not yet another debit code? > debit_type_code => [ 'OVERDUE', 'M' ], > This needs fixing somehow but I agree that it should be done on its own > report.. Totally agree, I've never been entirely sure why 'M' was in that list and have been slowly working towards removing it by clarifying it's use in bugs like this. > > sub GetFine > + WHERE debit_type_code LIKE 'OVERDUE' > Shouldnt that be = ? Also agree, I remember coming accross that one and thinking the same but forgetting to actually change it. Thanks :). (I think historically it comes from when fines were 'F' and 'FU' so it was a `LIKE "F%"`.) > > flexability [a.o. :) ] > No, not so flexible. > > sub adjust > - my $account_type = $self->accounttype; > [..] > + my $debit_type_code = $self->debit_type_code; > As I understand, the only allowed code is now overdue so debit. But this > change does not look very solid to me. Hopefully, we will not adjust too > much ;) Not sure I follow.. I'll double check the code but I believe it checks against 'OVERDUE' + 'UNRETURNED' (mix of type + status) > > Copier Fees > Database: | Copier Fees | Copier Fees | > 1 | 0.250000 | 0 | > -INSERT INTO `authorised_values` (category, authorised_value, lib) VALUES > ('MANUAL_INV','Copier Fees','.25'); > git grep 'Copier Fees' > installer/data/mysql/es-ES/optional/auth_val.sql:INSERT INTO > `authorised_values` (category, authorised_value, lib) VALUES > ('MANUAL_INV','Copier Fees','.25'); > Oops, you should remove that one above. > Since we previously installed Copier Fees as a manual invoice, I think you > should do now too. So add it too account_debit_types.sql. > And Copier Fees should also have a new code. Now the description and code > are the same. It is inconsistent with the other debit types. You also > changed lots of other codes, or not? I spotted that one this morning too, it certainly needs cleaning up here :). > Does this hold for some other debit types too that were formerly hardcoded? > Stuff like sundry etc. ? Which actually is a horrible category.. > + [%- SWITCH account.debit_type_code -%] > + [%- CASE 'ACCOUNT' -%]Account creation fee > + [%- CASE 'ACCOUNT_RENEW' -%]Account renewal fee > + [%- CASE 'LOST' -%]Lost item > + [%- CASE 'M' -%]Sundry > + [%- CASE 'NEW_CARD' -%]New card > + [%- CASE 'OVERDUE' -%]Fine > + [%- CASE 'PROCESSING' -%]Lost item processing fee > + [%- CASE 'RENT' -%]Rental fee > + [%- CASE 'RENT_DAILY' -%]Daily rental fee > + [%- CASE 'RENT_RENEW' -%]Renewal of rental item > + [%- CASE 'RENT_DAILY_RENEW' -%]Rewewal of daily rental item > + [%- CASE 'RESERVE' -%]Hold fee > + [%- CASE 'RESERVE_EXPIRED' -%]Hold waiting too long > + [%- CASE -%][% account.debit_type.description > | html %] > + [%- END -%] > Note that koha-tmpl/intranet-tmpl/prog/en/includes/accounts.inc contains > debit code M for sundry while account_debit_types.sql install M as Manual > fee ! > This is a BLOCKER. Please correct. Good catch :) > At the credit side, I am seeing code FORW (and FOR and W and WO !) and > having doubts about it, leaving it outside the scope for now.. I've submitted a bug for the conversion of credit type accounttypes now too which should take care of those.. Keeping them separate for now in an attempt to not drown QA in one superbug ;). > > Submitting another comment for the failing tests. Thanks :)
Rebased and pushed addition QA followups to the github branch. I'm unable to replicate your test failures at the moment and have tried with 6 different datasets... sorry but I'm going to struggle to fix that one without being able to replicate it :(.
(In reply to Martin Renvoize from comment #204) > Rebased and pushed addition QA followups to the github branch. > > I'm unable to replicate your test failures at the moment and have tried with > 6 different datasets... sorry but I'm going to struggle to fix that one > without being able to replicate it :(. OK Coming back to it soon
Revisiting
(Minor) If you archive a debit type, you should probably be no longer allowed to create a manual invoice with that type. (Minor) UPDATE accountlines SET accounttype = ? WHERE accounttype = ? => Hope you dont have too many clashes on the prefix here ;) The algorithm now just assumes that it should be the first one found. But hard to solve.. (Minor) UPDATE accountlines SET accounttype = 'MANUAL' WHERE accounttype = 'M' => How do you know that you are not moving sundry to manual fee here? Or is that okay (did we manage to get rid of it completely) ? (Major) t/db_dependent/api/v1/patrons_accounts.t not ok 5 - 200 OK # Failed test '200 OK' # at t/db_dependent/api/v1/patrons_accounts.t line 95. # got: '500' # expected: '200' not ok 6 - exact match for JSON Pointer "" # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_accounts.t line 95. # Structures begin differing at: # $got->{balance} = Does not exist # $expected->{balance} = '100.01' not ok 11 - 200 OK # Failed test '200 OK' # at t/db_dependent/api/v1/patrons_accounts.t line 139. # got: '500' # expected: '200' not ok 12 - exact match for JSON Pointer "" # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/patrons_accounts.t line 139. # Structures begin differing at: # $got->{outstanding_debits} = Does not exist # $expected->{outstanding_debits} = HASH(0x55d12cacfd80) => Some other tests with API calls do pass. (Major) Three other tests still fail. Related to new constraint on type check.
(In reply to Marcel de Rooy from comment #207) > (Minor) If you archive a debit type, you should probably be no longer > allowed to create a manual invoice with that type. Corrected on the branch now. > (Minor) UPDATE accountlines SET accounttype = ? WHERE accounttype = ? > => Hope you dont have too many clashes on the prefix here ;) The algorithm > now just assumes that it should be the first one found. But hard to solve.. Agreed, I did think about this but when I looked at out customer base I couldn't find any cases in real life so i decided this was better than leaving the mess. > (Minor) UPDATE accountlines SET accounttype = 'MANUAL' WHERE accounttype = > 'M' > => How do you know that you are not moving sundry to manual fee here? Or is > that okay (did we manage to get rid of it completely) ? I believe this is resolved in previous bugs already. the two accounttypes were merged. > (Major) t/db_dependent/api/v1/patrons_accounts.t > not ok 5 - 200 OK > # Failed test '200 OK' > # at t/db_dependent/api/v1/patrons_accounts.t line 95. > # got: '500' > # expected: '200' > not ok 6 - exact match for JSON Pointer "" > # Failed test 'exact match for JSON Pointer ""' > # at t/db_dependent/api/v1/patrons_accounts.t line 95. > # Structures begin differing at: > # $got->{balance} = Does not exist > # $expected->{balance} = '100.01' > not ok 11 - 200 OK > # Failed test '200 OK' > # at t/db_dependent/api/v1/patrons_accounts.t line 139. > # got: '500' > # expected: '200' > not ok 12 - exact match for JSON Pointer "" > # Failed test 'exact match for JSON Pointer ""' > # at t/db_dependent/api/v1/patrons_accounts.t line 139. > # Structures begin differing at: > # $got->{outstanding_debits} = Does not exist > # $expected->{outstanding_debits} = HASH(0x55d12cacfd80) > => Some other tests with API calls do pass. Looking into this one at the moment. > (Major) Three other tests still fail. Related to new constraint on type > check. I can't replicate these failures still :(
Fixed the API test now on the pushed branch.
t/db_dependent/api/v1/patrons_accounts.t .. ok All tests successful. Hopeful ! Hang on
[OK] t/db_dependent/Koha/Account/Offsets.t | 7 ++++++- [OK] t/db_dependent/Members.t | 15 ++++++++------ t/db_dependent/Accounts.t (Wstat: 65280 Tests: 31 Failed: 10) Failed tests: 12-14, 17-22, 31 # Failed test 'The 'charges' attribute should be correctly filled (bug 17836)' # at t/db_dependent/ILSDI_Services.t line 230. # got: '0.00' # expected: '10.00' # Looks like you failed 1 test of 5. t/db_dependent/ILSDI_Services.t .. 3/9
Koha/Schema/Result/AccountDebitType.pm | 12 ++++- Koha/Schema/Result/Accountline.pm | 6 +-- admin/debit_types.pl | 43 +++++++++------- => Imo we should not mix schema changes with regular codebase changes. Do we have a rule for that?
[FAIL] t/db_dependent/Accounts.t [FAIL] t/db_dependent/Koha/Account.t [FAIL] t/db_dependent/Koha/Account/Lines.t [1] Comment on the first failure in Accounts.t ok 14 - outstanding_debits returns a list of Koha::Account::Line objects in list context DBD::mysql::st execute failed: CONSTRAINT `accountlines_check_type` failed for `koha_master`.`accountlines` [for Statement "INSERT INTO `accountlines` ( `amountoutstanding`, `borrowernumber`, `interface`) VALUES ( ?, ?, ? )" with ParamValues: 0=-2, 1=102, 2='commandline'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. # Looks like you planned 22 tests but ran 14. not ok 2 - outstanding_debits() tests # Failed test 'outstanding_debits() tests' # at t/db_dependent/Koha/Account.t line 113. Coming from: # create a pathological credit with amountoutstanding > 0 (BZ 14591) Koha::Account::Line->new({ borrowernumber => $patron_4->id, amount => -3, amountoutstanding => 3, interface => 'commandline' })->store(); => This statement obviously fails, since we do not set accounttype or debittype. I am inclined to think that we should start without the type check constraint. Pretty sure that we did not catch all occurrences in code where an account line is added without one of the two types. [2] The first problem in Lines.t t/db_dependent/Koha/Account/Lines.t .. 8/9 DBD::mysql::st execute failed: CONSTRAINT `accountlines_check_type` failed for `koha_master`.`accountlines` [for Statement "INSERT INTO `accountlines` ( `amount`, `amountoutstanding`, `borrowernumber`, `interface`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0=10, 1=10, 2='115', 3='commandline'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. # No tests run! # Failed test 'No tests run for subtest "void() tests"' # at t/db_dependent/Koha/Account/Lines.t line 591. What about: my $line1 = Koha::Account::Line->new({ borrowernumber => $borrower->borrowernumber, amount => 10, amountoutstanding => 10, interface => 'commandline' })->store(); Here another insert that should fail because of the new constraint.
And I am still a bit worried that we did not complete resolve the amount<0 or amount=0 cases in the upgrade..
OK, I've now grepped the codebase for INSERT INTO `accountlines` Koha::Account::Line->new Koha::Schema::Result::Accountline->new and corrected all cases.. I confident in saying these were only used in tests at this point and those have now been corrected. Putting back to Signed off.. thanks for all your efforts Marcel.. I think we might finally be there now :)
DBD::mysql::st execute failed: CONSTRAINT `accountlines_check_type` failed for `koha_master`.`accountlines` [for Statement "INSERT INTO accountlines ( borrowernumber, amountoutstanding, date, description, interface, accounttype, debit_type_code ) VALUES ( ?, ?, (select date_sub(CURRENT_DATE, INTERVAL ? DAY) ), ?, ?, ?, ? )" with ParamValues: 0='117', 1=undef, 2=6, 3='purge_zero_balance_fees should delete NULL balance fees with date after threshold day', 4='commandline', 5=undef, 6=undef] at t/db_dependent/Accounts.t line 143. 5 and 6 are both undef BOOM
t/db_dependent/Accounts.t (Wstat: 768 Tests: 33 Failed: 3) Failed tests: 12-14 t/db_dependent/ILSDI_Services.t .. ok All tests successful. t/db_dependent/Koha/Account/Offsets.t .. ok All tests successful. t/db_dependent/Members.t .. ok All tests successful. prove t/db_dependent/Koha/Account.t t/db_dependent/Koha/Account.t .. 3/11 DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_master`.`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`) VALUES ( ?, ?, ?, ?, NOW(), ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0=5, 1=5, 2=169, 3='X8YVxu', 4='failure', 5='type validation failure', 6='commandline', 7=undef, 8=undef, 9=169, 10='this should fail anyway', 11=undef] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. t/db_dependent/Koha/Account.t .. ok All tests successful. => Please check for me if this exception was intended (it seems). Should we catch the warning instead ? t/db_dependent/Koha/Account/Lines.t .. ok All tests successful.
Hmm, The exception is intended and is in fact caught and rethrown within Koha::Account.pm.. not sure why it results in noise in the test though.. it should be hidden by the 'throws_ok' call.. Investigating.
(In reply to Martin Renvoize from comment #218) > Hmm, > > The exception is intended and is in fact caught and rethrown within > Koha::Account.pm.. not sure why it results in noise in the test though.. it > should be hidden by the 'throws_ok' call.. > > Investigating. my $schema = Koha::Database->new->schema; $schema->storage->dbh->{PrintError} = 0; Might do it in Koha/Account.t Please try
Pushed again... I managed at least to get this all running under MariaDB 10.2.27 and see the check constraint in action myself. Turns out one cannot use a ternary the way I tried to inside an execute call. All tests should now pass, the warning should now be suppressed and the db update now also handles the '0' case.
(In reply to Martin Renvoize from comment #220) > I managed at least to get this all running under MariaDB 10.2.27 and see the > check constraint in action myself. Turns out one cannot use a ternary the > way I tried to inside an execute call. I think that you are mistaken. You used the wrong conditions in the ternary: - $sth->execute($borrower->borrowernumber, $data->{amount}, $data->{days_ago}, $data->{description}, 'commandline', $data->{amount} > 0 ? 'W' : undef, $data->{amount} >= 0 ? undef : 'OVERDUE' ); + $sth->execute($borrower->borrowernumber, $data->{amount}, $data->{days_ago}, $data->{description}, 'commandline', $data->{amount} < 0 ? 'W' : undef, $data->{amount} < 0 ? undef : 'OVERDUE' ); This should work. Note the differences between >0 and <0 but also between >=0 and <0. Credit is negative ! I would suggest to remove patch 34. Coming back here soon.
Created attachment 94648 [details] [review] Bug 23049: [LAST PATCH ONLY] (QA follow-up) Catch '0' case debits Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #221) > (In reply to Martin Renvoize from comment #220) > > I managed at least to get this all running under MariaDB 10.2.27 and see the > > check constraint in action myself. Turns out one cannot use a ternary the > > way I tried to inside an execute call. > > I think that you are mistaken. You used the wrong conditions in the ternary: > > - $sth->execute($borrower->borrowernumber, $data->{amount}, > $data->{days_ago}, $data->{description}, 'commandline', $data->{amount} > 0 > ? 'W' : undef, $data->{amount} >= 0 ? undef : 'OVERDUE' ); > + $sth->execute($borrower->borrowernumber, $data->{amount}, > $data->{days_ago}, $data->{description}, 'commandline', $data->{amount} < 0 > ? 'W' : undef, $data->{amount} < 0 ? undef : 'OVERDUE' ); > > This should work. Note the differences between >0 and <0 but also between > >=0 and <0. Credit is negative ! > I would suggest to remove patch 34. > Coming back here soon. We keep the new code. Only adjust the commit message.
Nice work! Pushed to master for 19.11.00
*** Bug 23636 has been marked as a duplicate of this bug. ***
enchancment wont be backported to 19.05.x series