Bug 8002 - Can't add patron attribute type in newer installation
Summary: Can't add patron attribute type in newer installation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: 3.8
Hardware: All All
: P2 critical (vote)
Assignee: Owen Leonard
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-24 15:10 UTC by Owen Leonard
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8002 - Can't add patron attribute type in newer installation (1.89 KB, patch)
2012-04-24 15:26 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 8002 [REVISED] Can't add patron attribute type in newer installation (3.07 KB, patch)
2012-04-24 15:47 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 8002 [SIGNED-OFF][REVISED] Can't add patron attribute type in newer installation (3.17 KB, patch)
2012-04-27 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8002 follow-up to use 3.09 and MODIFY instead of 3.08 and CHANGE (1.70 KB, patch)
2012-04-30 12:36 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8002: FIX Change datatype from varchar 1 to 10 for borrower_attribute_types.category_code (2.25 KB, patch)
2012-09-06 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] FIX Change datatype from varchar 1 to 10 (2.63 KB, patch)
2012-09-06 14:28 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 8002: FIX Change datatype from varchar 1 to 10 for borrower_attribute_types.category_code (2.49 KB, patch)
2012-09-07 07:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
[3.8.X] Bug 8002: FIX Change datatype from varchar 1 to 10 for borrower_attribute_types.category_code (2.49 KB, patch)
2012-09-07 07:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8002: FIX Change datatype from varchar 1 to 10 for borrower_attribute_types.category_code (2.54 KB, patch)
2012-09-14 03:12 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2012-04-24 15:10:57 UTC
The patch for Bug 7154 (Group borrower attributes and link between patron category and attribute) contains this in updatedatabase.pl:

$dbh->do("ALTER TABLE borrower_attribute_types ADD COLUMN category_code VARCHAR(10) NULL DEFAULT NULL AFTER `display_checkout`");

...which allows borrower_attribute_types.category_code to be NULL.

However, the addition to kohastructure.sql includes:

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

...which requires that it /not/ be NULL. Users of new installations will find that they cannot add patron attribute types if they don't limit it to a patron category. After trying to add a record without a patron category limit the user will be told the operation was successful but the entry will not be saved. An error appears in the log:

patron-attr-types.pl: DBD::mysql::st execute failed: Column 'category_code' cannot be null at /home/oleonard/kohaclone/C4/Members/AttributeTypes.pm line 220
Comment 1 Owen Leonard 2012-04-24 15:26:03 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-04-24 15:47:58 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2012-04-27 08:46:00 UTC Comment hidden (obsolete)
Comment 4 Ian Walls 2012-04-27 20:16:56 UTC
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.
Comment 5 Paul Poulain 2012-04-30 12:36:58 UTC Comment hidden (obsolete)
Comment 6 Paul Poulain 2012-04-30 12:39:01 UTC
(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 ;-) )
Comment 7 Paul Poulain 2012-04-30 12:39:14 UTC
patch and follow-up pushed
Comment 8 Frédéric Demians 2012-09-06 11:07:39 UTC
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.
Comment 9 Paul Poulain 2012-09-06 11:31:25 UTC
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
Comment 10 Paul Poulain 2012-09-06 11:36:25 UTC
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 ;-)
Comment 11 Jonathan Druart 2012-09-06 14:13:07 UTC Comment hidden (obsolete)
Comment 12 Frédéric Demians 2012-09-06 14:23:14 UTC
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.
Comment 13 Frédéric Demians 2012-09-06 14:28:10 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2012-09-06 14:43:40 UTC
(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).
Comment 15 Frédéric Demians 2012-09-06 15:01:17 UTC
> 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 16 Frédéric Demians 2012-09-06 15:02:12 UTC
Comment on attachment 12012 [details] [review]
[SIGNED-OFF] FIX Change datatype from varchar 1 to 10

updatedatabase.pl must also be fixed.
Comment 17 Jonathan Druart 2012-09-06 15:03:52 UTC
Yes I have just seen that ! But why mysql breaks a constraint ??
Comment 18 Frédéric Demians 2012-09-06 15:50:27 UTC
> 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.
Comment 19 Jonathan Druart 2012-09-07 07:28:34 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2012-09-07 07:29:10 UTC
Created attachment 12034 [details] [review]
[3.8.X] Bug 8002: FIX Change datatype from varchar 1 to 10 for borrower_attribute_types.category_code
Comment 21 Jonathan Druart 2012-09-07 07:36:19 UTC
(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.
Comment 22 Paul Poulain 2012-09-07 07:42:44 UTC
(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 !
Comment 23 Chris Cormack 2012-09-14 03:12:51 UTC
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>
Comment 24 Mason James 2012-09-14 07:00:06 UTC
(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
Comment 25 Paul Poulain 2012-09-14 13:43:20 UTC
Patch pushed to master

Patch pushed on master branch directly, as the new/bug_8002 is old
Comment 26 Chris Cormack 2012-09-16 08:20:52 UTC
Pushed to 3.8.x, will be in 3.8.5