Created attachment 99763 [details] [review] Bug 24759: Move OpacRenewalBranch code to Koha::Item This patchset moves all code to calculate the correct renewal branch into Koha::Item.pm When interface is opac we follow the syspref, otherwise we use the current userenv, or pass through a defined branch To test: 1 - Check out an item to a patron 2 - Set allowed renewals in the circ rules to 100 (just so you can keep testing) 3 - Renew the item in staff interface, confirm it is recorded correctly in statistics table (as signed in branch) 4 - Renew via the opac, testing with each setting of OpacRenewalbranch 5 - prove -v t/db_dependent/Koha/Item.t
Created attachment 99764 [details] [review] Bug 24759: CleanUp OpacRenewalBranch values We had a unique behvaiour where the syspref was set to string 'NULL' as opposed to undef, we need to clean that up To test: 1 - Set OpacRenewalBranch to 'NULL' in staff interface 2 - Renew via opac 3 - Check statistics to ensure branch is blank
This uncovered some issues, previously if you chose 'NULL' in the system preferences for OpacRenewalBranch it saved as "" - and the test for null looked for the string 'NULL' - so setting it to null was like setting it to 'opacrenew', having it undef was the same. Only having it set to the string value 'NULL' gave you a blank string in statistics (which, I must point out, is not the same a NULL) The second patch here makes the NULL option 'none' and cleans up old pref values - it shouldn't change behaviour, but may change pref value
Created attachment 99780 [details] [review] Bug 23165: Unit tests for AddRenewal
*** Bug 23165 has been marked as a duplicate of this bug. ***
renewalbranch is not a correct name. renewal_branchcode is more appropriate here to avoid confusion. Even better, for consistency, I would prefer renewal_library and make it returns a Koha::Library object (if if you need an additional fetch).
Created attachment 99953 [details] [review] Bug 24759: Rename renewalbranch to renewal_branchcode
(In reply to Jonathan Druart from comment #6) > renewalbranch is not a correct name. renewal_branchcode is more appropriate > here to avoid confusion. > Even better, for consistency, I would prefer renewal_library and make it > returns a Koha::Library object (if if you need an additional fetch). Renamed to renewal_branchcode - returning a library object doesn't work so well because we can have "" or "OPACRenew" as values, they won't have Library objects
Created attachment 100002 [details] [review] Bug 24759: Simplify tests
Created attachment 100003 [details] [review] Bug 24759: Test intranet at the same time
Created attachment 101238 [details] [review] Bug 24759: (follow-up) Fix tests Earlier tests delete borrowers before issues, can cause a constraint failure Wrong test count
Created attachment 101239 [details] [review] Bug 24759: Move OpacRenewalBranch code to Koha::Item This patchset moves all code to calculate the correct renewal branch into Koha::Item.pm When interface is opac we follow the syspref, otherwise we use the current userenv, or pass through a defined branch To test: 1 - Check out an item to a patron 2 - Set allowed renewals in the circ rules to 100 (just so you can keep testing) 3 - Renew the item in staff interface, confirm it is recorded correctly in statistics table (as signed in branch) 4 - Renew via the opac, testing with each setting of OpacRenewalbranch 5 - prove -v t/db_dependent/Koha/Item.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 101240 [details] [review] Bug 24759: CleanUp OpacRenewalBranch values We had a unique behvaiour where the syspref was set to string 'NULL' as opposed to undef, we need to clean that up To test: 1 - Set OpacRenewalBranch to 'NULL' in staff interface 2 - Renew via opac 3 - Check statistics to ensure branch is blank Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 101241 [details] [review] Bug 23165: Unit tests for AddRenewal https://bugs.koha-community.org/show_bug.cgi?id=24759 Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 101242 [details] [review] Bug 24759: Rename renewalbranch to renewal_branchcode Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 101243 [details] [review] Bug 24759: Simplify tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 101244 [details] [review] Bug 24759: Test intranet at the same time Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 101245 [details] [review] Bug 24759: (follow-up) Fix tests Earlier tests delete borrowers before issues, can cause a constraint failure Wrong test count Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 101285 [details] [review] Bug 24759: Move OpacRenewalBranch code to Koha::Item This patchset moves all code to calculate the correct renewal branch into Koha::Item.pm When interface is opac we follow the syspref, otherwise we use the current userenv, or pass through a defined branch To test: 1 - Check out an item to a patron 2 - Set allowed renewals in the circ rules to 100 (just so you can keep testing) 3 - Renew the item in staff interface, confirm it is recorded correctly in statistics table (as signed in branch) 4 - Renew via the opac, testing with each setting of OpacRenewalbranch 5 - prove -v t/db_dependent/Koha/Item.t Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 101286 [details] [review] Bug 24759: CleanUp OpacRenewalBranch values We had a unique behvaiour where the syspref was set to string 'NULL' as opposed to undef, we need to clean that up To test: 1 - Set OpacRenewalBranch to 'NULL' in staff interface 2 - Renew via opac 3 - Check statistics to ensure branch is blank Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 101287 [details] [review] Bug 23165: Unit tests for AddRenewal https://bugs.koha-community.org/show_bug.cgi?id=24759 Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 101288 [details] [review] Bug 24759: Rename renewalbranch to renewal_branchcode Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 101289 [details] [review] Bug 24759: Simplify tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 101290 [details] [review] Bug 24759: Test intranet at the same time Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 101291 [details] [review] Bug 24759: (follow-up) Fix tests Earlier tests delete borrowers before issues, can cause a constraint failure Wrong test count Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x