Bug 16504 - All borrower attribute values for a given code deleted if that attribute has branch limits
Summary: All borrower attribute values for a given code deleted if that attribute has ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 15163
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-11 22:13 UTC by Jesse Weaver
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

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


Attachments
Bug 16504: Add regression tests (2.37 KB, patch)
2016-05-13 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16504: (follow-up for bug 15163) Do not remove attributes of other patrons (1.35 KB, patch)
2016-05-13 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16504: Add regression tests (2.41 KB, patch)
2016-05-16 06:07 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16504: (follow-up for bug 15163) Do not remove attributes of other patrons (1.40 KB, patch)
2016-05-16 06:08 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16504: Add regression tests (2.51 KB, patch)
2016-05-31 12:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16504: (follow-up for bug 15163) Do not remove attributes of other patrons (1.50 KB, patch)
2016-05-31 12:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16504: [QA Follow-up] Make koha-qa happy with two newlines (1.04 KB, patch)
2016-05-31 12:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Weaver 2016-05-11 22:13:47 UTC
Bug 15163 introduced a regression for branch-limited patron attributes; when modifying a patron attribute with branch limits, all of the values for that attribute for every patron will be deleted due to a predecence mistake:

            WHERE b_branchcode = ? OR b_branchcode IS NULL
                AND borrowernumber = ?

is parsed as:

            WHERE b_branchcode = ? OR (b_branchcode IS NULL
                AND borrowernumber = ?)
Comment 1 Jonathan Druart 2016-05-13 14:00:43 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-05-13 14:00:47 UTC Comment hidden (obsolete)
Comment 3 Srdjan Jankovic 2016-05-16 06:07:39 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2016-05-16 06:08:53 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2016-05-31 12:24:50 UTC
Created attachment 51923 [details] [review]
Bug 16504: Add regression tests

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2016-05-31 12:24:56 UTC
Created attachment 51924 [details] [review]
Bug 16504: (follow-up for bug 15163) Do not remove attributes of other patrons

Simple patch for a silly error, this single line is going to fix a
critical bug.
If a patron attribute is limited to a library, all the values for that attributes
for every patrons will be deleted.

Test plan:
Create a patron attribute limited to a library
Set the the attribute for a patron
Set the the attribute for another patron
=> Without this patch applied, the attribute's value for the first
patron is deleted
=> With this patch applied, the 2 values exist in the DB after the
second edition

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2016-05-31 12:25:00 UTC
Created attachment 51925 [details] [review]
Bug 16504: [QA Follow-up] Make koha-qa happy with two newlines

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2016-05-31 12:27:03 UTC
Thanks for regression test.
Could have been little more explicit but is sufficient.
Comment 9 Jonathan Druart 2016-05-31 13:32:01 UTC
(In reply to Marcel de Rooy from comment #8)
> Thanks for regression test.
> Could have been little more explicit but is sufficient.

What do you mean by "more explicit"?
Comment 10 Marcel de Rooy 2016-05-31 13:41:27 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Marcel de Rooy from comment #8)
> > Thanks for regression test.
> > Could have been little more explicit but is sufficient.
> 
> What do you mean by "more explicit"?

It is not directly obvious what happens here.
And that you still have one record of a limited type for the other patron, that would be deleted without this patch.
No big problem
Comment 11 Brendan Gallagher 2016-06-03 06:35:56 UTC
Pushed to Master - Should be in the November 2016 release. Thanks!
Comment 12 Frédéric Demians 2016-06-15 07:33:33 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 13 Julian Maurice 2016-06-16 07:52:53 UTC
Patches pushed to 3.22.x, will be in 3.22.8