Bug 26594

Summary: Patrons merge problem with restriction
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: PatronsAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: aleisha, gmcharlt, jonathan.druart, koha, kyle.m.hall, kyle, lucas
Version: Main   
Hardware: All   
OS: All   
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
Bug Depends on: 9302    
Bug Blocks:    
Attachments: Bug 26594: Fix patrons merge problem with restrictions
Bug 26594: add Unit Test
Bug 26594: Fix patrons merge problem with restrictions
Bug 26594: add Unit Test
Bug 26594: Fix patrons merge problem with restrictions
Bug 26594: Fix patrons merge problem with restrictions
Bug 26594: add Unit Test
Bug 26594: Fix patrons merge problem with restrictions
Bug 26594: add Unit Test
Bug 26594: Fix patrons merge problem with restrictions
Bug 26594: (QA follow-up) Make _UpdateBorrowerDebarmentFlags a public method

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