Bug 10636

Summary: patronimage should have borrowernumber as PK, not cardnumber
Product: Koha Reporter: Galen Charlton <gmcharlt>
Component: DatabaseAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: gitbot, kyle, srdjan
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14338
Change sponsored?: --- Patch complexity: ---
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 8798, 11784    
Attachments: Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber
Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber
Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber
Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber
Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber
Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber
[SIGNED-OFF] Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber

Description Galen Charlton 2013-07-24 00:30:50 UTC
The patronimage table currently has cardnumber as its primary key rather than borrowernumber.  patronimage.cardnumber in turn has a FK constraint referencing borrowers.cardnumber.

This is less than ideal for a couple reasons:

[1] DBIX::Class (see bug 8798) complains about it.  In particular, any code that loads Koha::Database would litter the Apache logs with the following:

DBIx::Class::Carp::__ANON__(): "might_have/has_one" must not be on columns with is_nullable set to true (Koha::Schema::Result::Borrower/cardnumber). This might indicate an incorrect use of those relationship helpers instead of belongs_to. at /usr/share/perl5/DBIx/Class/Relationship/HasOne.pm line 96

[2] Every other table that has an FK to borrowers uses borrowernumber as the referent.
Comment 1 Kyle M Hall 2013-08-29 17:39:32 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-08-29 17:50:17 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-08-29 17:52:30 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2013-09-03 01:22:12 UTC
DDL statements ( ALTER TABLE ) cannot be rolled back, and should not be part of transaction. In that respect the update transaction makes no sense, and should be removed. If some kind of failure rollback needs to be implemented, it has to be done in some other way.
Comment 5 Kyle M Hall 2013-09-03 12:10:43 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-09-03 12:11:16 UTC
Thanks for the info! Does this new patch have a more sensible database update?

Kyle

(In reply to Srdjan Jankovic from comment #4)
> DDL statements ( ALTER TABLE ) cannot be rolled back, and should not be part
> of transaction. In that respect the update transaction makes no sense, and
> should be removed. If some kind of failure rollback needs to be implemented,
> it has to be done in some other way.
Comment 7 Srdjan Jankovic 2013-09-03 23:43:36 UTC
No, this is as good as it can be. The only thing that I'm not sure of is whether you should reinstate AutoCommit and RaiseError straight after eval. I hate to admit that whenever I had a db update it was always simple and I started with copying one before, so I'm not familiar with AutoCommit and RaiseError policy in updatedatabase.pl.

In a way, because it is a single change within transaction, you can even get away with not changing AutoCommit at all (RaiseError is still in order).

If you are happy I'll proceed with sign-off, and we'll let QA team cast the final verdict.
Comment 8 Kyle M Hall 2013-09-04 11:21:23 UTC
That sounds good to me!

> If you are happy I'll proceed with sign-off, and we'll let QA team cast the
> final verdict.
Comment 9 Srdjan Jankovic 2013-09-05 05:13:44 UTC
1. I had problems with either borrowers changing card numbers, or being removed (have no idea what is possible, but the situation is real, I'm using a clients prod dump). In order to establish new PK there must be no nulls, so patronimage should be purged first, something on the lines of DELETE FROM patronimage wHERE NOT EXISTS(borrower).

2. The patch adds UNIQUE and then PK. I understand why UNIQUE, but that is maybe an overkill. So it should either be removed (as in not included in the statement), or dropped afterwards.

3. There should be an FK statement to replace dropped FK
Comment 10 Kyle M Hall 2013-09-05 13:45:34 UTC Comment hidden (obsolete)
Comment 11 Srdjan Jankovic 2013-09-10 00:09:01 UTC
Kyle, I'm afraid DELETE FROM patronimage WHERE cardnumber NOT IN ( SELECT cardnumber FROM borrowers ) is not good. Logically it is ok, however the number of borrowers can be so huge that it will use heaps of resources or even fail (there are some limits on the list size I believe, postgres is 2000, not sure about mysql). I think EXISTS is a better option.
Comment 12 I'm just a bot 2013-09-18 11:16:50 UTC
Applying: Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber
Using index info to reconstruct a base tree...
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
M	members/printinvoice.pl
Falling back to patching base and 3-way merge...
Auto-merging members/printinvoice.pl
CONFLICT (content): Merge conflict in members/printinvoice.pl
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Patch failed at 0001 Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 13 Kyle M Hall 2013-10-09 15:09:55 UTC Comment hidden (obsolete)
Comment 14 Srdjan Jankovic 2013-10-10 04:00:25 UTC
Created attachment 21931 [details] [review]
[SIGNED-OFF] Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Enable patronimages
4) Verify patron images are still displaying correctly
5) Test deleting a patron image
6) Test adding a patron image from moremember.pl
7) Test adding a patron image from tools/picture-upload.pl

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Comment 15 Galen Charlton 2013-10-14 21:23:56 UTC
I've pushed to this master, as it's a dependency for the successful inclusion of DBIx::Class.  Thanks, Kyle!