Bug 13084

Summary: Prevent mixup of columns in deletedborrowers
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: PatronsAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: gmcharlt, jonathan.druart, kyle.m.hall, kyle, m.de.rooy, mtompset, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 13610    
Attachments: Bug 13084: Mixup of columns in deletedborrowers (3.14 version)
Bug 13084: Mixup of columns in deletedborrowers (MASTER)
Bug 13084 - Improve t/db_dependent/Members.t test coverage
Bug 13084 - Improve t/db_dependent/Members.t test coverage (3.14 Version)
[SIGNED OFF] Bug 13084: Mixup of columns in deletedborrowers (3.14 version)
[SIGNED OFF] Bug 13084: Mixup of columns in deletedborrowers (MASTER)
Bug 13084 - Improve t/db_dependent/Members.t test coverage
Bug 13084: Mixup of columns in deletedborrowers (MASTER)
Bug 13084 - Improve t/db_dependent/Members.t test coverage (3.14 Version)
Bug 13084: Mixup of columns in deletedborrowers (3.14 version)
Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage
Bug 13084 [Master] - Mixup of columns in deletedborrowers
Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic
[PASSED QA] Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage
[PASSED QA] Bug 13084 [Master] - Mixup of columns in deletedborrowers
[PASSED QA] Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic
[PASSED QA] Bug 13084 [3.14.x] - Improve t/db_dependent/Members.t test coverage
[PASSED QA] Bug 13084 [3.14.x]: Mixup of columns in deletedborrowers
Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage
Bug 13084 [Master] - Mixup of columns in deletedborrowers
Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic

Description Marcel de Rooy 2014-10-15 12:23:03 UTC
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 ;)
Comment 1 Marcel de Rooy 2014-10-15 13:52:22 UTC
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.
Comment 2 Marcel de Rooy 2014-10-15 14:45:51 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2014-10-15 14:49:48 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2014-10-15 14:51:51 UTC
Original patch seems to apply on both versions, but for completeness I added them both.
Signoff on master (at least) would be appreciated.
Comment 5 Mark Tompsett 2014-10-15 21:54:57 UTC
Where is the appropriate test to confirm the library function still works as before?
Comment 6 Mark Tompsett 2014-10-15 22:20:29 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2014-10-15 22:22:41 UTC
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.
Comment 8 Mark Tompsett 2014-10-15 22:38:15 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2014-10-15 22:47:01 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2014-10-15 22:51:23 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2014-10-16 06:59:27 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2014-10-16 06:59:51 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2014-10-16 07:01:21 UTC
(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..
Comment 14 Marcel de Rooy 2014-10-16 07:04:17 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2014-10-16 07:05:13 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall (khall) 2014-10-17 13:11:24 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall (khall) 2014-10-17 13:11:33 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall (khall) 2014-10-17 13:11:36 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall (khall) 2014-10-17 13:12:32 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall (khall) 2014-10-17 13:12:41 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall (khall) 2014-10-17 13:12:43 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall (khall) 2014-10-17 13:21:50 UTC
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>
Comment 23 Kyle M Hall (khall) 2014-10-17 13:22:00 UTC
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>
Comment 24 Kyle M Hall (khall) 2014-10-17 14:09:53 UTC
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.
Comment 25 Kyle M Hall (khall) 2014-10-17 14:12:12 UTC
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>
Comment 26 Kyle M Hall (khall) 2014-10-17 14:12:19 UTC
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>
Comment 27 Kyle M Hall (khall) 2014-10-17 14:12:21 UTC
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>
Comment 28 Tomás Cohen Arazi (tcohen) 2014-10-23 19:05:06 UTC
Patches pushed to master.

Thanks Marcel!
Comment 29 Jonathan Druart 2015-01-22 12:17:37 UTC
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.
Comment 30 Jonathan Druart 2015-01-22 12:30:41 UTC
RMaints, be care, don't backport this!
Or at least without the last patch.