Bug 26594 - Patrons merge problem with restriction
Summary: Patrons merge problem with restriction
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 9302
Blocks:
  Show dependency treegraph
 
Reported: 2020-10-01 12:12 UTC by Fridolin Somers
Modified: 2021-06-14 21:31 UTC (History)
7 users (show)

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


Attachments
Bug 26594: Fix patrons merge problem with restrictions (1.67 KB, patch)
2020-10-01 12:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26594: add Unit Test (2.34 KB, patch)
2020-10-02 07:23 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26594: Fix patrons merge problem with restrictions (1.67 KB, patch)
2020-10-02 07:23 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 26594: add Unit Test (2.40 KB, patch)
2020-10-04 03:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 26594: Fix patrons merge problem with restrictions (1.71 KB, patch)
2020-10-04 03:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 26594: Fix patrons merge problem with restrictions (1.77 KB, patch)
2020-10-15 11:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26594: add Unit Test (2.46 KB, patch)
2020-10-15 11:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26594: Fix patrons merge problem with restrictions (1.77 KB, patch)
2020-10-15 11:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26594: add Unit Test (2.52 KB, patch)
2020-11-02 15:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26594: Fix patrons merge problem with restrictions (1.84 KB, patch)
2020-11-02 15:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26594: (QA follow-up) Make _UpdateBorrowerDebarmentFlags a public method (2.81 KB, patch)
2020-11-05 11:48 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 Fridolin Somers 2020-10-01 12:12:37 UTC
When merging a patron A with restriction(s) into an other patron B, the entries of borrower_debarments are OK but the value of borrowers.debarred for patron B need to be updated.
An effect of this strange storage of restrictions in both places.
Comment 1 Fridolin Somers 2020-10-01 12:20:03 UTC
Created attachment 111036 [details] [review]
Bug 26594: Fix patrons merge problem with restrictions

When merging a patron A with restriction(s) into an other patron B,
the entries of borrower_debarments are OK but the value of borrowers.debarred for patron B need to be updated.
An effect of this strange storage of restrictions in both places.

Test plan :
1) Create a patron A with a manual restriction with some date and
   comment
2) Create a patron B with no restriction
3) Merge patron A and B, keeping patron B
4) Look at patron B details page
=> Without patch you see no restriction
=> With patch you see the restriction from patron A
Comment 2 Fridolin Somers 2020-10-01 12:21:55 UTC
I'm working on a unit test
Comment 3 Fridolin Somers 2020-10-02 07:23:39 UTC
Created attachment 111058 [details] [review]
Bug 26594: add Unit Test

Run prove t/db_dependent/Patron/Borrower_Debarments.t
Comment 4 Fridolin Somers 2020-10-02 07:23:56 UTC
Created attachment 111059 [details] [review]
Bug 26594: Fix patrons merge problem with restrictions

When merging a patron A with restriction(s) into an other patron B,
the entries of borrower_debarments are OK but the value of borrowers.debarred for patron B need to be updated.
An effect of this strange storage of restrictions in both places.

Test plan :
1) Create a patron A with a manual restriction with some date and
   comment
2) Create a patron B with no restriction
3) Merge patron A and B, keeping patron B
4) Look at patron B details page
=> Without patch you see no restriction
=> With patch you see the restriction from patron A
Comment 5 Fridolin Somers 2020-10-02 07:33:34 UTC
We should work on using Koha objects for debarments.
And remove debarred from borrowers table.
Comment 6 David Nind 2020-10-04 03:13:31 UTC
Created attachment 111197 [details] [review]
Bug 26594: add Unit Test

Run prove t/db_dependent/Patron/Borrower_Debarments.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2020-10-04 03:13:35 UTC
Created attachment 111198 [details] [review]
Bug 26594: Fix patrons merge problem with restrictions

When merging a patron A with restriction(s) into an other patron B,
the entries of borrower_debarments are OK but the value of borrowers.debarred for patron B need to be updated.
An effect of this strange storage of restrictions in both places.

Test plan :
1) Create a patron A with a manual restriction with some date and
   comment
2) Create a patron B with no restriction
3) Merge patron A and B, keeping patron B
4) Look at patron B details page
=> Without patch you see no restriction
=> With patch you see the restriction from patron A

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Katrin Fischer 2020-10-15 11:06:20 UTC
Created attachment 111702 [details] [review]
Bug 26594: Fix patrons merge problem with restrictions

When merging a patron A with restriction(s) into an other patron B,
the entries of borrower_debarments are OK but the value of borrowers.debarred for patron B need to be updated.
An effect of this strange storage of restrictions in both places.

Test plan :
1) Create a patron A with a manual restriction with some date and
   comment
2) Create a patron B with no restriction
3) Merge patron A and B, keeping patron B
4) Look at patron B details page
=> Without patch you see no restriction
=> With patch you see the restriction from patron A

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2020-10-15 11:09:42 UTC
Created attachment 111703 [details] [review]
Bug 26594: add Unit Test

Run prove t/db_dependent/Patron/Borrower_Debarments.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2020-10-15 11:09:48 UTC
Created attachment 111704 [details] [review]
Bug 26594: Fix patrons merge problem with restrictions

When merging a patron A with restriction(s) into an other patron B,
the entries of borrower_debarments are OK but the value of borrowers.debarred for patron B need to be updated.
An effect of this strange storage of restrictions in both places.

Test plan :
1) Create a patron A with a manual restriction with some date and
   comment
2) Create a patron B with no restriction
3) Merge patron A and B, keeping patron B
4) Look at patron B details page
=> Without patch you see no restriction
=> With patch you see the restriction from patron A

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Jonathan Druart 2020-10-25 22:18:12 UTC
I am not convince by this patch, we should not call a private method.
Asking for a second QA POV.
Comment 12 Kyle M Hall 2020-11-02 15:44:08 UTC
Created attachment 112849 [details] [review]
Bug 26594: add Unit Test

Run prove t/db_dependent/Patron/Borrower_Debarments.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2020-11-02 15:44:17 UTC
Created attachment 112850 [details] [review]
Bug 26594: Fix patrons merge problem with restrictions

When merging a patron A with restriction(s) into an other patron B,
the entries of borrower_debarments are OK but the value of borrowers.debarred for patron B need to be updated.
An effect of this strange storage of restrictions in both places.

Test plan :
1) Create a patron A with a manual restriction with some date and
   comment
2) Create a patron B with no restriction
3) Merge patron A and B, keeping patron B
4) Look at patron B details page
=> Without patch you see no restriction
=> With patch you see the restriction from patron A

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Jonathan Druart 2020-11-03 08:26:19 UTC
(In reply to Jonathan Druart from comment #11)
> I am not convince by this patch, we should not call a private method.
> Asking for a second QA POV.

Kyle, can you confirm that you see this comment before you PQAed?
Comment 15 Kyle M Hall 2020-11-05 11:48:23 UTC
Created attachment 113094 [details] [review]
Bug 26594: (QA follow-up) Make _UpdateBorrowerDebarmentFlags a public method
Comment 16 Jonathan Druart 2020-11-10 14:00:50 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 17 Lucas Gass 2020-11-16 22:37:42 UTC
backported to 20.05.x for 20.05.06
Comment 18 Aleisha Amohia 2020-11-17 05:42:40 UTC
this didn't work for me on 19.11.x, the tests passed cleanly but i still so no restrictions on patron B after merging patron A into patron B. not backported