Sets DB rev number starting with 3.08 instead of 3.09.  Also uses CHANGE instead of MODIFY, and since we're not renaming the column, better to use what we've used in the past to do this job.

Small things, I know, but marking Failed QA until they're resolved.

(In reply to comment #4)
> Sets DB rev number starting with 3.08 instead of 3.09.  Also uses CHANGE
> instead of MODIFY, and since we're not renaming the column, better to use
> what we've used in the past to do this job.
> 
> Small things, I know, but marking Failed QA until they're resolved.

oups... seems I made a mistake, by missing Ian (perfectly correct) QA comment.
As I already had pushed the bugfix provided, I also have pushed a follow-up to fix this QA comment.

Ian, please check everything is OK, and sorry again (i'm back from holiday, maybe my brain is still not 100% efficient ;-) )

patch and follow-up pushed

WARNING:

This patch borrower_attribute_types by truncating category_code content.

Those lines:

`category_code` VARCHAR(1) NULL DEFAULT NULL,-- defines a category for an attribute_type

$DBversion = "3.08.00.XXX";
if ( C4::Context->preference("Version") < TransformToNum($DBversion) ) {
    $dbh->do("ALTER TABLE borrower_attribute_types CHANGE category_code category_code VARCHAR( 1 ) NULL DEFAULT NULL");
    print "Upgrade to $DBversion done. (Bug 8002: Update patron attribute types table to allow NULL category_code)\n";
    SetVersion($DBversion);
}

are wrong. category_code length is 10 not 1! It means that after update,
attributes by category don't work anymore, and configuration is lost.

mmm... The patch is:
-  `category_code` VARCHAR(1) NOT NULL DEFAULT '',-- defines a category for an attribute_type
+  `category_code` VARCHAR(1) NULL DEFAULT NULL,-- defines a category for an attribute_type

so category_code is already set to varchar(1) before this patch. This patch just set NULL instead of NOT NULL

Digging kohastructure git history now

The patch introducing this column is: bb35b608282e670e1bb07bf4cea324fc16b6ec90
Auteur: Jonathan Druart <jonathan.druart@biblibre.com>  2012-02-10 09:57:35
    Bug 7154: Modification in borrower attributes
    
    * group some attributes for a more friendly display
    
    * allow a link between a borrower category and an attribute to display
      it only for the specified category
    
    * Attr classes filled with AV 'PA_CLASS' category (you have to create AV
      with a new category 'PA_CLASS' to fill class list)
    
    this patch add 2 fields in borrower_attribute_types :
        category_code
        class
    
    Signed-off-by: Julien Sicot <julien.sicot@gmail.com>
    
    Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>

I just tried to add a filtered attribute =
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`lecannetdesmaures`.`borrower_attribute_types`, CONSTRAINT `category_code_fk` FOREIGN KEY (`category_code`) REFERENCES `categories` (`categorycode`)) at /home/paul/koha.dev/koha-community/C4/Members/AttributeTypes.pm line 220.

!!!

Just spoke with jonathan, and, unfortunately, in his test database, he only has 1 digit categories, so it work fine !!!

Patch coming ;-)

Thanks a lot for fixing it so quickly! I will sign-off the patch.

Don't you think a warning, somewhere, must be addressed to Koha
sysadmin? I propose: 

Warning to Koha System Administrators: If you use borrower attributes
defined by borrower categories, you have to check your configuration.  A
bug may have removed your attribute links to borrower categories.
Please check, and fix it if necessary.

(In reply to comment #12)
> Don't you think a warning, somewhere, must be addressed to Koha
> sysadmin?

I think it is not necessary given that informations were not updated (or inserted).
Moreover the issue exists only for a new install (not updated install).

> I think it is not necessary given that informations were not updated
> (or inserted).  Moreover the issue exists only for a new install (not
> updated install).

In updatedatabase.pl, on HEAD branch, line 5218, an ALTER TABLE truncate
category_code field from 10 to 1 character. Same issue, same line in
3.8.x, for 3.08.00.001 version. So we need a patch for the two branches.
And a warning message. I confirm that I have a library on 3.8 which has
had truncated its attributes by borrower categories.

Comment on attachment 12012 [details] [review]
[SIGNED-OFF] FIX Change datatype from varchar 1 to 10

updatedatabase.pl must also be fixed.

Yes I have just seen that ! But why mysql breaks a constraint ??

> Yes I have just seen that ! But why mysql breaks a constraint ??

When category_code is truncated to 1 character, it is no more possible
to link borrower attribute types to borrower categories which are still
10 characters length. And if 2 attributes have been linked to 2
distinct borrower categories like ENFANT and ETUDIANT, category_code
will contain E after ALTER TABLE for both attributes, and the link won't
work anymore.

Created attachment 12034 [details] [review]
[3.8.X] Bug 8002: FIX Change datatype from varchar 1 to 10 for borrower_attribute_types.category_code

(In reply to comment #18)
> > Yes I have just seen that ! But why mysql breaks a constraint ??
> 
> When category_code is truncated to 1 character, it is no more possible
> to link borrower attribute types to borrower categories which are still
> 10 characters length. And if 2 attributes have been linked to 2
> distinct borrower categories like ENFANT and ETUDIANT, category_code
> will contain E after ALTER TABLE for both attributes, and the link won't
> work anymore.

Hi Frédéric,
Yes, I has understand.
I would said it is strange that MySQL does not keep integrity and raise an error when we want to reduce data.
Besides
But it is normal MySQL does what we ask him :)

I proposed 2 patchs (same as previous). 1 for master, 1 for 3.8.x
A warning message is displayed on the updatedatabase page.
I hope it is what you wanted.

(In reply to comment #21)
> But it is normal MySQL does what we ask him :)

Definetly it's not normal, and it's a bug !
When you don't have a constraint and try to add one, if there are data that would not respect the constraint, you get a message, and the constraint is not set. Seel update22to30.pl to see how I handled this case in the past = I throw a clear warning, with the query to run to see which data was wrong, so the library could fix it and put the constraint.

I'm expecting from ALTER TABLE that an existing constraint is checked before the table is altered !
Also note that, iirc, you can't put a constraint on 2 columns with different type/size. That could be easily checked by mysql when you ALTER TABLE !

Created attachment 12206 [details] [review]
Bug 8002: FIX Change datatype from varchar 1 to 10 for borrower_attribute_types.category_code

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

(In reply to comment #23)
> Created attachment 12206 [details] [review]
> Bug 8002: FIX Change datatype from varchar 1 to 10 for
> borrower_attribute_types.category_code
> 
> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

passing QA on patch against master


$ koha-qa.pl 

testing 1 commit(s) (applied to commit 60790d0)
 * 5599413 Bug 8002: FIX Change datatype from varchar 1 to 10 for borrower_a
      installer/data/mysql/kohastructure.sql
      installer/data/mysql/updatedatabase.pl

* installer/data/mysql/updatedatabase.pl                                   OK

Patch pushed to master

Patch pushed on master branch directly, as the new/bug_8002 is old

Pushed to 3.8.x, will be in 3.8.5