Bug 3777 - Field Sizes Inconsistent
Summary: Field Sizes Inconsistent
Status: REOPENED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: PC All
: P5 - low trivial (vote)
Assignee: Galen Charlton
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-06 05:22 UTC by Nicole C. Engard
Modified: 2018-02-19 07:09 UTC (History)
3 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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:14:52 UTC


---- Reported by nengard@gmail.com 2009-11-06 17:22:12 ----

When looking at the borrowers table the field sizes are inconsistent.  All address fields should be the same and all city/state fields should be the same, etc etc.

`address` mediumtext NOT NULL,
  `address2` text,
  `city` mediumtext NOT NULL,

`B_address` varchar(100) default NULL,
  `B_address2` text default NULL,
  `B_city` mediumtext,


`altcontactaddress1` varchar(255) default NULL,
  `altcontactaddress2` varchar(255) default NULL,
  `altcontactaddress3` varchar(255) default NULL,

I recommend mediumtext for them all.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:14 UTC  ---

This bug was previously known as _bug_ 3777 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3777

Actual time not defined. Setting to 0.0

Comment 1 Katrin Fischer 2010-11-25 22:35:33 UTC
I am willing to work on this, but need some help deciding about the right data types.

This lists a lot of fields from the borrowers/deletedborrwoers tables, sorted and with suggested changes. The line numbers refer to kohastructure.sql in git.  

(http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=installer/data/mysql/kohastructure.sql;h=9148970ca8a7a44d2846c4c182d05eb460a3df18;hb=HEAD#l250)

SUGGESTION: text?
 199   `surname` mediumtext NOT NULL,
 251   `altcontactsurname` varchar(255) default NULL,
 234   `contactname` mediumtext,

SUGGESTION: text? default null?
 250   `altcontactfirstname` varchar(255) default NULL,
 200   `firstname` text,
 235   `contactfirstname` text,

SUGGESTION: text?
 201   `title` mediumtext,
 236   `contacttitle` text,

 202   `othernames` mediumtext,
 203   `initials` text,

OK - altcontact is missing streetnumber
 204   `streetnumber` varchar(10) default NULL,
 217   `B_streetnumber` varchar(10) default NULL,

OK - altcontact is missing streettype
 205   `streettype` varchar(50) default NULL,
 218   `B_streettype` varchar(50) default NULL,

SUGGESTION: text?
 206   `address` mediumtext NOT NULL,
 219   `B_address` varchar(100) default NULL,
 252   `altcontactaddress1` varchar(255) default NULL,

SUGGESTION: text?
 207   `address2` text,
 220   `B_address2` text default NULL,
 253   `altcontactaddress2` varchar(255) default NULL,

SUGGESTION: text?
 208   `city` mediumtext NOT NULL,
 221   `B_city` mediumtext,
 254   `altcontactaddress3` varchar(255) default NULL,

SUGGESTION: varchar(50)?
 209   `zipcode` varchar(25) default NULL,
 222   `B_zipcode` varchar(25) default NULL,
 255   `altcontactzipcode` varchar(50) default NULL,

SUGGESTION: default null?
 210   `country` text,
 223   `B_country` text,
 256   `altcontactcountry` text default NULL,

SUGGESTION: text? altcontact has no email field.
 211   `email` mediumtext,
 215   `emailpro` text,
 224   `B_email` text,

SUGGESTION: text, default null?
 212   `phone` text,
 216   `phonepro` text,
 213   `mobile` varchar(50) default NULL,
 214   `fax` mediumtext,
 225   `B_phone` mediumtext,
 257   `altcontactphone` varchar(50) default NULL,

SUGGESTION: text, default null?
 238   `borrowernotes` mediumtext,
 246   `opacnote` mediumtext,
 247   `contactnote` varchar(255) default NULL, 
 241   `ethnotes` varchar(255) default NULL,
Comment 2 Nicole C. Engard 2010-11-29 14:59:12 UTC
Cait I agree with your suggestions - and do think we need to add in the missing fields while we're at it.
Comment 3 Katrin Fischer 2010-11-29 15:04:57 UTC
Hi Nicole, thx for your feedback :) I agree - we should complete the table and add the missing fields, but perhaps better split that into separate bugs? 
Because the other changes will require changing the templates and some scripts.
Comment 4 Brendan Gallagher 2010-11-30 06:12:35 UTC
Making the userid field longer couldn't hurt.  IRC it's something like 30 characters now.  Some users may wish to use their email address as userid.  So something like 50 characters would work.
Comment 5 Katrin Fischer 2010-11-30 21:17:10 UTC
Started work on database changes, will all suggested changes made table borrowers will look like this:

DROP TABLE IF EXISTS `borrowers`;
CREATE TABLE `borrowers` (
  `borrowernumber` int(11) NOT NULL auto_increment,
  `cardnumber` varchar(16) default NULL,
  `surname` text default NULL,
  `firstname` text default NULL,
  `title` text default NULL,
  `othernames` text default NULL,
  `initials` text default NULL,
  `streetnumber` varchar(10) default NULL,
  `streettype` varchar(50) default NULL,
  `address` text default NULL,
  `address2` text default NULL,
  `city` text default NULL,
  `zipcode` varchar(50) default NULL,
  `country` text default NULL,
  `email` text default NULL,
  `phone` text default NULL,
  `mobile` text default NULL,
  `fax` text default NULL,
  `emailpro` text default NULL,
  `phonepro` text default NULL,
  `B_streetnumber` varchar(10) default NULL,
  `B_streettype` varchar(50) default NULL,
  `B_address` text default NULL,
  `B_address2` text default NULL,
  `B_city` text default NULL,
  `B_zipcode` varchar(50) default NULL,
  `B_country` text default NULL,
  `B_email` text default NULL,
  `B_phone` text default NULL,
  `dateofbirth` date default NULL,
  `branchcode` varchar(10) NOT NULL default '',
  `categorycode` varchar(10) NOT NULL default '',
  `dateenrolled` date default NULL,
  `dateexpiry` date default NULL,
  `gonenoaddress` tinyint(1) default NULL,
  `lost` tinyint(1) default NULL,
  `debarred` tinyint(1) default NULL,
  `contactname` text default NULL,
  `contactfirstname` text default NULL,
  `contacttitle` text default NULL,
  `guarantorid` int(11) default NULL,
  `borrowernotes` text default NULL,
  `relationship` varchar(100) default NULL,
  `ethnicity` varchar(50) default NULL,
  `ethnotes` text default NULL,
  `sex` varchar(1) default NULL,
  `password` varchar(30) default NULL,
  `flags` int(11) default NULL,
  `userid` varchar(50) default NULL,
  `opacnote` text default NULL,
  `contactnote` text default NULL,
  `sort1` varchar(80) default NULL,
  `sort2` varchar(80) default NULL,
  `altcontactfirstname` text default NULL,
  `altcontactsurname` text default NULL,
  `altcontactaddress1` text default NULL,
  `altcontactaddress2` text default NULL,
  `altcontactaddress3` text default NULL,
  `altcontactzipcode` varchar(50) default NULL,
  `altcontactcountry` text default NULL,
  `altcontactphone` text default NULL,
  `smsalertnumber` text default NULL,
  `privacy` integer(11) DEFAULT '1' NOT NULL,
  UNIQUE KEY `cardnumber` (`cardnumber`),
  PRIMARY KEY `borrowernumber` (`borrowernumber`),
  KEY `categorycode` (`categorycode`),
  KEY `branchcode` (`branchcode`),
  KEY `userid` (`userid`),
  KEY `guarantorid` (`guarantorid`),
  CONSTRAINT `borrowers_ibfk_1` FOREIGN KEY (`categorycode`) REFERENCES `categories` (`categorycode`),
  CONSTRAINT `borrowers_ibfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

Please let me know if you see any problem with that or have suggestions for further improvement.
Comment 6 Katrin Fischer 2010-12-03 07:27:28 UTC
Learned something new about MySQL:
BLOB and  TEXT columns cannot have DEFAULT values.  

Reworking suggested table structure.
Comment 7 Katrin Fischer 2012-04-07 17:14:54 UTC
Resetting this to default assignee. I am not sure it's something we want to do - perhaps should be part of a bigger code cleanup and needs some guidelines.
Comment 8 Katrin Fischer 2013-01-28 13:03:47 UTC
Not sure this is still something that is wanted - closing bug, please feel free to reopen.
Comment 9 Nicole C. Engard 2013-01-28 13:20:55 UTC
Yes, this is still wanted and needed - addresses can be more than 100 characters in length and we need to be consistent in this table about the size of these fields regardless of it being the primary address or the alternate address.

Nicole
Comment 10 Katrin Fischer 2013-01-28 14:49:31 UTC
Maybe it would be easier to have separate bugs then for fields that need to be increased in size?
Comment 11 Nicole C. Engard 2013-01-28 14:51:26 UTC
Well originally I was just reporting the address fields.  


`address` mediumtext NOT NULL,
  `address2` text,
  `city` mediumtext NOT NULL,

`B_address` varchar(100) default NULL,
  `B_address2` text default NULL,
  `B_city` mediumtext,


`altcontactaddress1` varchar(255) default NULL,
  `altcontactaddress2` varchar(255) default NULL,
  `altcontactaddress3` varchar(255) default NULL,


So the others can be in a different bug report.
Comment 12 Jon Knight 2017-11-22 10:13:41 UTC
I'm just trawling through old REOPENed cases and I see that the current (17.05) borrower schema still has the inconsistencies in the addresses.  As this was reopened because it was proving to be an issue for some addresses longer than 100 characters, is it worth just attacking just those fields to make them all match the address versions?

In other words make these field changes in the borrowers table schema:

B_address mediumtext NOT NULL,
B_address2 text,
B_city mediumtext NOT NULL,

and

altcontactaddress1 mediumtext NOT NULL,
altcontactaddress2 text,
altcontactaddress3 mediumtext NOT NULL,
Comment 13 Katrin Fischer 2018-02-19 07:09:13 UTC
(In reply to Jon Knight from comment #12)
> I'm just trawling through old REOPENed cases and I see that the current
> (17.05) borrower schema still has the inconsistencies in the addresses.  As
> this was reopened because it was proving to be an issue for some addresses
> longer than 100 characters, is it worth just attacking just those fields to
> make them all match the address versions?
> 
> In other words make these field changes in the borrowers table schema:
> 
> B_address mediumtext NOT NULL,
> B_address2 text,
> B_city mediumtext NOT NULL,
> 
> and
> 
> altcontactaddress1 mediumtext NOT NULL,
> altcontactaddress2 text,
> altcontactaddress3 mediumtext NOT NULL,

I once tried to start a clean-up, but ended very confused. In your eample I think there is probably no reason that ...address2 should not be mediumtext not null too?
One thing I encountered back then was that some types can't be using not null (iirc).