|
Description
Galen Charlton
2013-07-24 00:30:50 UTC
Created attachment 20722 [details] [review] 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 Created attachment 20723 [details] [review] 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 Created attachment 20724 [details] [review] 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 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. Created attachment 20765 [details] [review] 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 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. 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. 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.
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 Created attachment 20801 [details] [review] 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 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. 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". Created attachment 21923 [details] [review] 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 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> I've pushed to this master, as it's a dependency for the successful inclusion of DBIx::Class. Thanks, Kyle! |