Created attachment 105236 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ

Created attachment 105237 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ

Created attachment 105238 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ

Created attachment 105239 [details] [review]
Bug 25560: Updated schema file

Sponsored-By: Waikato Institute of Technology, NZ

Created attachment 105240 [details] [review]
Bug 25560: Template & syspref yaml changes

Created attachment 105241 [details] [review]
Bug 25560: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin

Sponsored-By: Waikati Institute of Technology, NZ

Created attachment 105242 [details] [review]
Bug 25560: Template & syspref yaml changes

Sponsored-By: Waikato Institute of Technology, NZ

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

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

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

Created attachment 109562 [details] [review]
Bug 25560: Database and installer changes

-- Adds boolean flag itemtypes.updatenotforloanstatusoncheckin

Sponsored-By: Waikato Institute of Technology, NZ

Created attachment 109563 [details] [review]
Bug 25560: Updated schema file

Sponsored-By: Waikato Institute of Technology, NZ

Created attachment 109564 [details] [review]
Bug 25560: Template & syspref yaml changes

Sponsored-By: Waikato Institute of Technology, NZ

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

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

Rebased against master

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

Still applies, comes with a nice test plan, written in NZ... might be nice for DIY at KohaCon.

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

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

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

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

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

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

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

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

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

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

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!

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

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

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

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>

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>

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>

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>

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>

Yay! Thank you so much for testing David!

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

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>

Created attachment 131817 [details] [review]
Bug 25560: Updated schema file

Sponsored-By: Waikato Institute of Technology, NZ

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>

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>

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>

(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

This patchset still applies cleanly to master and is ready for testing if someone is available :)

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>

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>

This patchset is rebased against master

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>

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>

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>

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>

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>

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>

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.

(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.

(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.

Alex, any chance that the feature could be adapted? 

And maybe the others could detail how this could look like?

Hi Katrin, 

Yes, I can certainly adapt this patchet.

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

Thanks,
Alex

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.

(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

Created attachment 138494 [details] [review]
Bug 25560: Database changes

Sponsored-by: Waikato Institute of Technology, New Zealand

Created attachment 138495 [details] [review]
Bug 25560: Template changes

Sponsored-by: Waikato Institute of Technology, New Zealand

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

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

(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

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

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

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 :)

Careful Bug 30407 is also modifying this preference

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.

(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

> 
> I am working on amending my patches to do that now.
> 
> Kind regards,
> Alex

Hi Alex, moving this out of queue for now - please switch back to Signed-off once you are ready!

(In reply to Katrin Fischer from comment #74)
> > 
> > I am working on amending my patches to do that now.
> > 
> > Kind regards,
> > Alex
> 
> Hi Alex, moving this out of queue for now - please switch back to Signed-off
> once you are ready!

Hi Katrin, thank you, will do!

Created attachment 146556 [details] [review]
Bug 25560: Database changes

Sponsored-by: Waikato Institute of Technology, New Zealand

Created attachment 146557 [details] [review]
Bug 25560: Template changes

Sponsored-by: Waikato Institute of Technology, New Zealand

Created attachment 146558 [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:
_ALL_:
 -1: 0

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

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

CD:
 -1: 2
 2: 0

9. Check-in an 'ordered' (NOT_LOAN=-1) CD item (item level itype='CD') and observe the item's notforloan status updates to 'Staff collection' (NOT_LOAN=2). This is because the _ALL_ rule does not override all other rules.

10. Check-in a 'Staff collection' (NOT_LOAN=2) CD item (item level itype='CD') and
observe the item's notforloan status updates to 'Available for loan'
(NOT_LOAN=0)

11. Check-in a 'Staff collection' (NOT_LOAN=2) DVD item (item level itype='DVD') and observe
the items notforloan status does not change

12. Check-in an 'ordered' (NOT_LOAN=-1) DVD item (item level itype='DVD') and observe the items notforloan status updates to 'Available for loan' (NOT_LOAN=0)

Sponsored-By: Waikato Institute of Technology, NZ

Created attachment 146559 [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

I've attached some updated patches that use a wildcard '_ALL_' as a special term to apply for all item types. 

These patches are not quite ready for testing yet.

Created attachment 146993 [details] [review]
Bug 25560: Unit tests

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

Sponsored-By: Waikato Institute of Technology, NZ

(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, 

Setting this back to 'Signed-off' as Katrin said in comment #74.

I have added the _ALL_ wildcard, like how Nick suggested in comment #62 . Also, as suggested in that comment I have made this wildcard not override other rules. 

I've also updated the unit tests to test using the wildcard.

Could you please take a look and let me know what other changes might be needed for QA?

Thanks,
Alex

Created attachment 148679 [details] [review]
Bug 25560: Database changes

Sponsored-by: Waikato Institute of Technology, New Zealand

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 148680 [details] [review]
Bug 25560: Database changes

Sponsored-by: Waikato Institute of Technology, New Zealand

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 148681 [details] [review]
Bug 25560: Template changes

Sponsored-by: Waikato Institute of Technology, New Zealand

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 148682 [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:
_ALL_:
 -1: 0

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

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

CD:
 -1: 2
 2: 0

9. Check-in an 'ordered' (NOT_LOAN=-1) CD item (item level itype='CD') and observe the item's notforloan status updates to 'Staff collection' (NOT_LOAN=2). This is because the _ALL_ rule does not override all other rules.

10. Check-in a 'Staff collection' (NOT_LOAN=2) CD item (item level itype='CD') and
observe the item's notforloan status updates to 'Available for loan'
(NOT_LOAN=0)

11. Check-in a 'Staff collection' (NOT_LOAN=2) DVD item (item level itype='DVD') and observe
the items notforloan status does not change

12. Check-in an 'ordered' (NOT_LOAN=-1) DVD item (item level itype='DVD') and observe the items notforloan status updates to 'Available for loan' (NOT_LOAN=0)

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 148683 [details] [review]
Bug 25560: Unit tests

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

Sponsored-By: Waikato Institute of Technology, NZ

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Pushed to master for 23.11.

Nice work everyone, thanks!

Enhancement not pushed to 23.05.x