Created attachment 94153 [details] [review]
Bug 23805: Add account_credit_types tables

Created attachment 94154 [details] [review]
Bug 23805: DBIC Classes

Created attachment 94155 [details] [review]
Bug 23805: Update references to accounttype to credit_type_code

Created attachment 94156 [details] [review]
Bug 23805: Update accounttype references in tests

Created attachment 94157 [details] [review]
Bug 23805:  Update 'C' to 'CREDIT' for consistency

Created attachment 94158 [details] [review]
Bug 23805:  Update 'FOR' to 'FORGIVEN' for consistency

Created attachment 94159 [details] [review]
Bug 23805:  Update 'lost_item_return' to 'LOST_RETURN' for consistency

Created attachment 94160 [details] [review]
Bug 23805:  Update 'Pay' to 'PAYMENT' for consistency

Created attachment 94161 [details] [review]
Bug 23805:  Update 'W' to 'WRITEOFF' for consistency

Created attachment 94162 [details] [review]
Bug 23805: Drop type lookup as it's now a foreign key

Created attachment 94163 [details] [review]
Bug 23805: (follow-up) Lowercase credit type for UpdateStats

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.

Created attachment 94166 [details] [review]
Bug 23805: Add account_credit_types tables

Created attachment 94167 [details] [review]
Bug 23805: DBIC Classes

Created attachment 94168 [details] [review]
Bug 23805: Update references to accounttype to credit_type_code

Created attachment 94169 [details] [review]
Bug 23805: Update accounttype references in tests

Created attachment 94170 [details] [review]
Bug 23805:  Update 'C' to 'CREDIT' for consistency

Created attachment 94171 [details] [review]
Bug 23805:  Update 'FOR' to 'FORGIVEN' for consistency

Created attachment 94172 [details] [review]
Bug 23805:  Update 'lost_item_return' to 'LOST_RETURN' for consistency

Created attachment 94173 [details] [review]
Bug 23805:  Update 'Pay' to 'PAYMENT' for consistency

Created attachment 94174 [details] [review]
Bug 23805:  Update 'W' to 'WRITEOFF' for consistency

Created attachment 94175 [details] [review]
Bug 23805: Drop type lookup as it's now a foreign key

Created attachment 94176 [details] [review]
Bug 23805: (follow-up) Lowercase credit type for UpdateStats

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.

Created attachment 94178 [details] [review]
Bug 23049: (follow-up) Handle unexpected types

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

Testing looks good! Unit tests all pass as well for me. QA script passes with flying colors. Signing off!

All good. Tests coverage, and QA tools do great.

Nice work!

Pushed to master for 19.11.00

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)

(In reply to Tomás Cohen Arazi from comment #28)
> All good. Tests coverage, and QA tools do great.

Hmm. Bit fast?

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 !

ALTER TABLE table DROP CHECK constraint;

  `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

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

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.

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

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.

Created attachment 94801 [details] [review]
Bug 23805: (follow-up) Database update corrections

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

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.

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

Good spot Julian, thanks.. I'll look into it now.

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>

I tried a few solutions but this one seemed to be the most reliable.. any chance you could test this for me Julian?

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.

Created attachment 94832 [details] [review]
Bug 23805: (follow-up) [alternative] Re-order check constraint

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

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

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

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>

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?

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.

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

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

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

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.

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

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