Bug 13084 - Prevent mixup of columns in deletedborrowers
Summary: Prevent mixup of columns in deletedborrowers
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 13610
  Show dependency treegraph
 
Reported: 2014-10-15 12:23 UTC by Marcel de Rooy
Modified: 2016-06-21 21:36 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13084: Mixup of columns in deletedborrowers (3.14 version) (2.53 KB, patch)
2014-10-15 14:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13084: Mixup of columns in deletedborrowers (MASTER) (2.35 KB, patch)
2014-10-15 14:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13084 - Improve t/db_dependent/Members.t test coverage (1.46 KB, patch)
2014-10-15 22:20 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 13084 - Improve t/db_dependent/Members.t test coverage (3.14 Version) (1.66 KB, patch)
2014-10-15 22:38 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13084: Mixup of columns in deletedborrowers (3.14 version) (2.78 KB, patch)
2014-10-15 22:47 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13084: Mixup of columns in deletedborrowers (MASTER) (2.61 KB, patch)
2014-10-15 22:51 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 13084 - Improve t/db_dependent/Members.t test coverage (1.56 KB, patch)
2014-10-16 06:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13084: Mixup of columns in deletedborrowers (MASTER) (2.64 KB, patch)
2014-10-16 06:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13084 - Improve t/db_dependent/Members.t test coverage (3.14 Version) (1.76 KB, patch)
2014-10-16 07:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13084: Mixup of columns in deletedborrowers (3.14 version) (2.82 KB, patch)
2014-10-16 07:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage (1.53 KB, patch)
2014-10-17 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13084 [Master] - Mixup of columns in deletedborrowers (2.60 KB, patch)
2014-10-17 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic (1.79 KB, patch)
2014-10-17 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage (1.60 KB, patch)
2014-10-17 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13084 [Master] - Mixup of columns in deletedborrowers (2.67 KB, patch)
2014-10-17 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic (1.86 KB, patch)
2014-10-17 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13084 [3.14.x] - Improve t/db_dependent/Members.t test coverage (1.78 KB, patch)
2014-10-17 13:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13084 [3.14.x]: Mixup of columns in deletedborrowers (2.84 KB, patch)
2014-10-17 13:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13084 [Master] - Improve t/db_dependent/Members.t test coverage (1.58 KB, patch)
2014-10-17 14:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13084 [Master] - Mixup of columns in deletedborrowers (2.66 KB, patch)
2014-10-17 14:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13084 [Master] - QA Followup - Use DBIx::Class to simplify logic (1.88 KB, patch)
2014-10-17 14:12 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 2014-10-17 13:11:24 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-10-17 13:11:33 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2014-10-17 13:11:36 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2014-10-17 13:12:32 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2014-10-17 13:12:41 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2014-10-17 13:12:43 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 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 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 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 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 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 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 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.