Bug 24759 - OpacRenewalBranch code should be a Koha::Item method
Summary: OpacRenewalBranch code should be a Koha::Item method
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
: 23165 (view as bug list)
Depends on:
Blocks: 24754
  Show dependency treegraph
 
Reported: 2020-02-28 13:08 UTC by Nick Clemens
Modified: 2020-05-18 13:46 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24759: Move OpacRenewalBranch code to Koha::Item (8.39 KB, patch)
2020-02-28 14:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24759: CleanUp OpacRenewalBranch values (6.70 KB, patch)
2020-02-28 14:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23165: Unit tests for AddRenewal (5.09 KB, patch)
2020-02-28 16:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24759: Rename renewalbranch to renewal_branchcode (7.41 KB, patch)
2020-03-02 17:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24759: Simplify tests (5.14 KB, patch)
2020-03-03 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24759: Test intranet at the same time (3.34 KB, patch)
2020-03-03 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24759: (follow-up) Fix tests (1.13 KB, patch)
2020-03-20 15:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24759: Move OpacRenewalBranch code to Koha::Item (8.45 KB, patch)
2020-03-20 15:11 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24759: CleanUp OpacRenewalBranch values (6.77 KB, patch)
2020-03-20 15:11 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 23165: Unit tests for AddRenewal (5.15 KB, patch)
2020-03-20 15:11 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24759: Rename renewalbranch to renewal_branchcode (7.47 KB, patch)
2020-03-20 15:11 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24759: Simplify tests (5.21 KB, patch)
2020-03-20 15:11 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24759: Test intranet at the same time (3.41 KB, patch)
2020-03-20 15:11 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24759: (follow-up) Fix tests (1.19 KB, patch)
2020-03-20 15:11 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24759: Move OpacRenewalBranch code to Koha::Item (8.51 KB, patch)
2020-03-20 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24759: CleanUp OpacRenewalBranch values (6.80 KB, patch)
2020-03-20 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23165: Unit tests for AddRenewal (5.22 KB, patch)
2020-03-20 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24759: Rename renewalbranch to renewal_branchcode (7.55 KB, patch)
2020-03-20 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24759: Simplify tests (5.27 KB, patch)
2020-03-20 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24759: Test intranet at the same time (3.48 KB, patch)
2020-03-20 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24759: (follow-up) Fix tests (1.27 KB, patch)
2020-03-20 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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