Description
Fridolin Somers
2020-10-01 12:12:37 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 I'm working on a unit test Created attachment 111058 [details] [review] Bug 26594: add Unit Test Run prove t/db_dependent/Patron/Borrower_Debarments.t 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 We should work on using Koha objects for debarments. And remove debarred from borrowers table. 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> 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> 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> 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> 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> I am not convince by this patch, we should not call a private method. Asking for a second QA POV. 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> 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> (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? Created attachment 113094 [details] [review] Bug 26594: (QA follow-up) Make _UpdateBorrowerDebarmentFlags a public method Pushed to master for 20.11, thanks to everybody involved! backported to 20.05.x for 20.05.06 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 |