Bug 23805 - Add a dedicated credit_types table
Summary: Add a dedicated credit_types table
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL: https://github.com/PTFS-Europe/koha/t...
Keywords:
Depends on: 23049
Blocks: 23894 24080 24177
  Show dependency treegraph
 
Reported: 2019-10-11 15:20 UTC by Martin Renvoize
Modified: 2019-12-10 14:00 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
This patchset moves the remaining accounttypes, all of which should be credits after bug 23049, into their own table. 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 credit_type_code field for accountlines of type 'credit' and use the updated coded values.
Version(s) released in:
19.11.00


Attachments
Bug 23805: Add account_credit_types tables (9.14 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: DBIC Classes (7.83 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update references to accounttype to credit_type_code (11.57 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update accounttype references in tests (6.07 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'C' to 'CREDIT' for consistency (7.00 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'FOR' to 'FORGIVEN' for consistency (8.06 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'lost_item_return' to 'LOST_RETURN' for consistency (2.56 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'Pay' to 'PAYMENT' for consistency (17.48 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'W' to 'WRITEOFF' for consistency (11.91 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Drop type lookup as it's now a foreign key (9.63 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: (follow-up) Lowercase credit type for UpdateStats (1.12 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update mancredit to use the new credit types (15.72 KB, patch)
2019-10-15 08:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Add account_credit_types tables (9.14 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: DBIC Classes (7.83 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update references to accounttype to credit_type_code (11.57 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update accounttype references in tests (6.07 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'C' to 'CREDIT' for consistency (7.00 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'FOR' to 'FORGIVEN' for consistency (8.06 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'lost_item_return' to 'LOST_RETURN' for consistency (2.56 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'Pay' to 'PAYMENT' for consistency (17.48 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update 'W' to 'WRITEOFF' for consistency (11.91 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Drop type lookup as it's now a foreign key (9.63 KB, patch)
2019-10-15 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: (follow-up) Lowercase credit type for UpdateStats (1.12 KB, patch)
2019-10-15 11:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: Update mancredit to use the new credit types (15.72 KB, patch)
2019-10-15 11:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23049: (follow-up) Handle unexpected types (1.32 KB, patch)
2019-10-15 11:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: (follow-up) Database update corrections (2.19 KB, patch)
2019-10-26 06:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: (RM follow-up) updatedatabase.pl mariadb support (1.58 KB, patch)
2019-10-28 17:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: (follow-up) [alternative] Re-order check constraint (1.82 KB, patch)
2019-10-29 13:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23805: (RM follow-up) updatedatabase.pl mariadb support (1.68 KB, patch)
2019-10-31 08:43 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-10-11 15:20:42 UTC
In bug 23049 we split debit_types out of accounttypes and setup foreign key relationships to ensure data integrity.

We should do the same for credit_types and thus deprecate the formerly mixed accounttypes field.

This leads to clearer identification of debits vs credits in the accountlines table and enforced consistency in the data via foreign key relationships.
Comment 1 Martin Renvoize 2019-10-15 08:47:15 UTC
Created attachment 94153 [details] [review]
Bug 23805: Add account_credit_types tables
Comment 2 Martin Renvoize 2019-10-15 08:47:18 UTC
Created attachment 94154 [details] [review]
Bug 23805: DBIC Classes
Comment 3 Martin Renvoize 2019-10-15 08:47:21 UTC
Created attachment 94155 [details] [review]
Bug 23805: Update references to accounttype to credit_type_code
Comment 4 Martin Renvoize 2019-10-15 08:47:24 UTC
Created attachment 94156 [details] [review]
Bug 23805: Update accounttype references in tests
Comment 5 Martin Renvoize 2019-10-15 08:47:26 UTC
Created attachment 94157 [details] [review]
Bug 23805:  Update 'C' to 'CREDIT' for consistency
Comment 6 Martin Renvoize 2019-10-15 08:47:29 UTC
Created attachment 94158 [details] [review]
Bug 23805:  Update 'FOR' to 'FORGIVEN' for consistency
Comment 7 Martin Renvoize 2019-10-15 08:47:32 UTC
Created attachment 94159 [details] [review]
Bug 23805:  Update 'lost_item_return' to 'LOST_RETURN' for consistency
Comment 8 Martin Renvoize 2019-10-15 08:47:34 UTC
Created attachment 94160 [details] [review]
Bug 23805:  Update 'Pay' to 'PAYMENT' for consistency
Comment 9 Martin Renvoize 2019-10-15 08:47:37 UTC
Created attachment 94161 [details] [review]
Bug 23805:  Update 'W' to 'WRITEOFF' for consistency
Comment 10 Martin Renvoize 2019-10-15 08:47:40 UTC
Created attachment 94162 [details] [review]
Bug 23805: Drop type lookup as it's now a foreign key
Comment 11 Martin Renvoize 2019-10-15 08:47:43 UTC
Created attachment 94163 [details] [review]
Bug 23805: (follow-up) Lowercase credit type for UpdateStats
Comment 12 Martin Renvoize 2019-10-15 08:47:46 UTC
Created attachment 94164 [details] [review]
Bug 23805: Update mancredit to use the new credit types

Test plan
Ensure the manual credit page continues to work as it did before this
patchset.
Comment 13 Martin Renvoize 2019-10-15 11:05:29 UTC
Created attachment 94166 [details] [review]
Bug 23805: Add account_credit_types tables
Comment 14 Martin Renvoize 2019-10-15 11:05:32 UTC
Created attachment 94167 [details] [review]
Bug 23805: DBIC Classes
Comment 15 Martin Renvoize 2019-10-15 11:05:35 UTC
Created attachment 94168 [details] [review]
Bug 23805: Update references to accounttype to credit_type_code
Comment 16 Martin Renvoize 2019-10-15 11:05:39 UTC
Created attachment 94169 [details] [review]
Bug 23805: Update accounttype references in tests
Comment 17 Martin Renvoize 2019-10-15 11:05:42 UTC
Created attachment 94170 [details] [review]
Bug 23805:  Update 'C' to 'CREDIT' for consistency
Comment 18 Martin Renvoize 2019-10-15 11:05:45 UTC
Created attachment 94171 [details] [review]
Bug 23805:  Update 'FOR' to 'FORGIVEN' for consistency
Comment 19 Martin Renvoize 2019-10-15 11:05:48 UTC
Created attachment 94172 [details] [review]
Bug 23805:  Update 'lost_item_return' to 'LOST_RETURN' for consistency
Comment 20 Martin Renvoize 2019-10-15 11:05:51 UTC
Created attachment 94173 [details] [review]
Bug 23805:  Update 'Pay' to 'PAYMENT' for consistency
Comment 21 Martin Renvoize 2019-10-15 11:05:54 UTC
Created attachment 94174 [details] [review]
Bug 23805:  Update 'W' to 'WRITEOFF' for consistency
Comment 22 Martin Renvoize 2019-10-15 11:05:58 UTC
Created attachment 94175 [details] [review]
Bug 23805: Drop type lookup as it's now a foreign key
Comment 23 Martin Renvoize 2019-10-15 11:06:01 UTC
Created attachment 94176 [details] [review]
Bug 23805: (follow-up) Lowercase credit type for UpdateStats
Comment 24 Martin Renvoize 2019-10-15 11:06:04 UTC
Created attachment 94177 [details] [review]
Bug 23805: Update mancredit to use the new credit types

Test plan
Ensure the manual credit page continues to work as it did before this
patchset.
Comment 25 Martin Renvoize 2019-10-15 11:06:07 UTC
Created attachment 94178 [details] [review]
Bug 23049: (follow-up) Handle unexpected types
Comment 26 Martin Renvoize 2019-10-24 09:49:31 UTC
I've moved this into a github branch (to keep the rebasing tidy): https://github.com/PTFS-Europe/koha/tree/bug_23805_credit

Ready for signoff.

Test plan:
1) Checkout branch 
2) Run all accounts related tests (They should all pass)
3) Check the 'manual invoice' functionality still works as expected.
4) Check the 'cash register stats' report

Signoff


Tests
t/db_dependent/Circulation.t
t/db_dependent/ILSDI_Services.t 
t/db_dependent/Koha/Account.t
t/db_dependent/Koha/Account/Lines.t
t/db_dependent/api/v1/patrons_accounts.t
Comment 27 Kyle M Hall 2019-10-24 13:57:46 UTC
Testing looks good! Unit tests all pass as well for me. QA script passes with flying colors. Signing off!
Comment 28 Tomás Cohen Arazi 2019-10-24 15:52:17 UTC
All good. Tests coverage, and QA tools do great.
Comment 29 Martin Renvoize 2019-10-24 16:26:51 UTC
Nice work!

Pushed to master for 19.11.00
Comment 30 Marcel de Rooy 2019-10-25 05:41:30 UTC
Upgrade to 19.06.00.043 done (Bug 23049 - Add account debit_types)
DBD::mysql::db do failed: Duplicate CHECK constraint name 'accountlines_check_type' [for Statement "
          ALTER TABLE
            accountlines
          ADD CONSTRAINT
            `accountlines_check_type`
          CHECK (credit_type_code IS NOT NULL OR debit_type_code IS NOT NULL)
        "] at installer/data/mysql/updatedatabase.pl line 20015.
DBD::mysql::db do failed: Duplicate CHECK constraint name 'accountlines_check_type' [for Statement "
        ALTER TABLE accountlines ADD CONSTRAINT `accountlines_check_type` CHECK (accounttype IS NOT NULL OR credit_type_code IS NOT NULL)
        "] at installer/data/mysql/updatedatabase.pl line 20081.
Upgrade to 19.06.00.044 done (Bug 23049 - Add account credit_types)
Comment 31 Marcel de Rooy 2019-10-25 05:42:19 UTC
(In reply to Tomás Cohen Arazi from comment #28)
> All good. Tests coverage, and QA tools do great.

Hmm. Bit fast?
Comment 32 Marcel de Rooy 2019-10-25 05:42:43 UTC
ALTER TABLE accountlines ADD CONSTRAINT `accountlines_check_type` CHECK (accounttype IS NOT NULL OR debit_type_code IS NOT NULL)
 # Dropping the check constraint in accountlines
ALTER TABLE
            accountlines
          ADD CONSTRAINT
            `accountlines_check_type`
          CHECK (credit_type_code IS NOT NULL OR debit_type_code IS NOT NULL)
[...]
    # Adding a check constraints to accountlines
    $dbh->do(
        qq{
        ALTER TABLE accountlines ADD CONSTRAINT `accountlines_check_type` CHECK (accounttype IS NOT NULL OR credit_type_code IS NOT NULL)
        }
    );

=> Come on, ADD is not dropping a constraint !
Comment 33 Marcel de Rooy 2019-10-25 05:43:52 UTC
ALTER TABLE table DROP CHECK constraint;
Comment 34 Marcel de Rooy 2019-10-25 05:46:43 UTC
  `accounttype` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `credit_type_code` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `debit_type_code` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
Why does accounttype still exist ?

kohastructure
-  `accounttype` varchar(80) default NULL,
+  `credit_type_code` varchar(80) default NULL,
but no dropping accounttype in the dbrev
Comment 35 Marcel de Rooy 2019-10-25 05:47:20 UTC
(In reply to Marcel de Rooy from comment #34)
>   `accounttype` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
>   `credit_type_code` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
>   `debit_type_code` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
> Why does accounttype still exist ?
> 
> kohastructure
> -  `accounttype` varchar(80) default NULL,
> +  `credit_type_code` varchar(80) default NULL,
> but no dropping accounttype in the dbrev

This should be an elementary check during QA imo.
Comment 36 Marcel de Rooy 2019-10-25 05:49:23 UTC
I will not reQA a pushed patch set, but am a bit disappointed to see these things in view of the time spent on report 23049.
Comment 37 Marcel de Rooy 2019-10-25 06:03:49 UTC
Couldnt let go yet ;)
[FAIL] t/db_dependent/Accounts.t                          |  50 ++---
[FAIL]  t/db_dependent/Circulation.t                       |  18 +-
[OK] t/db_dependent/ILSDI_Services.t                    |   1 -
[FAIL]   t/db_dependent/Koha/Account.t                      |  20 +-
[FAIL] t/db_dependent/Koha/Account/Lines.t                |   2 +-
[FAIL] t/db_dependent/api/v1/patrons_accounts.t           |   2 +-

I installed a fresh database with optional data, and ran the tests..
Comment 38 Tomás Cohen Arazi 2019-10-25 22:42:10 UTC
All the tests passed when I tested, expect from the API ones for which I submitted a fix. I think doing it outside bugzilla (branch on github and pull request) might have leaded to confusion? The branch contained more than one bug/patchset.
Comment 39 Martin Renvoize 2019-10-26 06:50:00 UTC
Created attachment 94801 [details] [review]
Bug 23805: (follow-up) Database update corrections

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2019-10-26 07:29:51 UTC
DB update corrections pushed.  

Need to get into the habit of running tests using recent versions of mysql/mariadb as that would have caught this issue.  I've submitted improvements to koha-testing-docker to facilitate this.

Thanks for spotting this and raising it Marcel.
Comment 41 Julian Maurice 2019-10-28 09:15:17 UTC
I think there's still a problem with the DB update:

DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'CHECK
            `accountlines_check_type`' at line 1 [for Statement "     
          ALTER TABLE                                                    
            accountlines
          DROP CHECK
            `accountlines_check_type`
        "] at installer/data/mysql/updatedatabase.pl line 20015.

"DROP CHECK" seems to have been added in MySQL 8.0 and is not available in MariaDB

https://mariadb.com/kb/en/library/alter-table/
https://dev.mysql.com/doc/refman/5.7/en/alter-table.html
https://dev.mysql.com/doc/refman/8.0/en/alter-table.html
Comment 42 Martin Renvoize 2019-10-28 12:48:07 UTC
Good spot Julian, thanks.. I'll look into it now.
Comment 43 Martin Renvoize 2019-10-28 17:15:03 UTC
Created attachment 94815 [details] [review]
Bug 23805: (RM follow-up) updatedatabase.pl mariadb support

MySQL and MariaDB have different syntax when it comes to dropping a
CHECK constraint.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 44 Martin Renvoize 2019-10-28 17:15:51 UTC
I tried a few solutions but this one seemed to be the most reliable.. any chance you could test this for me Julian?
Comment 45 Julian Maurice 2019-10-28 20:46:54 UTC
It looks like it doesn't work. I get the same error message, and I added a warn below `# MariaDB specific drop` and it is not displayed.
DBI is not dying (RaiseError is set to 0 at the beginning of updatedatabase.pl)

Maybe the problem could be avoided by changing the other queries. Currently the order is:
1. Drop check constraint
2. accounttype = NEW_CODE(accounttype)
3. credit_type_code = accounttype
4. Drop accounttype

Instead we can do:
1. credit_type_code = NEW_CODE(accounttype)
2. Drop accounttype

This way, no need to drop the check constraint first (because accounttype is not changed), and it is dropped automatically when the column is removed.
Comment 46 Martin Renvoize 2019-10-29 13:42:34 UTC
Created attachment 94832 [details] [review]
Bug 23805: (follow-up) [alternative] Re-order check constraint
Comment 47 Jonathan Druart 2019-10-30 11:27:04 UTC
(In reply to Martin Renvoize from comment #43)
> Created attachment 94815 [details] [review] [review]
> Bug 23805: (RM follow-up) updatedatabase.pl mariadb support
> 
> MySQL and MariaDB have different syntax when it comes to dropping a
> CHECK constraint.
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

That's what I would do, even if dirty. Maybe we should reverse the 2 queries: first try the MariaDB syntax, to highlight that it's our first choice.
Comment 48 Katrin Fischer 2019-10-30 20:36:39 UTC
(In reply to Jonathan Druart from comment #47)
> (In reply to Martin Renvoize from comment #43)
> > Created attachment 94815 [details] [review] [review] [review]
> > Bug 23805: (RM follow-up) updatedatabase.pl mariadb support
> > 
> > MySQL and MariaDB have different syntax when it comes to dropping a
> > CHECK constraint.
> > 
> > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> 
> That's what I would do, even if dirty. Maybe we should reverse the 2
> queries: first try the MariaDB syntax, to highlight that it's our first
> choice.

Have you seen the comments on bug 23914 about DBMS versions and support of check constraints?
Comment 49 Martin Renvoize 2019-10-31 08:39:05 UTC
(In reply to Julian Maurice from comment #45)
> 
> Instead we can do:
> 1. credit_type_code = NEW_CODE(accounttype)
> 2. Drop accounttype
> 
> This way, no need to drop the check constraint first (because accounttype is
> not changed), and it is dropped automatically when the column is removed.

I'm not sure that's true actually.. this is a table-level check constraint... I think auto drop is only supported at column level checks, though I may be wrong.  I've uploaded an alternative patch that attempts to do as you suggest but don't have a simple way to test it's effectiveness myself :(.
Comment 50 Martin Renvoize 2019-10-31 08:43:31 UTC
Created attachment 94895 [details] [review]
Bug 23805: (RM follow-up) updatedatabase.pl mariadb support

MySQL and MariaDB have different syntax when it comes to dropping a
CHECK constraint.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Katrin Fischer 2019-10-31 23:10:48 UTC
I am really unhappy about this tbh - we are introducing database specific things.  Pretty new to the RDBMS too if I under understand Julian's comment on bug 23914 correctly and with different syntax already for the 2 RDBMS we support atm. We had agreed on not using MysqlIsms as a community and we should have discussed this...  Is it really worth it? Do we need this check so urgently and can't solve this in another way?
Comment 52 Martin Renvoize 2019-11-01 05:43:17 UTC
Check constraints are not a mysqlism and have been in mariadb since 4 Jul 2016, postgres since before 2001-08-15.. they're not be thing.

I will remove the constraint if people are really that anti having consistent data.  It really is a "do no harm" feature, I don't see why people are getting so sound up about it.  The database update threw a warning but carried on and didn't break anything.. you just ended up without the constraint.  

If we bring this silly about supported systems we should be removing the entire restful API in my mmm opinion as mojolicious is far less easily packaged, faster moving  and unstable.

I've said my piece.
Comment 53 Martin Renvoize 2019-11-01 06:25:33 UTC
As a final note, there has been a check constraint in Koha since 2012-09-12 when one was added as part of bug 5911
Comment 54 Julian Maurice 2019-11-01 11:24:25 UTC
(In reply to Martin Renvoize from comment #52)
> Check constraints are not a mysqlism and have been in mariadb since 4 Jul
> 2016, postgres since before 2001-08-15.. they're not be thing.
> 
> I will remove the constraint if people are really that anti having
> consistent data.  It really is a "do no harm" feature, I don't see why
> people are getting so sound up about it.  The database update threw a
> warning but carried on and didn't break anything.. you just ended up without
> the constraint.  
> 
> If we bring this silly about supported systems we should be removing the
> entire restful API in my mmm opinion as mojolicious is far less easily
> packaged, faster moving  and unstable.
> 
> I've said my piece.

Just to be clear, I am all for consistent data, but I'm also all for consistent behaviour. That's why I think Koha shouldn't use a database feature that is not supported by all database systems supported by Koha (btw, does the list of supported databases - with versions - exist somewhere ?).
With a CHECK constraint, someone with MySQL 8.0.16+ will have different database constraints than someone with MySQL 5.7. So, possibly different behaviours, bugs that are hard to reproduce, ... And that, IMO, should be avoided if possible.

That particular problem could be solved if we decide that Koha now requires at least MySQL 8.0.16 (which is 6 months old) or MariaDB 10.2 (first 10.2 stable version is more than 2 years old).
But there's still the problem of the error message shown during updatedatabase.pl (because of different syntax) that need to be solved (maybe in a new bug ?)
Comment 55 Katrin Fischer 2019-11-01 13:05:15 UTC
The update fails on the MariaDB version in my kohadevbox:
Server version: 10.1.38-MariaDB-0+deb9u1 Debian 9.8

See discussion on IRC:
http://irc.koha-community.org/koha/2019-11-01#i_2186242
Comment 56 Martin Renvoize 2019-11-01 13:14:47 UTC
I've plain removed the check constraints from both this bug and 23049 too.. Clearly people are generally not interested in data integrity and it's causing too many headaches attempting to pretend MariaDB and MySQL are the same.
Comment 57 Marcel de Rooy 2019-11-05 14:19:40 UTC
(In reply to Martin Renvoize from comment #53)
> As a final note, there has been a check constraint in Koha since 2012-09-12
> when one was added as part of bug 5911

+CREATE TABLE transport_cost (
+      frombranch varchar(10) NOT NULL,
+      tobranch varchar(10) NOT NULL,
+      cost decimal(6,2) NOT NULL,
+      disable_transfer tinyint(1) NOT NULL DEFAULT 0,
+      CHECK ( frombranch <> tobranch ), -- a dud check, mysql does not support that
+      PRIMARY KEY (frombranch, tobranch),
+      CONSTRAINT transport_cost_ibfk_1 FOREIGN KEY (frombranch) REFERENCES branches (branchcode) ON DELETE CASCADE ON UPDATE CASCADE,
+      CONSTRAINT transport_cost_ibfk_2 FOREIGN KEY (tobranch) REFERENCES branches (`branchcode`) ON DELETE CASCADE ON UPDATE CASCADE
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;

a dud check? Interestingly, MySQL 5.5, 5.7 allows you to define CHECK constraints, but with the note that they are parsed, but ignored by all storage engines.
I am having the impression that this explained why I saw it and you did not.
Comment 58 Marcel de Rooy 2019-11-05 14:42:46 UTC
(In reply to Martin Renvoize from comment #56)
> I've plain removed the check constraints from both this bug and 23049 too..
> Clearly people are generally not interested in data integrity [..]

Surely people are. Too bad that you had to remove it for now. But couldnt we add a check on the about: System information ?

> and it's
> causing too many headaches attempting to pretend MariaDB and MySQL are the
> same.

On the DBMS part, since a default mysql-server install now means MariaDB in Debian, we should imo try to keep the sql stuff in mysql folder running under MariaDB and MySQL as long as practically feasible.
Making another sql folder like we did for PostgreSQL was no success. Removed in 2013.