Bug 25560 - Define itemtype specific rules in the UpdateNotForLoanStatusOnCheckin system preference
Summary: Define itemtype specific rules in the UpdateNotForLoanStatusOnCheckin system ...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Kyle M Hall
URL:
Keywords: Academy, Manual, release-notes-needed
Depends on:
Blocks:
 
Reported: 2020-05-21 02:11 UTC by Alex Buckley
Modified: 2022-09-22 09:51 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
The UpdateNotForLoanStatusOnCheckin system preference is now more flexible: you can define rules to be applied to specific item types upon check-in. Adding the item type code followed by a colon, and then in separate lines below each value pair rules with leading spaces. Example: BK -1: 0 If an item type is not defined in UpdateNotForLoanStatusOnCheckin then items of that type will not have their notforloan status change on check-in, irrespective of their current notforloan value.
Version(s) released in:


Attachments
Bug 25560: Database and installer changes (2.32 KB, patch)
2020-05-22 00:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Database and installer changes (2.32 KB, patch)
2020-05-22 00:28 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Database and installer changes (2.32 KB, patch)
2020-05-22 00:36 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Updated schema file (1.58 KB, patch)
2020-05-22 00:42 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Template & syspref yaml changes (4.42 KB, patch)
2020-05-22 00:43 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin (2.98 KB, patch)
2020-05-22 00:44 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Template & syspref yaml changes (4.47 KB, patch)
2020-05-22 00:46 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin (4.44 KB, patch)
2020-05-22 00:57 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (4.25 KB, patch)
2020-05-22 04:45 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (5.50 KB, patch)
2020-05-22 04:53 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Database and installer changes (2.41 KB, patch)
2020-09-02 22:37 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Updated schema file (1.15 KB, patch)
2020-09-02 22:38 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Template & syspref yaml changes (4.52 KB, patch)
2020-09-02 22:38 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin (4.44 KB, patch)
2020-09-02 22:39 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (5.50 KB, patch)
2020-09-02 22:39 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (5.50 KB, patch)
2020-09-02 22:41 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (5.81 KB, patch)
2020-10-26 05:56 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (5.80 KB, patch)
2020-11-03 05:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (5.80 KB, patch)
2020-11-05 20:04 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (8.84 KB, patch)
2020-11-06 02:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Database and installer changes (2.45 KB, patch)
2020-11-06 05:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 25560: Updated schema file (1.19 KB, patch)
2020-11-06 05:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 25560: Template & syspref yaml changes (4.56 KB, patch)
2020-11-06 05:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin (4.51 KB, patch)
2020-11-06 05:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 25560: Unit tests (8.88 KB, patch)
2020-11-06 05:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 25560: Database and installer changes (2.52 KB, patch)
2022-03-16 23:55 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Updated schema file (1.60 KB, patch)
2022-03-16 23:56 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Template & syspref yaml changes (4.53 KB, patch)
2022-03-16 23:56 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin (5.50 KB, patch)
2022-03-16 23:56 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (8.48 KB, patch)
2022-03-16 23:57 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin (5.54 KB, patch)
2022-06-07 02:10 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (8.48 KB, patch)
2022-06-07 02:10 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Database and installer changes (2.58 KB, patch)
2022-06-15 11:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25560: Updated schema file (1.66 KB, patch)
2022-06-15 11:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25560: Template & syspref yaml changes (4.59 KB, patch)
2022-06-15 11:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin (5.60 KB, patch)
2022-06-15 11:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25560: Unit tests (8.54 KB, patch)
2022-06-15 11:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25560: (QA follow-up) Add boolean to schema for updatenotforloan and update syspref (2.77 KB, patch)
2022-06-15 11:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25560: Database changes (6.92 KB, patch)
2022-08-02 06:26 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Template changes (2.48 KB, patch)
2022-08-02 06:27 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Define item-type specific rules in UpdateNotForLoanStatusOnCheckin (3.25 KB, patch)
2022-08-02 06:27 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (5.59 KB, patch)
2022-08-02 06:27 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Define item-type specific rules in UpdateNotForLoanStatusOnCheckin (2.90 KB, patch)
2022-08-02 08:16 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25560: Unit tests (4.63 KB, patch)
2022-08-02 08:16 UTC, Alex Buckley
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2020-05-21 02:11:49 UTC
Currently the UpdateNotForLoanStatusOnCheckin syspref will update the items.notforloan value of ALL items checked in regardless of itemtype.

It would be useful to be able to exclude specific itemtypes.

This enhancement will add a checkbox 'Update not for loan status on check-in' to the administration item types page (Administration > Item types). 

When the UpdateNotForLoanStatusOnCheckin syspref contains content and this checkbox is unticked then that itemtype is excluded from having its items.notforloan changed upon checkin.
Comment 1 Alex Buckley 2020-05-22 00:24:54 UTC
Created attachment 105236 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ
Comment 2 Alex Buckley 2020-05-22 00:28:39 UTC
Created attachment 105237 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ
Comment 3 Alex Buckley 2020-05-22 00:36:23 UTC
Created attachment 105238 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ
Comment 4 Alex Buckley 2020-05-22 00:42:44 UTC
Created attachment 105239 [details] [review]
Bug 25560: Updated schema file

Sponsored-By: Waikato Institute of Technology, NZ
Comment 5 Alex Buckley 2020-05-22 00:43:40 UTC
Created attachment 105240 [details] [review]
Bug 25560: Template & syspref yaml changes
Comment 6 Alex Buckley 2020-05-22 00:44:00 UTC
Created attachment 105241 [details] [review]
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin

Sponsored-By: Waikati Institute of Technology, NZ
Comment 7 Alex Buckley 2020-05-22 00:46:08 UTC
Created attachment 105242 [details] [review]
Bug 25560: Template & syspref yaml changes

Sponsored-By: Waikato Institute of Technology, NZ
Comment 8 Alex Buckley 2020-05-22 00:57:32 UTC
Created attachment 105243 [details] [review]
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin

Adds a boolean flag itemtypes.updatenotforloanstatusoncheckin, enabled by default.
When disabled (if the UpdateNoForLoanStatusOnCheckin syspref is
configured) when an item of the type if checked in the notforloan status
does not update.

This enhancement respects the 'item-level_itypes' syspref, so will look
at the authoritative item type when determining to update the notforloan
status.

Test plan:
1. Paste into UpdateNotForLoanStatusOnCheckin syspref
    0 : 1
2. Issue and return an item and observe the item's notforloan status updates
3. Apply patch & update database
cd installer/data/mysql
sudo koha-shell <instancename>
./updatedatabase.pl
4. Restart plack
5. Go to BK (Book) item type page (Administration > Item types > BK).
6. Observe the 'Update not for loan status on check-in' checkbox is
selected by default. Untick it.
7. Make sure the 'item-level_itypes' syspref is set to 'specific item'
8. Issue and return a BK item (item level itype='BK') and observe item's
notforloan status does not update
9. Issue and return a CR (Continuing Resources) item (item level
itype='CR') and observe the item's notforloan status updates

10. Change 'item-level_itypes' syspref to 'bibliographic record'
11. Issue and return an item (with biblio-level itemtype='BK' &
item-level itype='CR') and observe the item's notforloan status does not
update
12. Issue and return an item (with biblio-level itemtype='CR' &
item-level itype='BK') and observe the item's notforloan status
updates

Sponsored-By: Waikato Institute of Technology, NZ
Comment 9 Alex Buckley 2020-05-22 04:45:50 UTC
Created attachment 105244 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 10 Alex Buckley 2020-05-22 04:53:32 UTC
Created attachment 105245 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t
prove t/db_dependent/Circulation/issues.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 11 Alex Buckley 2020-09-02 22:37:50 UTC
Created attachment 109562 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ
Comment 12 Alex Buckley 2020-09-02 22:38:09 UTC
Created attachment 109563 [details] [review]
Bug 25560: Updated schema file

Sponsored-By: Waikato Institute of Technology, NZ
Comment 13 Alex Buckley 2020-09-02 22:38:37 UTC
Created attachment 109564 [details] [review]
Bug 25560: Template & syspref yaml changes

Sponsored-By: Waikato Institute of Technology, NZ
Comment 14 Alex Buckley 2020-09-02 22:39:05 UTC
Created attachment 109565 [details] [review]
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin

Adds a boolean flag itemtypes.updatenotforloanstatusoncheckin, enabled by default.
When disabled (if the UpdateNoForLoanStatusOnCheckin syspref is
configured) when an item of the type if checked in the notforloan status
does not update.

This enhancement respects the 'item-level_itypes' syspref, so will look
at the authoritative item type when determining to update the notforloan
status.

Test plan:
1. Paste into UpdateNotForLoanStatusOnCheckin syspref
    0 : 1
2. Issue and return an item and observe the item's notforloan status updates
3. Apply patch & update database
cd installer/data/mysql
sudo koha-shell <instancename>
./updatedatabase.pl
4. Restart plack
5. Go to BK (Book) item type page (Administration > Item types > BK).
6. Observe the 'Update not for loan status on check-in' checkbox is
selected by default. Untick it.
7. Make sure the 'item-level_itypes' syspref is set to 'specific item'
8. Issue and return a BK item (item level itype='BK') and observe item's
notforloan status does not update
9. Issue and return a CR (Continuing Resources) item (item level
itype='CR') and observe the item's notforloan status updates

10. Change 'item-level_itypes' syspref to 'bibliographic record'
11. Issue and return an item (with biblio-level itemtype='BK' &
item-level itype='CR') and observe the item's notforloan status does not
update
12. Issue and return an item (with biblio-level itemtype='CR' &
item-level itype='BK') and observe the item's notforloan status
updates

Sponsored-By: Waikato Institute of Technology, NZ
Comment 15 Alex Buckley 2020-09-02 22:39:26 UTC
Created attachment 109566 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 16 Alex Buckley 2020-09-02 22:39:55 UTC
Rebased against master
Comment 17 Alex Buckley 2020-09-02 22:41:42 UTC
Created attachment 109567 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 18 Katrin Fischer 2020-10-19 00:50:40 UTC
Still applies, comes with a nice test plan, written in NZ... might be nice for DIY at KohaCon.
Comment 19 Alex Buckley 2020-10-26 05:55:22 UTC
Just documenting for my future reference this is the unit test output David Nind got when he tested yesterday:

---------------

prove -v xt
==============

root@kohadevbox:koha(bz25560)$ prove -v xt
xt/check_makefile.t .............. 
1..1
not ok 1 - All directories must be listed in Makefile.PL

#   Failed test 'All directories must be listed in Makefile.PL'
#   at xt/check_makefile.t line 39.
#          got: '1'
#     expected: '0'
# $VAR1 = [
#           './node_modules'
#         ];
# Looks like you failed 1 test of 1.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 
xt/find-license-problems.t ....... 

.....

Test Summary Report
-------------------
xt/check_makefile.t            (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=9, Tests=1264,  8 wallclock secs ( 0.22 usr  0.02 sys +  7.89 cusr  0.36 csys =  8.49 CPU)
Result: FAIL


prove -v t
===========

....

t/Logger.t .................................. 
1..1
# Subtest: Test01 -- Simple tests for Koha::Logger
    1..6
    ok 1 - Logger did not init correctly without config
    not ok 2 - Logger did not init correctly without permission

    #   Failed test 'Logger did not init correctly without permission'
    #   at t/Logger.t line 52.
    ok 3 - Log4perl config found
    ok 4 - ->warn returned a value
    ok 5 - Undefined method raises warning
    ok 6 - 'catastrophe' method undefined
    # Looks like you failed 1 test of 6.
not ok 1 - Test01 -- Simple tests for Koha::Logger

#   Failed test 'Test01 -- Simple tests for Koha::Logger'
#   at t/Logger.t line 63.
# Looks like you failed 1 test of 1.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

.....

Test Summary Report
-------------------
t/Logger.t                                (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=90, Tests=4083, 845 wallclock secs ( 1.44 usr  0.20 sys + 5335.45 cusr 24.10 csys = 5361.19 CPU)
Result: FAIL


prove -v t/db_dependent/Circulation/issue.t
===========================================

...

not ok 33 - UpdateNotForLoanStatusOnCheckin does not update notforloan value when itemtypes.updatenotforloanstatusoncheckin = 0

...
not ok 50 - The reserve should have been correctly cancelled

#   Failed test 'The reserve should have been correctly cancelled'
#   at t/db_dependent/Circulation/issue.t line 528.
#          got: 'Koha::Hold=HASH(0x5625ed03a538)'
#     expected: undef
# Looks like you failed 2 tests of 50.
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/50 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation/issue.t (Wstat: 512 Tests: 50 Failed: 2)
  Failed tests:  33, 50
  Non-zero exit status: 2
Files=1, Tests=50,  6 wallclock secs ( 0.03 usr  0.01 sys +  5.16 cusr  0.35 csys =  5.55 CPU)
Result: FAIL
Comment 20 Alex Buckley 2020-10-26 05:56:26 UTC
Created attachment 112492 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 21 Alex Buckley 2020-10-26 06:02:08 UTC
Hi David, 

I think I've solved the problem of t/db_dependent/Circulation/issue.t failing.

Unfortunately I haven't yet been able to replicate the other failed tests you encountered ( comment #19 ) I think I'll need to ask some other members of the team if they have any ideas. 

Could you please re-test the unit tests again to see if the change I've made has at least fixed up the t/db_dependent/Circulation/issue.t test?

Thanks,
Alex
Comment 22 David Nind 2020-10-27 11:33:30 UTC
Hi Alex.

It doesn't seem to have.

Errors now after patches applied are:
- prove xt - All pass
- prove t - See below
- prove t/db_dependent/Circulation/issue.t - See below
- prove t/db_dependent/Koha/Object.t - All pass

David

prove -v t
==========

not ok - Test::Perl::Critic for "C4/Barcodes/ValueBuilder.pm"

#   Failed test 'Test::Perl::Critic for "C4/Barcodes/ValueBuilder.pm"'
#   at /usr/share/perl5/Test/Perl/Critic.pm line 104.
# 
#   Package declaration must match filename at line 19, column 1.  Correct the filename or package statement.  (Severity: 5)

....

not ok - Test::Perl::Critic for "Koha/Z3950Responder/RPN.pm"

#   Failed test 'Test::Perl::Critic for "Koha/Z3950Responder/RPN.pm"'
#   at /usr/share/perl5/Test/Perl/Critic.pm line 104.
# 
#   Package declaration must match filename at line 1, column 1.  Correct the filename or package statement.  (Severity: 5)

...

t/Logger.t .................................. 
1..1
# Subtest: Test01 -- Simple tests for Koha::Logger
    1..6
    ok 1 - Logger did not init correctly without config
    not ok 2 - Logger did not init correctly without permission

    #   Failed test 'Logger did not init correctly without permission'
    #   at t/Logger.t line 52.
    ok 3 - Log4perl config found
    ok 4 - ->warn returned a value
    ok 5 - Undefined method raises warning
    ok 6 - 'catastrophe' method undefined
    # Looks like you failed 1 test of 6.
not ok 1 - Test01 -- Simple tests for Koha::Logger

#   Failed test 'Test01 -- Simple tests for Koha::Logger'
#   at t/Logger.t line 63.
# Looks like you failed 1 test of 1.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

....

Test Summary Report
-------------------
t/00-testcritic.t                         (Wstat: 256 Tests: 2132 Failed: 2)
  Failed tests:  842, 1055
  Non-zero exit status: 1
t/Logger.t                                (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=90, Tests=4082, 762 wallclock secs ( 1.23 usr  0.33 sys + 5095.96 cusr 18.73 csys = 5116.25 CPU)
Result: FAIL

prove t/db_dependent/Circulation/issue.t
========================================

root@kohadevbox:koha(bz25560)$ prove t/db_dependent/Circulation/issue.t
t/db_dependent/Circulation/issue.t .. 43/50 
#   Failed test 'The reserve should have been correctly cancelled'
#   at t/db_dependent/Circulation/issue.t line 529.
#          got: 'Koha::Hold=HASH(0x5628572c3ff8)'
#     expected: undef
# Looks like you failed 1 test of 50.
t/db_dependent/Circulation/issue.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/50 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation/issue.t (Wstat: 256 Tests: 50 Failed: 1)
  Failed test:  50
  Non-zero exit status: 1
Files=1, Tests=50,  5 wallclock secs ( 0.03 usr  0.01 sys +  4.27 cusr  0.35 csys =  4.66 CPU)
Result: FAIL
Comment 23 David Nind 2020-10-28 10:25:50 UTC
Hi Alex.

I ran the tests again, both before and after applying the patches, and running through the test plan:
- prove xt: all pass both before and after patches applied, and after working through the test plan
- prove t: the tests that fail are the same that fail BEFORE the patch is applied
- prove t/db_dependent/Circulation/issue.t: tests pass after patches applied, tests fail after have worked through the test plan and run tests again *
- prove t/db_dependent/Koha/Object.t: all pass both before and after patches applied

I'm not sure how the tests that fail before the patches are applied get fixed up, but obviously not caused by the patches in this bug.

David

* Failed tests for prove t/db_dependent/Circulation/issue.t after patches applied and have walked through the test plan

not ok 33 - UpdateNotForLoanStatusOnCheckin does not update notforloan value when itemtypes.updatenotforloanstatusoncheckin = 0

#   Failed test 'UpdateNotForLoanStatusOnCheckin does not update notforloan value when itemtypes.updatenotforloanstatusoncheckin = 0'
#   at t/db_dependent/Circulation/issue.t line 440.

not ok 50 - The reserve should have been correctly cancelled

#   Failed test 'The reserve should have been correctly cancelled'
#   at t/db_dependent/Circulation/issue.t line 529.
#          got: 'Koha::Hold=HASH(0x555f3aa32530)'
#     expected: undef
# Looks like you failed 2 tests of 50.
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/50 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation/issue.t (Wstat: 512 Tests: 50 Failed: 2)
  Failed tests:  33, 50
  Non-zero exit status: 2
Files=1, Tests=50,  6 wallclock secs ( 0.03 usr  0.01 sys +  4.84 cusr  0.38 csys =  5.26 CPU)
Result: FAIL
Comment 24 Alex Buckley 2020-10-28 10:31:10 UTC
Hi David, 

Thanks so much for your testing of this! I'll take a look at why the t/db_dependent/Circulation/issue.t tests are failing after running through the test plan, as I thought I'd fixed that up. 

Glad to hear that the t and xt unit tests are not failing because of these patches.

Thanks again!
Alex
Comment 25 Alex Buckley 2020-11-03 05:24:52 UTC
Created attachment 112887 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 26 Alex Buckley 2020-11-03 05:26:24 UTC
Hi David, 

I've attached a (hopefully!) fixed up unit tests patch. Would you mind re-testing the unit tests and see if the t/db_dependent/Circulation/issue.t test now passes for you?

Many thanks,
Alex
Comment 27 David Nind 2020-11-03 09:45:52 UTC
Hi Alex.

I'm only getting one test failure now:

not ok 33 - UpdateNotForLoanStatusOnCheckin does not update notforloan value when itemtypes.updatenotforloanstatusoncheckin = 0

#   Failed test 'UpdateNotForLoanStatusOnCheckin does not update notforloan value when itemtypes.updatenotforloanstatusoncheckin = 0'
#   at t/db_dependent/Circulation/issue.t line 440.

This happens after applying the patches, and working through the test plan including the system preference changes.

If I just apply the patches and update the database the tests pass.

Let me know if that is sufficient to sign off.

David
Comment 28 Alex Buckley 2020-11-05 20:04:06 UTC
Created attachment 113161 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 29 David Nind 2020-11-05 22:50:07 UTC
For prove -v t/db_dependent/Circulation/issue.t

1. If I run before and after the patches are applied - all tests pass!
2. If I run after running through the test plan - test 33 fails
    not ok 33 - UpdateNotForLoanStatusOnCheckin does not update notforloan value when itemtypes.updatenotforloanstatusoncheckin = 0
    
    #   Failed test 'UpdateNotForLoanStatusOnCheckin does not update notforloan value when itemtypes.updatenotforloanstatusoncheckin = 0'
    #   at t/db_dependent/Circulation/issue.t line 440.
3. If I change item-level_itypes system preference back to 'specific item' - everything passes!
Comment 30 Alex Buckley 2020-11-06 02:24:32 UTC
Created attachment 113212 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 31 Alex Buckley 2020-11-06 02:27:35 UTC
Hi David, 

I've attached another unit test patch. This time I set a mock item-level_itypes preference value before line 440. So now the unit test shouldn't care about what value you have set in the item-level_itypes syspref on your Koha instance.

I've also added in a few more unit test checks in t/db_dependent/Circulation/issue.t and have added more comments which will hopefully help people know what the test is doing!

Could you please re-try testing now?

Many thanks
Alex
Comment 32 David Nind 2020-11-06 05:41:27 UTC
Hi Alex.

Success! Now signing off!

I ran the tests again, both before and after applying the patches, and running through the test plan:
- prove xt: the tests that fail are the same that fail BEFORE the patch is applied, and after working through the test plan (xt/check_makefile.t is the test that fails)
- prove t: the tests that fail are the same that fail BEFORE the patch is applied, and after working through the test plan (t/Logger.t is the test that fails)
- prove t/db_dependent/Circulation/issue.t: all pass both before and after patches applied, and test plan worked through (also passes with changing item-level_itypes)
- prove t/db_dependent/Koha/Object.t: all pass both before and after patches applied, and test plan worked through (also passes with changing item-level_itypes)

David
Comment 33 David Nind 2020-11-06 05:42:32 UTC
Created attachment 113213 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 34 David Nind 2020-11-06 05:42:36 UTC
Created attachment 113214 [details] [review]
Bug 25560: Updated schema file

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 35 David Nind 2020-11-06 05:42:40 UTC
Created attachment 113215 [details] [review]
Bug 25560: Template & syspref yaml changes

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 David Nind 2020-11-06 05:42:44 UTC
Created attachment 113216 [details] [review]
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin

Adds a boolean flag itemtypes.updatenotforloanstatusoncheckin, enabled by default.
When disabled (if the UpdateNoForLoanStatusOnCheckin syspref is
configured) when an item of the type if checked in the notforloan status
does not update.

This enhancement respects the 'item-level_itypes' syspref, so will look
at the authoritative item type when determining to update the notforloan
status.

Test plan:
1. Paste into UpdateNotForLoanStatusOnCheckin syspref
    0 : 1
2. Issue and return an item and observe the item's notforloan status updates
3. Apply patch & update database
cd installer/data/mysql
sudo koha-shell <instancename>
./updatedatabase.pl
4. Restart plack
5. Go to BK (Book) item type page (Administration > Item types > BK).
6. Observe the 'Update not for loan status on check-in' checkbox is
selected by default. Untick it.
7. Make sure the 'item-level_itypes' syspref is set to 'specific item'
8. Issue and return a BK item (item level itype='BK') and observe item's
notforloan status does not update
9. Issue and return a CR (Continuing Resources) item (item level
itype='CR') and observe the item's notforloan status updates

10. Change 'item-level_itypes' syspref to 'bibliographic record'
11. Issue and return an item (with biblio-level itemtype='BK' &
item-level itype='CR') and observe the item's notforloan status does not
update
12. Issue and return an item (with biblio-level itemtype='CR' &
item-level itype='BK') and observe the item's notforloan status
updates

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 37 David Nind 2020-11-06 05:42:48 UTC
Created attachment 113217 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 38 Alex Buckley 2020-11-08 00:04:52 UTC
Yay! Thank you so much for testing David!
Comment 39 Nick Clemens 2021-01-21 20:12:06 UTC
Hi Alex,

This looks good, that's a long column name :-)
Few things:
1 - We should check the itemtype outside of the loop, I would actually check it before loading and parsing the pref
2 - You can use effective_itemtype to avoid the conditional, in fact there is a fetch of that in code above, so you just need to get the object
3 - I am not sure what the behvaiour should be if the itemtype isn't defined or doesn't match a defined value - just mentioning as it may error when you try to fetch the object
Comment 40 Alex Buckley 2022-03-16 23:55:38 UTC
Created attachment 131816 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloan

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 41 Alex Buckley 2022-03-16 23:56:01 UTC
Created attachment 131817 [details] [review]
Bug 25560: Updated schema file

Sponsored-By: Waikato Institute of Technology, NZ
Comment 42 Alex Buckley 2022-03-16 23:56:25 UTC
Created attachment 131818 [details] [review]
Bug 25560: Template & syspref yaml changes

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 43 Alex Buckley 2022-03-16 23:56:48 UTC
Created attachment 131819 [details] [review]
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin

Adds a boolean flag itemtypes.updatenotforloanstatusoncheckin, enabled by default.
When disabled (if the UpdateNoForLoanStatusOnCheckin syspref is
configured) when an item of the type if checked in the notforloan status
does not update.

This enhancement respects the 'item-level_itypes' syspref, so will look
at the authoritative item type when determining to update the notforloan
status.

Test plan:
1. Paste into UpdateNotForLoanStatusOnCheckin syspref
    0 : 1
2. Issue and return an item and observe the item's notforloan status updates
3. Apply patch & update database
cd installer/data/mysql
sudo koha-shell <instancename>
./updatedatabase.pl
4. Restart plack
5. Go to BK (Book) item type page (Administration > Item types > BK).
6. Observe the 'Update not for loan status on check-in' checkbox is
selected by default. Untick it.
7. Make sure the 'item-level_itypes' syspref is set to 'specific item'
8. Issue and return a BK item (item level itype='BK') and observe item's
notforloan status does not update
9. Issue and return a CR (Continuing Resources) item (item level
itype='CR') and observe the item's notforloan status updates

10. Change 'item-level_itypes' syspref to 'bibliographic record'
11. Issue and return an item (with biblio-level itemtype='BK' &
item-level itype='CR') and observe the item's notforloan status does not
update
12. Issue and return an item (with biblio-level itemtype='CR' &
item-level itype='BK') and observe the item's notforloan status
updates

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 Alex Buckley 2022-03-16 23:57:22 UTC
Created attachment 131820 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 Alex Buckley 2022-03-16 23:59:50 UTC
(In reply to Nick Clemens from comment #39)
> Hi Alex,
> 
> This looks good, that's a long column name :-)
> Few things:
> 1 - We should check the itemtype outside of the loop, I would actually check
> it before loading and parsing the pref
> 2 - You can use effective_itemtype to avoid the conditional, in fact there
> is a fetch of that in code above, so you just need to get the object
> 3 - I am not sure what the behvaiour should be if the itemtype isn't defined
> or doesn't match a defined value - just mentioning as it may error when you
> try to fetch the object

Hi Nick, 

My apologies for my slow response!

I think I've implemented your suggestions from comment #39 now. I have handled point 3 by checking if the object for the effective_itemtype is defined before fetching the updatenotforloan value. 

I've also shortened the column name to itemtypes.updatenotforloan, hopefully that is better too.

Would you mind re-testing?

Many thanks,
Alex
Comment 46 Alex Buckley 2022-04-12 02:25:28 UTC
This patchset still applies cleanly to master and is ready for testing if someone is available :)
Comment 47 Alex Buckley 2022-06-07 02:10:00 UTC
Created attachment 135742 [details] [review]
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin

Adds a boolean flag itemtypes.updatenotforloanstatusoncheckin, enabled by default.
When disabled (if the UpdateNoForLoanStatusOnCheckin syspref is
configured) when an item of the type if checked in the notforloan status
does not update.

This enhancement respects the 'item-level_itypes' syspref, so will look
at the authoritative item type when determining to update the notforloan
status.

Test plan:
1. Paste into UpdateNotForLoanStatusOnCheckin syspref
    0 : 1
2. Issue and return an item and observe the item's notforloan status updates
3. Apply patch & update database
cd installer/data/mysql
sudo koha-shell <instancename>
./updatedatabase.pl
4. Restart plack
5. Go to BK (Book) item type page (Administration > Item types > BK).
6. Observe the 'Update not for loan status on check-in' checkbox is
selected by default. Untick it.
7. Make sure the 'item-level_itypes' syspref is set to 'specific item'
8. Issue and return a BK item (item level itype='BK') and observe item's
notforloan status does not update
9. Issue and return a CR (Continuing Resources) item (item level
itype='CR') and observe the item's notforloan status updates

10. Change 'item-level_itypes' syspref to 'bibliographic record'
11. Issue and return an item (with biblio-level itemtype='BK' &
item-level itype='CR') and observe the item's notforloan status does not
update
12. Issue and return an item (with biblio-level itemtype='CR' &
item-level itype='BK') and observe the item's notforloan status
updates

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 48 Alex Buckley 2022-06-07 02:10:22 UTC
Created attachment 135743 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>
Comment 49 Alex Buckley 2022-06-07 02:11:04 UTC
This patchset is rebased against master
Comment 50 Nick Clemens 2022-06-15 11:31:52 UTC
Created attachment 136079 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloan

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 51 Nick Clemens 2022-06-15 11:31:58 UTC
Created attachment 136080 [details] [review]
Bug 25560: Updated schema file

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 52 Nick Clemens 2022-06-15 11:32:02 UTC
Created attachment 136081 [details] [review]
Bug 25560: Template & syspref yaml changes

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 53 Nick Clemens 2022-06-15 11:32:07 UTC
Created attachment 136082 [details] [review]
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin

Adds a boolean flag itemtypes.updatenotforloanstatusoncheckin, enabled by default.
When disabled (if the UpdateNoForLoanStatusOnCheckin syspref is
configured) when an item of the type if checked in the notforloan status
does not update.

This enhancement respects the 'item-level_itypes' syspref, so will look
at the authoritative item type when determining to update the notforloan
status.

Test plan:
1. Paste into UpdateNotForLoanStatusOnCheckin syspref
    0 : 1
2. Issue and return an item and observe the item's notforloan status updates
3. Apply patch & update database
cd installer/data/mysql
sudo koha-shell <instancename>
./updatedatabase.pl
4. Restart plack
5. Go to BK (Book) item type page (Administration > Item types > BK).
6. Observe the 'Update not for loan status on check-in' checkbox is
selected by default. Untick it.
7. Make sure the 'item-level_itypes' syspref is set to 'specific item'
8. Issue and return a BK item (item level itype='BK') and observe item's
notforloan status does not update
9. Issue and return a CR (Continuing Resources) item (item level
itype='CR') and observe the item's notforloan status updates

10. Change 'item-level_itypes' syspref to 'bibliographic record'
11. Issue and return an item (with biblio-level itemtype='BK' &
item-level itype='CR') and observe the item's notforloan status does not
update
12. Issue and return an item (with biblio-level itemtype='CR' &
item-level itype='BK') and observe the item's notforloan status
updates

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Nick Clemens 2022-06-15 11:32:13 UTC
Created attachment 136083 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t/db_dependent/Circulation/issue.t
prove t/db_dependent/Koha/Object.t

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Nick Clemens 2022-06-15 11:32:18 UTC
Created attachment 136084 [details] [review]
Bug 25560: (QA follow-up) Add boolean to schema for updatenotforloan and update syspref

This patch adds boolean definition to schema and adds some verbosity to the syspref
to make it clear that itemtypes must be set to update

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens 2022-06-15 11:34:50 UTC
This works and is a relatively small change so signing off.

I do think this would be better if it didn't split this control into two places: syspref and itemtype administration.

The existing syspref is YAML based, and could be expanded to take itemtypes into account, and would consolidate the feature into a single control.
Comment 57 Jonathan Druart 2022-06-16 13:07:34 UTC
(In reply to Nick Clemens from comment #56)
> This works and is a relatively small change so signing off.
> 
> I do think this would be better if it didn't split this control into two
> places: syspref and itemtype administration.
> 
> The existing syspref is YAML based, and could be expanded to take itemtypes
> into account, and would consolidate the feature into a single control.

I went here to say this. I agree with Nick, it smells to have 2 different places to configure a feature.
Comment 58 Katrin Fischer 2022-06-16 13:09:48 UTC
(In reply to Nick Clemens from comment #56)
> This works and is a relatively small change so signing off.
> 
> I do think this would be better if it didn't split this control into two
> places: syspref and itemtype administration.
> 
> The existing syspref is YAML based, and could be expanded to take itemtypes
> into account, and would consolidate the feature into a single control.

I've had the same thought actually. Having different places to check always makes it harder to debug any configuration issues as it's hard to keep all options in mind that might have an influence on a certain behaviour.
Comment 59 Katrin Fischer 2022-06-16 13:11:20 UTC
Alex, any chance that the feature could be adapted? 

And maybe the others could detail how this could look like?
Comment 60 Alex Buckley 2022-06-20 07:51:54 UTC
Hi Katrin, 

Yes, I can certainly adapt this patchet.

Nick and Jonathan, could you please detail how this could look?

Thanks,
Alex
Comment 61 Alex Buckley 2022-07-29 03:44:31 UTC
Just to note I have a working patchset that can use YAML defined in the UpdateNotForLoanStatusOnCheckin syspref like so:

BK:
 -1: 0

CR: 
 -2: 0

I am currently working on how to handle existing values in the syspref so they still apply for all itemtypes for upgrading libraries.
Any suggestions would be much appreciated on this point.
Comment 62 Nick Clemens 2022-08-01 15:52:59 UTC
(In reply to Alex Buckley from comment #61)
> Just to note I have a working patchset that can use YAML defined in the
> UpdateNotForLoanStatusOnCheckin syspref like so:
> 
> BK:
>  -1: 0
> 
> CR: 
>  -2: 0
> 
> I am currently working on how to handle existing values in the syspref so
> they still apply for all itemtypes for upgrading libraries.
> Any suggestions would be much appreciated on this point.

Maybe add an _ALL_ option - and then existing rules can be placed under that? Similar to UpdateItemLocationOnCheckin - but probably not make _ALL_ override the other rules as it does on that pref
Comment 63 Alex Buckley 2022-08-02 06:26:43 UTC
Created attachment 138494 [details] [review]
Bug 25560: Database changes

Sponsored-by: Waikato Institute of Technology, New Zealand
Comment 64 Alex Buckley 2022-08-02 06:27:07 UTC
Created attachment 138495 [details] [review]
Bug 25560: Template changes

Sponsored-by: Waikato Institute of Technology, New Zealand
Comment 65 Alex Buckley 2022-08-02 06:27:29 UTC
Created attachment 138496 [details] [review]
Bug 25560: Define item-type specific rules in UpdateNotForLoanStatusOnCheckin

This enhancement respects the 'item-level_itypes' syspref, so will look
at the authoritative item type when determining to update the notforloan
status.

Test plan:
1. Set -1 to 'Ordered', and 1 to 'Not for Loan' in Administration > Authorised values > NOT_LOAN

2. Make sure the 'item-level_itypes' syspref is set to 'specific item'

3. Add the following rules in UpdateNotForLoanStatusOnCheckin system
preference:
-1: 0

4. Apply patch & update database
cd installer/data/mysql
sudo koha-shell <instancename>
./updatedatabase.pl

5. Restart plack

6. Observe the UpdateNotForLoanStatusOnCheckin syspref values have
updated to the following format:
<itemtype_1>:
 -1: 0

<itemtype_2>:
 -1: 0

etc.

Each item type setup in Koha has the existing rule(s) listed under it

7. Check in an 'Ordered' item of any item type and confirm it is changed
to 'Available for loan'

8. Reset the UpdateNotForLoanStatusOnCheckin syspref to:
BK:
 -1: 0

CD:
 1: 0

9. Check-in an 'ordered' BK item (item level itype='BK') and observe the item's
notforloan status updates

10. Check-in a 'Not for Loan' CD item (item level itype='CD') and
observe the item's notforloan status updates

11. Check-in an 'ordered' DVD item (item level itype='DVD') and observe
the items notforloan status does not change

12. Change 'item-level_itypes' syspref to 'bibliographic record'

13. Check-in an 'ordered' item (with biblio-level itemtype='BK' &
item-level itype='DVD') and observe the item's notforloan status updates

14. Check-in an 'ordered' item (with biblio-level itemtype='DVD' &
item-level itype='BK') and observe the item's notforloan status does not change

Sponsored-By: Waikato Institute of Technology, NZ
Comment 66 Alex Buckley 2022-08-02 06:27:51 UTC
Created attachment 138497 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t/db_dependent/Circulation/issue.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 67 Alex Buckley 2022-08-02 07:50:45 UTC
(In reply to Nick Clemens from comment #62)
> (In reply to Alex Buckley from comment #61)
> > Just to note I have a working patchset that can use YAML defined in the
> > UpdateNotForLoanStatusOnCheckin syspref like so:
> > 
> > BK:
> >  -1: 0
> > 
> > CR: 
> >  -2: 0
> > 
> > I am currently working on how to handle existing values in the syspref so
> > they still apply for all itemtypes for upgrading libraries.
> > Any suggestions would be much appreciated on this point.
> 
> Maybe add an _ALL_ option - and then existing rules can be placed under
> that? Similar to UpdateItemLocationOnCheckin - but probably not make _ALL_
> override the other rules as it does on that pref

Thanks for that Nick. Am certainly happy to make that change if you would like. 

You will see in the attached patchset I have just added, that I'm listing the existing rules under each item type, like so:

BK:
 -1: 0

CR:
 -1: 0

etc.

The reason for this is I had a chat with Chris and we wondered whether having an _ALL_ option could create user confusion in which rule is overridden in a situation of conflicting rules, like:

_ALL_:
 -1: 0

BK:
 -1: 2

By explicitly defining every item type and all rules that could apply for it, as I have in my attached patchset, it clarifies any potential confusion around rules overriding. It does, however, mean librarians will need to write a rule under every item type option for it to be applied to all items. 

I'm not sure if this should be 'Need signoff' so will leave it with the current setting for now. 

Happy to hear your thoughts though? :)
Alex
Comment 68 Alex Buckley 2022-08-02 08:16:12 UTC
Created attachment 138498 [details] [review]
Bug 25560: Define item-type specific rules in UpdateNotForLoanStatusOnCheckin

This enhancement respects the 'item-level_itypes' syspref, so will look
at the authoritative item type when determining to update the notforloan
status.

Test plan:
1. Set -1 to 'Ordered', and 1 to 'Not for Loan' in Administration > Authorised values > NOT_LOAN

2. Make sure the 'item-level_itypes' syspref is set to 'specific item'

3. Add the following rules in UpdateNotForLoanStatusOnCheckin system
preference:
-1: 0

4. Apply patch & update database
cd installer/data/mysql
sudo koha-shell <instancename>
./updatedatabase.pl

5. Restart plack

6. Observe the UpdateNotForLoanStatusOnCheckin syspref values have
updated to the following format:
<itemtype_1>:
 -1: 0

<itemtype_2>:
 -1: 0

etc.

Each item type setup in Koha has the existing rule(s) listed under it

7. Check in an 'Ordered' item of any item type and confirm it is changed
to 'Available for loan'

8. Reset the UpdateNotForLoanStatusOnCheckin syspref to:
BK:
 -1: 0

CD:
 1: 0

9. Check-in an 'ordered' BK item (item level itype='BK') and observe the item's notforloan status updates

10. Check-in a 'Not for Loan' CD item (item level itype='CD') and
observe the item's notforloan status updates

11. Check-in an 'ordered' DVD item (item level itype='DVD') and observe
the items notforloan status does not change

Sponsored-By: Waikato Institute of Technology, NZ
Comment 69 Alex Buckley 2022-08-02 08:16:33 UTC
Created attachment 138499 [details] [review]
Bug 25560: Unit tests

Test plan:
1. Run tests
sudo koha-shell <instancename>
prove xt
prove t/db_dependent/Circulation/issue.t

Sponsored-By: Waikato Institute of Technology, NZ
Comment 70 Alex Buckley 2022-08-02 08:19:59 UTC
Sorry I realised this patchset no longer checks what the  item-level_itypes syspref is set to. I've removed that, as it is not checked on master, so I re-added the last two patches to reflect that. 

Ready for testing/review again :)
Comment 71 Fridolin Somers 2022-08-13 08:38:15 UTC
Careful Bug 30407 is also modifying this preference
Comment 72 Kyle M Hall 2022-09-16 18:00:10 UTC
Alex, why not add a wildcard option ( say "*" )? That way, if a notforloan change applies to all itemtypes, it won't require creating a section for each and every itemtype. It should also simplify the upgrade patch.
Comment 73 Alex Buckley 2022-09-22 09:51:19 UTC
(In reply to Kyle M Hall from comment #72)
> Alex, why not add a wildcard option ( say "*" )? That way, if a notforloan
> change applies to all itemtypes, it won't require creating a section for
> each and every itemtype. It should also simplify the upgrade patch.

Hi Kyle, 

Thanks for this feedback. Chris and I had thought it might make it clearer having a separate section for each item type, that way there is no confusion around which rule (if there multiple conflicting ones) was prioritised for a particular itemtype.

However, thinking about this more I think you and Nick (in comment #62) are right about having a wildcard option for every item type, and making for a simpler upgrade patch. 

I think Nick's suggestion of using the _ALL_ option as the wildcard, and having that override item type specific rules, like in UpdateItemLocationOnCheckin, would be good so as to maintain consistency in the sysprefs.

I am working on amending my patches to do that now.

Kind regards,
Alex