Bug 25560 - Exclude itemtypes from UpdateNotForLoanStatusOnCheckin
Summary: Exclude itemtypes from UpdateNotForLoanStatusOnCheckin
Status: Failed QA
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: Testopia
URL:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2020-05-21 02:11 UTC by Alex Buckley
Modified: 2021-01-21 20:12 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Text to go in the release notes:
Add boolean flag itemtypes.updatenotforloanstatusoncheckin, enabled by default. When the flag is disabled any items of the type will be excluded from check-in update by UpdateNotForLoanStatusOnCheckin syspref.
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

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