Bug 24759

Summary: OpacRenewalBranch code should be a Koha::Item method
Product: Koha Reporter: Nick Clemens <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, gmcharlt, jonathan.druart, kyle.m.hall, kyle, martin.renvoize
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 24754    
Attachments: Bug 24759: Move OpacRenewalBranch code to Koha::Item
Bug 24759: CleanUp OpacRenewalBranch values
Bug 23165: Unit tests for AddRenewal
Bug 24759: Rename renewalbranch to renewal_branchcode
Bug 24759: Simplify tests
Bug 24759: Test intranet at the same time
Bug 24759: (follow-up) Fix tests
Bug 24759: Move OpacRenewalBranch code to Koha::Item
Bug 24759: CleanUp OpacRenewalBranch values
Bug 23165: Unit tests for AddRenewal
Bug 24759: Rename renewalbranch to renewal_branchcode
Bug 24759: Simplify tests
Bug 24759: Test intranet at the same time
Bug 24759: (follow-up) Fix tests
Bug 24759: Move OpacRenewalBranch code to Koha::Item
Bug 24759: CleanUp OpacRenewalBranch values
Bug 23165: Unit tests for AddRenewal
Bug 24759: Rename renewalbranch to renewal_branchcode
Bug 24759: Simplify tests
Bug 24759: Test intranet at the same time
Bug 24759: (follow-up) Fix tests

Description Nick Clemens 2020-02-28 13:08:50 UTC

    
Comment 1 Nick Clemens 2020-02-28 14:35:33 UTC
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
Comment 2 Nick Clemens 2020-02-28 14:35:35 UTC
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
Comment 3 Nick Clemens 2020-02-28 14:38:54 UTC
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
Comment 4 Nick Clemens 2020-02-28 16:29:04 UTC
Created attachment 99780 [details] [review]
Bug 23165: Unit tests for AddRenewal
Comment 5 Nick Clemens 2020-02-28 16:30:23 UTC
*** Bug 23165 has been marked as a duplicate of this bug. ***
Comment 6 Jonathan Druart 2020-03-02 14:20:06 UTC
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).
Comment 7 Nick Clemens 2020-03-02 17:03:10 UTC
Created attachment 99953 [details] [review]
Bug 24759: Rename renewalbranch to renewal_branchcode
Comment 8 Nick Clemens 2020-03-02 17:04:09 UTC
(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
Comment 9 Jonathan Druart 2020-03-03 13:28:42 UTC
Created attachment 100002 [details] [review]
Bug 24759: Simplify tests
Comment 10 Jonathan Druart 2020-03-03 13:28:45 UTC
Created attachment 100003 [details] [review]
Bug 24759: Test intranet at the same time
Comment 11 Nick Clemens 2020-03-20 15:08:13 UTC
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
Comment 12 Andrew Fuerste-Henry 2020-03-20 15:11:26 UTC
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>
Comment 13 Andrew Fuerste-Henry 2020-03-20 15:11:29 UTC
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>
Comment 14 Andrew Fuerste-Henry 2020-03-20 15:11:32 UTC
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>
Comment 15 Andrew Fuerste-Henry 2020-03-20 15:11:35 UTC
Created attachment 101242 [details] [review]
Bug 24759: Rename renewalbranch to renewal_branchcode

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 16 Andrew Fuerste-Henry 2020-03-20 15:11:38 UTC
Created attachment 101243 [details] [review]
Bug 24759: Simplify tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 17 Andrew Fuerste-Henry 2020-03-20 15:11:41 UTC
Created attachment 101244 [details] [review]
Bug 24759: Test intranet at the same time

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 18 Andrew Fuerste-Henry 2020-03-20 15:11:44 UTC
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>
Comment 19 Jonathan Druart 2020-03-20 15:21:01 UTC
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>
Comment 20 Jonathan Druart 2020-03-20 15:21:04 UTC
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>
Comment 21 Jonathan Druart 2020-03-20 15:21:08 UTC
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>
Comment 22 Jonathan Druart 2020-03-20 15:21:11 UTC
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>
Comment 23 Jonathan Druart 2020-03-20 15:21:15 UTC
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>
Comment 24 Jonathan Druart 2020-03-20 15:21:19 UTC
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>
Comment 25 Jonathan Druart 2020-03-20 15:21:22 UTC
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>
Comment 26 Martin Renvoize 2020-03-24 11:05:22 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Joy Nelson 2020-04-03 21:38:01 UTC
enhancement not backported to 19.11.x