[Bug 10636] New: patronimage should have borrowernumber as PK, not cardnumber
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Bug ID: 10636 Summary: patronimage should have borrowernumber as PK, not cardnumber Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Database Assignee: gmcharlt@gmail.com Reporter: gmcharlt@gmail.com QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |8798 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20722 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20722&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20722|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20723 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20723&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20723|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20724 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20724&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |srdjan@catalyst.net.nz --- Comment #4 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20724|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20765 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20765&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- 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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 --- Comment #7 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- 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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 --- Comment #9 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20765|0 |1 is obsolete| | --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20801 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20801&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 --- Comment #11 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 I'm just a bot <gitbot@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |gitbot@bugs.koha-community. | |org --- Comment #12 from I'm just a bot <gitbot@bugs.koha-community.org> --- 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". -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20801|0 |1 is obsolete| | --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 21923 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21923&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #21923|0 |1 is obsolete| | --- Comment #14 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Created attachment 21931 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21931&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #15 from Galen Charlton <gmcharlt@gmail.com> --- I've pushed to this master, as it's a dependency for the successful inclusion of DBIx::Class. Thanks, Kyle! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |11784 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10636 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14338 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org