In a 3.14.X database I discover some kind of shift between columns and values. For instance, branchcode was saved in the B_phone field, etc. I tested in current master (3.17.00.27), but could *happily* not reproduce. The problem can date from before 3.14 though. Just saving for the record. If more people have this problem, it might be interesting to see if we can repair the damage ;)
UPDATE: This problem still occurs in 3.14.4 (at least). Does anyone know which patch addressed this problem, if any? Will send a 3.14.X patch.
Created attachment 32343 [details] [review] Bug 13084: Mixup of columns in deletedborrowers (3.14 version) I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. NOTE: The callers of this routine should use the return value to check, and not simply assume that it will be fine ;) Changing those places is out of the scope of this report. Test plan: Edit a borrower. Fill all fields if possible. Delete the borrower. Check that the table deletedborrower contains the data in the correct fields.
Created attachment 32344 [details] [review] Bug 13084: Mixup of columns in deletedborrowers (MASTER) I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. Test plan: Edit a borrower. Fill all fields if possible. Delete the borrower. Check that the table deletedborrower contains the data in the correct fields.
Original patch seems to apply on both versions, but for completeness I added them both. Signoff on master (at least) would be appreciated.
Where is the appropriate test to confirm the library function still works as before?
Created attachment 32378 [details] [review] Bug 13084 - Improve t/db_dependent/Members.t test coverage Added a MoveMemberToDeleted set of tests. TEST PLAN --------- 1) prove -v t/db_dependent/Members.t -- all should pass. 2) Run koha QA tests.
By the way, I only think the mix ups occur if your deletedborrowers and borrowers have the fields in different order. Mine were identical. I tested my tests on a plain master.
Created attachment 32379 [details] [review] Bug 13084 - Improve t/db_dependent/Members.t test coverage (3.14 Version) Added a MoveMemberToDeleted set of tests. TEST PLAN --------- 1) prove -v t/db_dependent/Members.t -- all should pass. 2) Run koha QA tests.
Created attachment 32380 [details] [review] [SIGNED OFF] Bug 13084: Mixup of columns in deletedborrowers (3.14 version) I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. NOTE: The callers of this routine should use the return value to check, and not simply assume that it will be fine ;) Changing those places is out of the scope of this report. TEST PLAN --------- 1) Alter the order of your deletedborrowers table. e.g. alter table deletedborrowers change column privacy privacy int(11) after city; 2) Apply the 3.14.x test patch. 3) prove -v t/db_dependent/Members.t -- Will fail on the new test. 4) Apply the 3.14.x code change patch. 5) prove -v t/db_dependent/Members.t -- Will succeed on the new test. 6) run koha qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 32381 [details] [review] [SIGNED OFF] Bug 13084: Mixup of columns in deletedborrowers (MASTER) I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. TEST PLAN --------- 1) Alter the order of your deletedborrowers table. e.g. alter table deletedborrowers change column privacy privacy int(11) after city; 2) Apply the 3.14.x test patch. 3) prove -v t/db_dependent/Members.t -- Will fail on the new test. 4) Apply the 3.14.x code change patch. 5) prove -v t/db_dependent/Members.t -- Will succeed on the new test. 6) run koha qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 32394 [details] [review] Bug 13084 - Improve t/db_dependent/Members.t test coverage Added a MoveMemberToDeleted set of tests. TEST PLAN --------- 1) prove -v t/db_dependent/Members.t -- all should pass. 2) Run koha QA tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 32395 [details] [review] Bug 13084: Mixup of columns in deletedborrowers (MASTER) I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. TEST PLAN --------- 1) Alter the order of your deletedborrowers table. e.g. alter table deletedborrowers change column privacy privacy int(11) after city; 2) Apply the 3.14.x test patch. 3) prove -v t/db_dependent/Members.t -- Will fail on the new test. 4) Apply the 3.14.x code change patch. 5) prove -v t/db_dependent/Members.t -- Will succeed on the new test. 6) run koha qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
(In reply to M. Tompsett from comment #7) > By the way, I only think the mix ups occur if your deletedborrowers and > borrowers have the fields in different order. Mine were identical. I tested > my tests on a plain master. I compared the order of my 3.14 tables and had two differences: | smsalertnumber | varchar(50) | YES | | NULL | | | privacy | int(11) | YES | | NULL | | In deletedborrowers they were in the middle, in borrowers at the end. I looked at kohastructure, but that is fine. I looked in updatedatabase, and it also does not explain this phenomenon..
Created attachment 32396 [details] [review] Bug 13084 - Improve t/db_dependent/Members.t test coverage (3.14 Version) Added a MoveMemberToDeleted set of tests. TEST PLAN --------- 1) prove -v t/db_dependent/Members.t -- all should pass. 2) Run koha QA tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 32397 [details] [review] Bug 13084: Mixup of columns in deletedborrowers (3.14 version) I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. NOTE: The callers of this routine should use the return value to check, and not simply assume that it will be fine ;) Changing those places is out of the scope of this report. TEST PLAN --------- 1) Alter the order of your deletedborrowers table. e.g. alter table deletedborrowers change column privacy privacy int(11) after city; 2) Apply the 3.14.x test patch. 3) prove -v t/db_dependent/Members.t -- Will fail on the new test. 4) Apply the 3.14.x code change patch. 5) prove -v t/db_dependent/Members.t -- Will succeed on the new test. 6) run koha qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 32490 [details] [review] Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage Added a MoveMemberToDeleted set of tests. TEST PLAN --------- 1) prove -v t/db_dependent/Members.t -- all should pass. 2) Run koha QA tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 32491 [details] [review] Bug 13084 [Master] - Mixup of columns in deletedborrowers I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. TEST PLAN --------- 1) Alter the order of your deletedborrowers table. e.g. alter table deletedborrowers change column privacy privacy int(11) after city; 2) Apply the 3.14.x test patch. 3) prove -v t/db_dependent/Members.t -- Will fail on the new test. 4) Apply the 3.14.x code change patch. 5) prove -v t/db_dependent/Members.t -- Will succeed on the new test. 6) run koha qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 32492 [details] [review] Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic
Created attachment 32493 [details] [review] [PASSED QA] Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage Added a MoveMemberToDeleted set of tests. TEST PLAN --------- 1) prove -v t/db_dependent/Members.t -- all should pass. 2) Run koha QA tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 32494 [details] [review] [PASSED QA] Bug 13084 [Master] - Mixup of columns in deletedborrowers I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. TEST PLAN --------- 1) Alter the order of your deletedborrowers table. e.g. alter table deletedborrowers change column privacy privacy int(11) after city; 2) Apply the 3.14.x test patch. 3) prove -v t/db_dependent/Members.t -- Will fail on the new test. 4) Apply the 3.14.x code change patch. 5) prove -v t/db_dependent/Members.t -- Will succeed on the new test. 6) run koha qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 32495 [details] [review] [PASSED QA] Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 32496 [details] [review] [PASSED QA] Bug 13084 [3.14.x] - Improve t/db_dependent/Members.t test coverage Added a MoveMemberToDeleted set of tests. TEST PLAN --------- 1) prove -v t/db_dependent/Members.t -- all should pass. 2) Run koha QA tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 32497 [details] [review] [PASSED QA] Bug 13084 [3.14.x]: Mixup of columns in deletedborrowers I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. NOTE: The callers of this routine should use the return value to check, and not simply assume that it will be fine ;) Changing those places is out of the scope of this report. TEST PLAN --------- 1) Alter the order of your deletedborrowers table. e.g. alter table deletedborrowers change column privacy privacy int(11) after city; 2) Apply the 3.14.x test patch. 3) prove -v t/db_dependent/Members.t -- Will fail on the new test. 4) Apply the 3.14.x code change patch. 5) prove -v t/db_dependent/Members.t -- Will succeed on the new test. 6) run koha qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
RM Note: My QA followup does change the implementation of MoveMemberToDeleted completely. This new implementation is much simpler and passes the provided unit tests. However, if you feel it should be re-qa'ed, please reset the status to signed off. This followup has not been included in the 3.14.x patch set, as 3.14 has a bug where unit tests involving DBIC code will fail.
Created attachment 32500 [details] [review] Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage Added a MoveMemberToDeleted set of tests. TEST PLAN --------- 1) prove -v t/db_dependent/Members.t -- all should pass. 2) Run koha QA tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 32501 [details] [review] Bug 13084 [Master] - Mixup of columns in deletedborrowers I could reproduce this problem in 3.14.4 where I wrote the patch. Fields shifted like branchcode moved to B_phone. Could not reproduce it for master. But this code change seems to improve things on master too. TEST PLAN --------- 1) Alter the order of your deletedborrowers table. e.g. alter table deletedborrowers change column privacy privacy int(11) after city; 2) Apply the 3.14.x test patch. 3) prove -v t/db_dependent/Members.t -- Will fail on the new test. 4) Apply the 3.14.x code change patch. 5) prove -v t/db_dependent/Members.t -- Will succeed on the new test. 6) run koha qa test tools. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 32502 [details] [review] Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Patches pushed to master. Thanks Marcel!
Comment on attachment 32502 [details] [review] Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic This patch breaks misc/cronjob/delete_patrons.pl on debian squeeze, and maybe other things.
RMaints, be care, don't backport this! Or at least without the last patch.