Bug 23049 - Replace MANUAL_INV authorised value with a dedicated table
Summary: Replace MANUAL_INV authorised value with a dedicated table
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL: https://github.com/PTFS-Europe/koha/t...
Keywords:
: 23636 (view as bug list)
Depends on: 22610 23321
Blocks: 21739 22393 23805 24241 24316 24532 24542 25010 26398
  Show dependency treegraph
 
Reported: 2019-06-05 11:37 UTC by Martin Renvoize
Modified: 2022-04-26 11:16 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patchset moves the `MANUAL_INV` authorized values into their own table and adds an interface into the administration pages to allow the addition and modification of such account types. This serves as the foundation for enhancing the accounts system and leads to clearer code and more consistent data via database-level constraints. **Reports note**: Reports will need to be updated to look in the new debit_type_code field for accountlines of type 'debit' and use the updated coded values.
Version(s) released in:
19.11.00


Attachments
Bug 23049: Add debit_type (14.53 KB, patch)
2019-10-03 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add branch limitations tables (2.19 KB, patch)
2019-10-03 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update DBIC Classes for branch limitations (7.59 KB, patch)
2019-10-03 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (1.88 KB, patch)
2019-10-03 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.30 KB, patch)
2019-10-03 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (15.04 KB, patch)
2019-10-03 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Use debit_type in staff client views (4.42 KB, patch)
2019-10-03 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update existing accounttype values (940 bytes, patch)
2019-10-03 07:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Split system and local types (5.95 KB, patch)
2019-10-03 07:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add tests (6.18 KB, patch)
2019-10-03 07:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Use a filtered table instead of two tables (5.85 KB, patch)
2019-10-03 08:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Impliment feedback (10.18 KB, patch)
2019-10-03 11:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Impliment feedback (10.18 KB, patch)
2019-10-03 11:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Remove MANUAL_INV from AV (2.41 KB, patch)
2019-10-03 11:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add debit_type (14.56 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Add branch limitations tables (2.33 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Update DBIC Classes for branch limitations (7.73 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (2.02 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.44 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (15.18 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Use debit_type in staff client views (4.57 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Update existing accounttype values (1.06 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Split system and local types (6.08 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Add tests (6.33 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Use a filtered table instead of two tables (5.98 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: (follow-up) Impliment feedback (10.33 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: (follow-up) Remove MANUAL_INV from AV (2.62 KB, patch)
2019-10-03 11:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 23049: Add debit_type (28.01 KB, patch)
2019-10-10 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (2.14 KB, patch)
2019-10-10 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.22 KB, patch)
2019-10-10 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Filter out system types by default (3.58 KB, patch)
2019-10-10 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (20.10 KB, patch)
2019-10-10 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add tests (6.33 KB, patch)
2019-10-10 16:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update existing code to use debit_type (97.46 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'Res' to 'RESERVE' for consistency (7.91 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise 'rent' calls to add_debit (3.96 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'PF' to 'PROCESSING' for consistency (31.26 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for account (2.17 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency (7.93 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for lost_item (3.15 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Remove manual_debit as it's never called (1.47 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for overdue (10.84 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: 'N' -> 'NEW_CARD' (5.37 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Drop type lookup as it's now a foreign key (4.07 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update API (1.80 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Fix manual debits via add_debit (1.17 KB, patch)
2019-10-10 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add debit_type (28.14 KB, patch)
2019-10-11 11:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (2.14 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.25 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Filter out system types by default (3.58 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (20.16 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add tests (6.33 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update existing code to use debit_type (97.42 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update debit_type_code 'Res' to 'RESERVE' (8.58 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `rent` (7.56 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'PF' to 'PROCESSING' for consistency (30.43 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `account` (5.93 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency (7.93 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for lost_item (3.13 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Remove manual_debit as it's never called (1.47 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `overdue` (11.29 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'N' to 'NEW_CARD' for consistency (5.39 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Drop type lookup as it's now a foreign key (7.89 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update API (1.80 KB, patch)
2019-10-11 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: More thorough param checking (1.62 KB, patch)
2019-10-11 11:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23064: (follow-up) VARCHAR(80) + ac -> account (10.70 KB, patch)
2019-10-11 11:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add debit_type (28.14 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (2.14 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.25 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Filter out system types by default (3.58 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (20.16 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add tests (6.33 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update existing code to use debit_type (97.42 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update debit_type_code 'Res' to 'RESERVE' (8.58 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `rent` (7.56 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'PF' to 'PROCESSING' for consistency (30.43 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `account` (5.93 KB, patch)
2019-10-14 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency (7.93 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for lost_item (3.13 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Remove manual_debit as it's never called (1.47 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `overdue` (11.29 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'N' to 'NEW_CARD' for consistency (5.39 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Drop type lookup as it's now a foreign key (7.89 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update API (1.80 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: More thorough param checking (1.62 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) VARCHAR(80) + ac -> account (10.65 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Correction to includes (2.03 KB, patch)
2019-10-14 10:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add debit_type (28.14 KB, patch)
2019-10-15 07:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (2.14 KB, patch)
2019-10-15 07:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.25 KB, patch)
2019-10-15 07:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Filter out system types by default (3.58 KB, patch)
2019-10-15 07:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (20.16 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add tests (6.33 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update existing code to use debit_type (97.42 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update debit_type_code 'Res' to 'RESERVE' (8.58 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `rent` (7.56 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'PF' to 'PROCESSING' for consistency (30.43 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `account` (5.93 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency (7.93 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for lost_item (3.13 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Remove manual_debit as it's never called (1.47 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `overdue` (11.29 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'N' to 'NEW_CARD' for consistency (5.39 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Drop type lookup as it's now a foreign key (7.89 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update API (1.80 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: More thorough param checking (1.62 KB, patch)
2019-10-15 07:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) VARCHAR(80) + ac -> account (10.65 KB, patch)
2019-10-15 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Correction to includes (2.40 KB, patch)
2019-10-15 07:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add debit_type (28.14 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (2.14 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.30 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Filter out system types by default (3.58 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (20.16 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add tests (6.33 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update existing code to use debit_type (97.42 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update debit_type_code 'Res' to 'RESERVE' (8.58 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `rent` (7.56 KB, patch)
2019-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'PF' to 'PROCESSING' for consistency (30.43 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `account` (5.93 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency (7.93 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for lost_item (3.13 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Remove manual_debit as it's never called (1.47 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `overdue` (11.29 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'N' to 'NEW_CARD' for consistency (5.39 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Drop type lookup as it's now a foreign key (7.89 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update API (1.80 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: More thorough param checking (1.62 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) VARCHAR(80) + ac -> account (10.65 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Correction to includes (2.40 KB, patch)
2019-10-15 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Handle unexpected types (1.38 KB, patch)
2019-10-15 11:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add debit_type (28.14 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (2.14 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.30 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Filter out system types by default (3.58 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (20.16 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add tests (6.33 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update existing code to use debit_type (97.42 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update debit_type_code 'Res' to 'RESERVE' (8.58 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `rent` (7.56 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'PF' to 'PROCESSING' for consistency (30.43 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `account` (5.93 KB, patch)
2019-10-16 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency (7.93 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for lost_item (3.13 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Remove manual_debit as it's never called (1.47 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `overdue` (11.29 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'N' to 'NEW_CARD' for consistency (5.39 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Drop type lookup as it's now a foreign key (7.89 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update API (1.80 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: More thorough param checking (1.62 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) VARCHAR(80) + ac -> account (10.65 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Correction to includes (2.40 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Handle unexpected types (1.38 KB, patch)
2019-10-16 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add debit_type (28.14 KB, patch)
2019-10-16 18:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add CHECK constraint to accountlines (2.14 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Account types configuration - Admin page (27.30 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Filter out system types by default (3.58 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update maninvoice to reference debit types (20.16 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Add tests (6.33 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update existing code to use debit_type (97.42 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update debit_type_code 'Res' to 'RESERVE' (8.58 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `rent` (7.56 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'PF' to 'PROCESSING' for consistency (30.43 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `account` (5.93 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'HE' to 'RESERVE_EXPIRED' for consistency (7.93 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for lost_item (3.13 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Remove manual_debit as it's never called (1.47 KB, patch)
2019-10-16 18:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Capitalise type passed to add_debit for `overdue` (11.29 KB, patch)
2019-10-16 18:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update 'N' to 'NEW_CARD' for consistency (5.39 KB, patch)
2019-10-16 18:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Drop type lookup as it's now a foreign key (7.89 KB, patch)
2019-10-16 18:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: Update API (1.80 KB, patch)
2019-10-16 18:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: More thorough param checking (1.62 KB, patch)
2019-10-16 18:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) VARCHAR(80) + ac -> account (10.65 KB, patch)
2019-10-16 18:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Correction to includes (2.40 KB, patch)
2019-10-16 18:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Handle unexpected types (1.38 KB, patch)
2019-10-16 18:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: [LAST PATCH ONLY] (QA follow-up) Catch '0' case debits (919 bytes, patch)
2019-10-24 08:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-06-05 11:37:04 UTC
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.
Comment 1 Jonathan Druart 2019-06-05 23:56:12 UTC
What about translation?
Comment 2 Martin Renvoize 2019-06-13 16:11:12 UTC
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.
Comment 3 Martin Renvoize 2019-10-03 07:24:41 UTC
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
Comment 4 Martin Renvoize 2019-10-03 07:24:43 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2019-10-03 07:24:46 UTC
Created attachment 93515 [details] [review]
Bug 23049: Update DBIC Classes for branch limitations
Comment 6 Martin Renvoize 2019-10-03 07:24:49 UTC
Created attachment 93516 [details] [review]
Bug 23049: Add CHECK constraint to accountlines
Comment 7 Martin Renvoize 2019-10-03 07:24:52 UTC
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.
Comment 8 Martin Renvoize 2019-10-03 07:24:55 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2019-10-03 07:24:58 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2019-10-03 07:25:01 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2019-10-03 07:25:04 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2019-10-03 07:25:07 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize 2019-10-03 07:28:02 UTC
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.
Comment 14 Martin Renvoize 2019-10-03 08:37:40 UTC Comment hidden (obsolete)
Comment 15 Séverine Queune 2019-10-03 10:01:32 UTC
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 !
Comment 16 Kyle M Hall 2019-10-03 10:23:32 UTC
(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!
Comment 17 Martin Renvoize 2019-10-03 11:29:40 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2019-10-03 11:39:21 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize 2019-10-03 11:39:24 UTC Comment hidden (obsolete)
Comment 20 Martin Renvoize 2019-10-03 11:43:39 UTC
Thankyou for the fast feedback Séverine, I believe I've resolved those issues now with the followup :).
Comment 21 Biblibre Sandboxes 2019-10-03 11:52:12 UTC Comment hidden (obsolete)
Comment 22 Biblibre Sandboxes 2019-10-03 11:54:18 UTC Comment hidden (obsolete)
Comment 23 Biblibre Sandboxes 2019-10-03 11:54:21 UTC Comment hidden (obsolete)
Comment 24 Biblibre Sandboxes 2019-10-03 11:54:24 UTC Comment hidden (obsolete)
Comment 25 Biblibre Sandboxes 2019-10-03 11:54:28 UTC Comment hidden (obsolete)
Comment 26 Biblibre Sandboxes 2019-10-03 11:54:31 UTC Comment hidden (obsolete)
Comment 27 Biblibre Sandboxes 2019-10-03 11:54:34 UTC Comment hidden (obsolete)
Comment 28 Biblibre Sandboxes 2019-10-03 11:54:37 UTC Comment hidden (obsolete)
Comment 29 Biblibre Sandboxes 2019-10-03 11:54:40 UTC Comment hidden (obsolete)
Comment 30 Biblibre Sandboxes 2019-10-03 11:54:43 UTC Comment hidden (obsolete)
Comment 31 Biblibre Sandboxes 2019-10-03 11:54:46 UTC Comment hidden (obsolete)
Comment 32 Biblibre Sandboxes 2019-10-03 11:54:49 UTC Comment hidden (obsolete)
Comment 33 Biblibre Sandboxes 2019-10-03 11:54:53 UTC Comment hidden (obsolete)
Comment 34 Biblibre Sandboxes 2019-10-03 11:54:56 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2019-10-04 08:47:30 UTC
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.
Comment 36 Martin Renvoize 2019-10-10 16:19:41 UTC Comment hidden (obsolete)
Comment 37 Martin Renvoize 2019-10-10 16:19:44 UTC Comment hidden (obsolete)
Comment 38 Martin Renvoize 2019-10-10 16:19:48 UTC Comment hidden (obsolete)
Comment 39 Martin Renvoize 2019-10-10 16:19:51 UTC Comment hidden (obsolete)
Comment 40 Martin Renvoize 2019-10-10 16:19:54 UTC Comment hidden (obsolete)
Comment 41 Martin Renvoize 2019-10-10 16:19:58 UTC Comment hidden (obsolete)
Comment 42 Martin Renvoize 2019-10-10 16:20:01 UTC Comment hidden (obsolete)
Comment 43 Martin Renvoize 2019-10-10 16:20:06 UTC Comment hidden (obsolete)
Comment 44 Martin Renvoize 2019-10-10 16:20:09 UTC Comment hidden (obsolete)
Comment 45 Martin Renvoize 2019-10-10 16:20:13 UTC Comment hidden (obsolete)
Comment 46 Martin Renvoize 2019-10-10 16:20:16 UTC Comment hidden (obsolete)
Comment 47 Martin Renvoize 2019-10-10 16:20:19 UTC Comment hidden (obsolete)
Comment 48 Martin Renvoize 2019-10-10 16:20:23 UTC Comment hidden (obsolete)
Comment 49 Martin Renvoize 2019-10-10 16:20:26 UTC Comment hidden (obsolete)
Comment 50 Martin Renvoize 2019-10-10 16:20:29 UTC Comment hidden (obsolete)
Comment 51 Martin Renvoize 2019-10-10 16:20:33 UTC Comment hidden (obsolete)
Comment 52 Martin Renvoize 2019-10-10 16:20:36 UTC Comment hidden (obsolete)
Comment 53 Martin Renvoize 2019-10-10 16:20:39 UTC Comment hidden (obsolete)
Comment 54 Martin Renvoize 2019-10-10 16:20:43 UTC Comment hidden (obsolete)
Comment 55 Marcel de Rooy 2019-10-11 08:24:00 UTC
OK Trying to be brave, start QA
Comment 56 Marcel de Rooy 2019-10-11 08:49:09 UTC
            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 ?
Comment 57 Marcel de Rooy 2019-10-11 08:54:44 UTC
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?
Comment 58 Marcel de Rooy 2019-10-11 09:01:32 UTC
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?
Comment 59 Marcel de Rooy 2019-10-11 09:06:50 UTC
 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.
Comment 60 Martin Renvoize 2019-10-11 09:09:32 UTC
Thanks for taking a look Marcel, I'll get onto the feedback today... all of it seems sane to me so far.
Comment 61 Marcel de Rooy 2019-10-11 09:35:12 UTC
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.
Comment 62 Martin Renvoize 2019-10-11 11:19:40 UTC
(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).
Comment 63 Martin Renvoize 2019-10-11 11:21:15 UTC
(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.
Comment 64 Martin Renvoize 2019-10-11 11:29:02 UTC
(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).
Comment 65 Martin Renvoize 2019-10-11 11:42:56 UTC Comment hidden (obsolete)
Comment 66 Martin Renvoize 2019-10-11 11:43:00 UTC Comment hidden (obsolete)
Comment 67 Martin Renvoize 2019-10-11 11:43:04 UTC Comment hidden (obsolete)
Comment 68 Martin Renvoize 2019-10-11 11:43:08 UTC Comment hidden (obsolete)
Comment 69 Martin Renvoize 2019-10-11 11:43:11 UTC Comment hidden (obsolete)
Comment 70 Martin Renvoize 2019-10-11 11:43:15 UTC Comment hidden (obsolete)
Comment 71 Martin Renvoize 2019-10-11 11:43:19 UTC Comment hidden (obsolete)
Comment 72 Martin Renvoize 2019-10-11 11:43:22 UTC Comment hidden (obsolete)
Comment 73 Martin Renvoize 2019-10-11 11:43:26 UTC Comment hidden (obsolete)
Comment 74 Martin Renvoize 2019-10-11 11:43:30 UTC Comment hidden (obsolete)
Comment 75 Martin Renvoize 2019-10-11 11:43:33 UTC Comment hidden (obsolete)
Comment 76 Martin Renvoize 2019-10-11 11:43:37 UTC Comment hidden (obsolete)
Comment 77 Martin Renvoize 2019-10-11 11:43:40 UTC Comment hidden (obsolete)
Comment 78 Martin Renvoize 2019-10-11 11:43:44 UTC Comment hidden (obsolete)
Comment 79 Martin Renvoize 2019-10-11 11:43:48 UTC Comment hidden (obsolete)
Comment 80 Martin Renvoize 2019-10-11 11:43:51 UTC Comment hidden (obsolete)
Comment 81 Martin Renvoize 2019-10-11 11:43:55 UTC Comment hidden (obsolete)
Comment 82 Martin Renvoize 2019-10-11 11:43:59 UTC Comment hidden (obsolete)
Comment 83 Martin Renvoize 2019-10-11 11:44:02 UTC Comment hidden (obsolete)
Comment 84 Martin Renvoize 2019-10-11 11:44:06 UTC Comment hidden (obsolete)
Comment 85 Kyle M Hall 2019-10-11 15:00:04 UTC
(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!
Comment 86 Martin Renvoize 2019-10-14 10:34:18 UTC Comment hidden (obsolete)
Comment 87 Martin Renvoize 2019-10-14 10:34:22 UTC Comment hidden (obsolete)
Comment 88 Martin Renvoize 2019-10-14 10:34:26 UTC Comment hidden (obsolete)
Comment 89 Martin Renvoize 2019-10-14 10:34:30 UTC Comment hidden (obsolete)
Comment 90 Martin Renvoize 2019-10-14 10:34:34 UTC Comment hidden (obsolete)
Comment 91 Martin Renvoize 2019-10-14 10:34:38 UTC Comment hidden (obsolete)
Comment 92 Martin Renvoize 2019-10-14 10:34:42 UTC Comment hidden (obsolete)
Comment 93 Martin Renvoize 2019-10-14 10:34:46 UTC Comment hidden (obsolete)
Comment 94 Martin Renvoize 2019-10-14 10:34:49 UTC Comment hidden (obsolete)
Comment 95 Martin Renvoize 2019-10-14 10:34:54 UTC Comment hidden (obsolete)
Comment 96 Martin Renvoize 2019-10-14 10:34:58 UTC Comment hidden (obsolete)
Comment 97 Martin Renvoize 2019-10-14 10:35:02 UTC Comment hidden (obsolete)
Comment 98 Martin Renvoize 2019-10-14 10:35:06 UTC Comment hidden (obsolete)
Comment 99 Martin Renvoize 2019-10-14 10:35:10 UTC Comment hidden (obsolete)
Comment 100 Martin Renvoize 2019-10-14 10:35:14 UTC Comment hidden (obsolete)
Comment 101 Martin Renvoize 2019-10-14 10:35:18 UTC Comment hidden (obsolete)
Comment 102 Martin Renvoize 2019-10-14 10:35:22 UTC
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>
Comment 103 Martin Renvoize 2019-10-14 10:35:26 UTC Comment hidden (obsolete)
Comment 104 Martin Renvoize 2019-10-14 10:35:30 UTC Comment hidden (obsolete)
Comment 105 Martin Renvoize 2019-10-14 10:35:33 UTC Comment hidden (obsolete)
Comment 106 Martin Renvoize 2019-10-14 10:35:37 UTC Comment hidden (obsolete)
Comment 107 Martin Renvoize 2019-10-15 07:22:45 UTC Comment hidden (obsolete)
Comment 108 Martin Renvoize 2019-10-15 07:22:49 UTC Comment hidden (obsolete)
Comment 109 Martin Renvoize 2019-10-15 07:22:53 UTC Comment hidden (obsolete)
Comment 110 Martin Renvoize 2019-10-15 07:22:57 UTC Comment hidden (obsolete)
Comment 111 Martin Renvoize 2019-10-15 07:23:01 UTC Comment hidden (obsolete)
Comment 112 Martin Renvoize 2019-10-15 07:23:05 UTC Comment hidden (obsolete)
Comment 113 Martin Renvoize 2019-10-15 07:23:09 UTC Comment hidden (obsolete)
Comment 114 Martin Renvoize 2019-10-15 07:23:13 UTC Comment hidden (obsolete)
Comment 115 Martin Renvoize 2019-10-15 07:23:17 UTC Comment hidden (obsolete)
Comment 116 Martin Renvoize 2019-10-15 07:23:21 UTC Comment hidden (obsolete)
Comment 117 Martin Renvoize 2019-10-15 07:23:25 UTC Comment hidden (obsolete)
Comment 118 Martin Renvoize 2019-10-15 07:23:29 UTC Comment hidden (obsolete)
Comment 119 Martin Renvoize 2019-10-15 07:23:34 UTC Comment hidden (obsolete)
Comment 120 Martin Renvoize 2019-10-15 07:23:38 UTC Comment hidden (obsolete)
Comment 121 Martin Renvoize 2019-10-15 07:23:42 UTC Comment hidden (obsolete)
Comment 122 Martin Renvoize 2019-10-15 07:23:46 UTC Comment hidden (obsolete)
Comment 123 Martin Renvoize 2019-10-15 07:23:50 UTC Comment hidden (obsolete)
Comment 124 Martin Renvoize 2019-10-15 07:23:54 UTC Comment hidden (obsolete)
Comment 125 Martin Renvoize 2019-10-15 07:23:58 UTC Comment hidden (obsolete)
Comment 126 Martin Renvoize 2019-10-15 07:24:02 UTC Comment hidden (obsolete)
Comment 127 Martin Renvoize 2019-10-15 07:24:06 UTC Comment hidden (obsolete)
Comment 128 Martin Renvoize 2019-10-15 08:19:24 UTC Comment hidden (obsolete)
Comment 129 Martin Renvoize 2019-10-15 08:19:28 UTC Comment hidden (obsolete)
Comment 130 Martin Renvoize 2019-10-15 08:19:32 UTC Comment hidden (obsolete)
Comment 131 Martin Renvoize 2019-10-15 08:19:36 UTC Comment hidden (obsolete)
Comment 132 Martin Renvoize 2019-10-15 08:19:40 UTC Comment hidden (obsolete)
Comment 133 Martin Renvoize 2019-10-15 08:19:45 UTC Comment hidden (obsolete)
Comment 134 Martin Renvoize 2019-10-15 08:19:49 UTC Comment hidden (obsolete)
Comment 135 Martin Renvoize 2019-10-15 08:19:53 UTC Comment hidden (obsolete)
Comment 136 Martin Renvoize 2019-10-15 08:19:58 UTC Comment hidden (obsolete)
Comment 137 Martin Renvoize 2019-10-15 08:20:02 UTC Comment hidden (obsolete)
Comment 138 Martin Renvoize 2019-10-15 08:20:07 UTC Comment hidden (obsolete)
Comment 139 Martin Renvoize 2019-10-15 08:20:12 UTC Comment hidden (obsolete)
Comment 140 Martin Renvoize 2019-10-15 08:20:16 UTC Comment hidden (obsolete)
Comment 141 Martin Renvoize 2019-10-15 08:20:20 UTC Comment hidden (obsolete)
Comment 142 Martin Renvoize 2019-10-15 08:20:25 UTC Comment hidden (obsolete)
Comment 143 Martin Renvoize 2019-10-15 08:20:30 UTC Comment hidden (obsolete)
Comment 144 Martin Renvoize 2019-10-15 08:20:34 UTC Comment hidden (obsolete)
Comment 145 Martin Renvoize 2019-10-15 08:20:38 UTC Comment hidden (obsolete)
Comment 146 Martin Renvoize 2019-10-15 08:20:42 UTC Comment hidden (obsolete)
Comment 147 Martin Renvoize 2019-10-15 08:20:47 UTC Comment hidden (obsolete)
Comment 148 Martin Renvoize 2019-10-15 08:20:51 UTC Comment hidden (obsolete)
Comment 149 Martin Renvoize 2019-10-15 11:02:43 UTC Comment hidden (obsolete)
Comment 150 Kyle M Hall 2019-10-16 15:23:09 UTC
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)
Comment 151 Martin Renvoize 2019-10-16 15:30:05 UTC Comment hidden (obsolete)
Comment 152 Martin Renvoize 2019-10-16 15:30:15 UTC Comment hidden (obsolete)
Comment 153 Martin Renvoize 2019-10-16 15:30:19 UTC Comment hidden (obsolete)
Comment 154 Martin Renvoize 2019-10-16 15:30:23 UTC Comment hidden (obsolete)
Comment 155 Martin Renvoize 2019-10-16 15:30:28 UTC Comment hidden (obsolete)
Comment 156 Martin Renvoize 2019-10-16 15:30:32 UTC Comment hidden (obsolete)
Comment 157 Martin Renvoize 2019-10-16 15:30:37 UTC Comment hidden (obsolete)
Comment 158 Martin Renvoize 2019-10-16 15:30:42 UTC Comment hidden (obsolete)
Comment 159 Martin Renvoize 2019-10-16 15:30:46 UTC Comment hidden (obsolete)
Comment 160 Martin Renvoize 2019-10-16 15:30:51 UTC Comment hidden (obsolete)
Comment 161 Martin Renvoize 2019-10-16 15:30:55 UTC Comment hidden (obsolete)
Comment 162 Martin Renvoize 2019-10-16 15:31:00 UTC Comment hidden (obsolete)
Comment 163 Martin Renvoize 2019-10-16 15:31:06 UTC Comment hidden (obsolete)
Comment 164 Martin Renvoize 2019-10-16 15:31:11 UTC Comment hidden (obsolete)
Comment 165 Martin Renvoize 2019-10-16 15:31:16 UTC Comment hidden (obsolete)
Comment 166 Martin Renvoize 2019-10-16 15:31:21 UTC Comment hidden (obsolete)
Comment 167 Martin Renvoize 2019-10-16 15:31:26 UTC Comment hidden (obsolete)
Comment 168 Martin Renvoize 2019-10-16 15:31:31 UTC Comment hidden (obsolete)
Comment 169 Martin Renvoize 2019-10-16 15:31:36 UTC Comment hidden (obsolete)
Comment 170 Martin Renvoize 2019-10-16 15:31:40 UTC Comment hidden (obsolete)
Comment 171 Martin Renvoize 2019-10-16 15:31:44 UTC Comment hidden (obsolete)
Comment 172 Martin Renvoize 2019-10-16 15:31:50 UTC Comment hidden (obsolete)
Comment 173 Martin Renvoize 2019-10-16 18:06:56 UTC Comment hidden (obsolete)
Comment 174 Martin Renvoize 2019-10-16 18:07:01 UTC Comment hidden (obsolete)
Comment 175 Martin Renvoize 2019-10-16 18:07:06 UTC Comment hidden (obsolete)
Comment 176 Martin Renvoize 2019-10-16 18:07:10 UTC Comment hidden (obsolete)
Comment 177 Martin Renvoize 2019-10-16 18:07:15 UTC Comment hidden (obsolete)
Comment 178 Martin Renvoize 2019-10-16 18:07:20 UTC Comment hidden (obsolete)
Comment 179 Martin Renvoize 2019-10-16 18:07:26 UTC Comment hidden (obsolete)
Comment 180 Martin Renvoize 2019-10-16 18:07:31 UTC Comment hidden (obsolete)
Comment 181 Martin Renvoize 2019-10-16 18:07:35 UTC Comment hidden (obsolete)
Comment 182 Martin Renvoize 2019-10-16 18:07:40 UTC Comment hidden (obsolete)
Comment 183 Martin Renvoize 2019-10-16 18:07:45 UTC Comment hidden (obsolete)
Comment 184 Martin Renvoize 2019-10-16 18:07:49 UTC Comment hidden (obsolete)
Comment 185 Martin Renvoize 2019-10-16 18:07:54 UTC Comment hidden (obsolete)
Comment 186 Martin Renvoize 2019-10-16 18:07:59 UTC Comment hidden (obsolete)
Comment 187 Martin Renvoize 2019-10-16 18:08:04 UTC Comment hidden (obsolete)
Comment 188 Martin Renvoize 2019-10-16 18:08:09 UTC Comment hidden (obsolete)
Comment 189 Martin Renvoize 2019-10-16 18:08:13 UTC Comment hidden (obsolete)
Comment 190 Martin Renvoize 2019-10-16 18:08:18 UTC Comment hidden (obsolete)
Comment 191 Martin Renvoize 2019-10-16 18:08:23 UTC Comment hidden (obsolete)
Comment 192 Martin Renvoize 2019-10-16 18:08:27 UTC Comment hidden (obsolete)
Comment 193 Martin Renvoize 2019-10-16 18:08:32 UTC Comment hidden (obsolete)
Comment 194 Martin Renvoize 2019-10-16 18:08:37 UTC Comment hidden (obsolete)
Comment 195 Martin Renvoize 2019-10-16 18:17:39 UTC
(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 :)
Comment 196 Martin Renvoize 2019-10-17 09:09:17 UTC
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
Comment 197 Marcel de Rooy 2019-10-18 05:28:14 UTC
Martin Renvoize <martin.renvoize@ptfs-europe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         QA Contact|m.de.rooy@rijksmuseum.nl    |kyle@bywatersolutions.com

Whats happening here ?
Comment 198 Marcel de Rooy 2019-10-18 05:42:26 UTC
Still working on this one, Martin. Dont change QA contact without communication please, especially within such a short time frame.
Comment 199 Marcel de Rooy 2019-10-18 08:09:48 UTC
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.
Comment 200 Marcel de Rooy 2019-10-18 08:10:50 UTC
[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
Comment 201 Marcel de Rooy 2019-10-18 08:15:26 UTC
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.
Comment 202 Marcel de Rooy 2019-10-18 08:16:38 UTC
(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.
Comment 203 Martin Renvoize 2019-10-18 09:02:56 UTC
(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 :)
Comment 204 Martin Renvoize 2019-10-18 12:27:04 UTC
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 :(.
Comment 205 Marcel de Rooy 2019-10-18 15:19:11 UTC
(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
Comment 206 Marcel de Rooy 2019-10-21 11:27:25 UTC
Revisiting
Comment 207 Marcel de Rooy 2019-10-21 11:35:39 UTC
(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.
Comment 208 Martin Renvoize 2019-10-22 09:59:21 UTC
(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 :(
Comment 209 Martin Renvoize 2019-10-22 10:16:00 UTC
Fixed the API test now on the pushed branch.
Comment 210 Marcel de Rooy 2019-10-23 11:45:53 UTC
t/db_dependent/api/v1/patrons_accounts.t .. ok
All tests successful.

Hopeful ! Hang on
Comment 211 Marcel de Rooy 2019-10-23 11:50:01 UTC
[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
Comment 212 Marcel de Rooy 2019-10-23 11:54:08 UTC
 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?
Comment 213 Marcel de Rooy 2019-10-23 12:02:26 UTC
[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.
Comment 214 Marcel de Rooy 2019-10-23 12:15:52 UTC
And I am still a bit worried that we did not complete resolve the amount<0 or amount=0 cases in the upgrade..
Comment 215 Martin Renvoize 2019-10-23 13:38:15 UTC
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 :)
Comment 216 Marcel de Rooy 2019-10-23 13:57:04 UTC
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
Comment 217 Marcel de Rooy 2019-10-23 14:01:50 UTC
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.
Comment 218 Martin Renvoize 2019-10-23 14:10:56 UTC
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.
Comment 219 Marcel de Rooy 2019-10-23 14:33:51 UTC
(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
Comment 220 Martin Renvoize 2019-10-23 15:42:45 UTC
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.
Comment 221 Marcel de Rooy 2019-10-24 06:58:05 UTC
(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.
Comment 222 Marcel de Rooy 2019-10-24 08:09:33 UTC
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>
Comment 223 Marcel de Rooy 2019-10-24 08:11:03 UTC
(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.
Comment 224 Martin Renvoize 2019-10-24 16:26:47 UTC
Nice work!

Pushed to master for 19.11.00
Comment 225 Martin Renvoize 2019-10-24 21:09:55 UTC
*** Bug 23636 has been marked as a duplicate of this bug. ***
Comment 226 Lucas Gass 2020-01-07 22:05:29 UTC
enchancment wont be backported to 19.05.x series