Bug 31447 - "Please confirm checkout" message uses patron's home library not holds pick up library
Summary: "Please confirm checkout" message uses patron's home library not holds pick u...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Emmi Takkinen
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 32353
  Show dependency treegraph
 
Reported: 2022-08-24 06:15 UTC by Emmi Takkinen
Modified: 2023-06-08 22:32 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31447: Use holds pick up branch in "Please confirm checkout" message (2.93 KB, patch)
2022-08-31 11:45 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 31447: Use holds pick up branch in "Please confirm checkout" message (2.99 KB, patch)
2022-09-08 12:56 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 31447: Use holds pick up branch in "Please confirm checkout" message (3.04 KB, patch)
2022-10-03 22:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31447: Add unit tests (2.84 KB, patch)
2022-11-03 13:32 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 31447: Add unit tests (2.88 KB, patch)
2022-11-07 10:51 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 31447: Add unit tests (2.88 KB, patch)
2022-11-08 06:51 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 31447: Use holds pick up branch in "Please confirm checkout" message (3.09 KB, patch)
2022-11-11 11:25 UTC, David Nind
Details | Diff | Splinter Review
Bug 31447: Add unit tests (2.93 KB, patch)
2022-11-11 11:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 31447: Use holds pick up branch in "Please confirm checkout" message (3.15 KB, patch)
2022-11-13 14:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31447: Add unit tests (2.98 KB, patch)
2022-11-13 14:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31447: Fix test (1.54 KB, patch)
2022-11-25 11:13 UTC, Emmi Takkinen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2022-08-24 06:15:13 UTC
When one tries to check out item which has hold in it, "Please confirm checkout" notice uses patrons home library instead of holds pick up library. It would be more logical to use latter here.

To test:
1. Find record with holds.
2. For first priority hold, change it's pick up library to differ from patrons homebranch if needed. 
3. Check out records item for a different patron. 
=> Note that notice reads: "Item ... has been on hold for ... at [patrons homebranch] since ...".
Comment 1 Katrin Fischer 2022-08-24 08:52:54 UTC
Hi Emmi, I am not aware of a "Please confirm checkout" notice - which letter code would that be?
Comment 2 Emmi Takkinen 2022-08-24 08:55:45 UTC
(In reply to Katrin Fischer from comment #1)
> Hi Emmi, I am not aware of a "Please confirm checkout" notice - which letter
> code would that be?

Sorry, I guess pop-up or dialog would have been better term. It's the yellow box that is displayed when on tries to check out item with a hold for another patron. The one with options to continue check out or cancel it.
Comment 3 Katrin Fischer 2022-08-24 09:20:31 UTC
(In reply to Emmi Takkinen from comment #2)
> (In reply to Katrin Fischer from comment #1)
> > Hi Emmi, I am not aware of a "Please confirm checkout" notice - which letter
> > code would that be?
> 
> Sorry, I guess pop-up or dialog would have been better term. It's the yellow
> box that is displayed when on tries to check out item with a hold for
> another patron. The one with options to continue check out or cancel it.

Ah! Yes, that makes sense, thanks!
Comment 4 Emmi Takkinen 2022-08-31 11:45:35 UTC
Created attachment 140000 [details] [review]
Bug 31447: Use holds pick up branch in "Please confirm checkout" message

When one tries to check out item which has hold in it,
"Please confirm checkout" message uses patrons home library
instead of holds pick up library. It would be more logical
to use latter here.

To test:
1. Find record with holds.
2. For first priority hold, change it's pick up library to differ from patrons homebranch if needed.
3. Check out records item for a different patron.
=> Note that notice reads: "Item ... has been on hold for ... at [patrons homebranch] since ...".
4. Apply this patch.
5. Repeat steps 2 and 3.
=> Notice should now read: "Item ... has been on hold for ... at [holds pick up branch] since ...".

Sponsored-by: Koha-Suomi Oy
Comment 5 ByWater Sandboxes 2022-09-08 12:56:35 UTC
Created attachment 140338 [details] [review]
Bug 31447: Use holds pick up branch in "Please confirm checkout" message

When one tries to check out item which has hold in it,
"Please confirm checkout" message uses patrons home library
instead of holds pick up library. It would be more logical
to use latter here.

To test:
1. Find record with holds.
2. For first priority hold, change it's pick up library to differ from patrons homebranch if needed.
3. Check out records item for a different patron.
=> Note that notice reads: "Item ... has been on hold for ... at [patrons homebranch] since ...".
4. Apply this patch.
5. Repeat steps 2 and 3.
=> Notice should now read: "Item ... has been on hold for ... at [holds pick up branch] since ...".

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>
Comment 6 Katrin Fischer 2022-10-03 22:00:34 UTC
The test plan is not complete, as also some other occurrences where changed:

RESERVED
[% IF ( RESERVED ) %]
 <li>Item <em>[% getTitleMessageIteminfo | html %]</em> ([% getBarcodeMessageIteminfo | html %]) has been on hold for <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% resborrowernumber | uri %]">[% resfirstname | html %] [% ressurname | html %]</a> ([% rescardnumber | html %]) at [% Branches.GetName( resbranchcode ) | html %] since [% resreservedate | $KohaDates %]</li>
[% END %]

TRANSFERRED
[% IF ( TRANSFERRED ) %]
<li>Item <em>[% getTitleMessageIteminfo | html %]</em> ([% getBarcodeMessageIteminfo | html %]) is on hold for <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% resborrowernumber | uri %]">[% resfirstname | html %] [% ressurname | html %]</a> ([% rescardnumber | html %]) and being transferred to [% Branches.GetName( resbranchcode ) | html %]</li>
[% END %]

PROCESSING
[% IF ( PROCESSING ) %]
<li>Item <em>[% getTitleMessageIteminfo | html %]</em> ([% getBarcodeMessageIteminfo | html %]) is being processed for <a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% resborrowernumber | uri %]">[% resfirstname | html %] [% ressurname | html %]</a> ([% rescardnumber | html %]) at [% Branches.GetName( resbranchcode ) | html %]</li>
[% END %]

In all cases the patron's home library makes no sense, but the hold's pick location does.

It looks like resbranchcode is only used in templates. All circulation tests pass.
Comment 7 Katrin Fischer 2022-10-03 22:01:28 UTC
Created attachment 141292 [details] [review]
Bug 31447: Use holds pick up branch in "Please confirm checkout" message

When one tries to check out item which has hold in it,
"Please confirm checkout" message uses patrons home library
instead of holds pick up library. It would be more logical
to use latter here.

To test:
1. Find record with holds.
2. For first priority hold, change it's pick up library to differ from patrons homebranch if needed.
3. Check out records item for a different patron.
=> Note that notice reads: "Item ... has been on hold for ... at [patrons homebranch] since ...".
4. Apply this patch.
5. Repeat steps 2 and 3.
=> Notice should now read: "Item ... has been on hold for ... at [holds pick up branch] since ...".

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2022-10-03 22:11:02 UTC
I believe the fix is correct, but I am not sure how that escaped so long?
Comment 9 Tomás Cohen Arazi 2022-10-04 11:34:16 UTC
Hi, I've tracked the uses of this and

* SCO
* SIP
* Illrequest

they all really don't care about this. Which is good.

I also read circulation.tt and was surprised this fix hadn't been requested years ago :-D

Can you please add some regression tests? It is clear there are no tests for this particular situation, but it should be easy to build a test by copying any of the ones in Circulation.t and manually changing the status for each use case.

Thanks!
Comment 10 Emmi Takkinen 2022-11-03 13:32:49 UTC
Created attachment 143112 [details] [review]
Bug 31447: Add unit tests

To test prove t/db_dependent/Circulation.t

Sponsored-by: Koha-Suomi Oy
Comment 11 David Nind 2022-11-06 21:19:11 UTC
The tests fail 8-(...

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 62/67 Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.

    #   Failed test 'Branchcodes match when hold exists.'
    #   at t/db_dependent/Circulation.t line 5718.
    #          got: undef
    #     expected: 'CU4TLG6'
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
    # Looks like you failed 1 test of 4.
t/db_dependent/Circulation.t .. 64/67 
#   Failed test 'CanBookBeIssued + needsconfirmation message'
#   at t/db_dependent/Circulation.t line 5733.
# Looks like you planned 67 tests but ran 64.
# Looks like you failed 1 test of 64 run.
t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 4/67 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 256 Tests: 64 Failed: 1)
  Failed test:  64
  Non-zero exit status: 1
  Parse errors: Bad plan.  You planned 67 tests but ran 64.
Files=1, Tests=64, 42 wallclock secs ( 0.11 usr  0.01 sys + 35.68 cusr  3.49 csys = 39.29 CPU)
Result: FAIL
Comment 12 Emmi Takkinen 2022-11-07 07:21:37 UTC
(In reply to David Nind from comment #11)
> The tests fail 8-(...
> 
> prove t/db_dependent/Circulation.t
> t/db_dependent/Circulation.t .. 62/67 Use of uninitialized value
> $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line
> 1119.
> 
>     #   Failed test 'Branchcodes match when hold exists.'
>     #   at t/db_dependent/Circulation.t line 5718.
>     #          got: undef
>     #     expected: 'CU4TLG6'
> Use of uninitialized value $rentalCharge in numeric gt (>) at
> /kohadevbox/koha/C4/Circulation.pm line 1119.
> Use of uninitialized value $rentalCharge in numeric gt (>) at
> /kohadevbox/koha/C4/Circulation.pm line 1119.
> Use of uninitialized value $rentalCharge in numeric gt (>) at
> /kohadevbox/koha/C4/Circulation.pm line 1119.
>     # Looks like you failed 1 test of 4.
> t/db_dependent/Circulation.t .. 64/67 
> #   Failed test 'CanBookBeIssued + needsconfirmation message'
> #   at t/db_dependent/Circulation.t line 5733.
> # Looks like you planned 67 tests but ran 64.
> # Looks like you failed 1 test of 64 run.
> t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
> Failed 4/67 subtests 
> 
> Test Summary Report
> -------------------
> t/db_dependent/Circulation.t (Wstat: 256 Tests: 64 Failed: 1)
>   Failed test:  64
>   Non-zero exit status: 1
>   Parse errors: Bad plan.  You planned 67 tests but ran 64.
> Files=1, Tests=64, 42 wallclock secs ( 0.11 usr  0.01 sys + 35.68 cusr  3.49
> csys = 39.29 CPU)
> Result: FAIL

It seems test fails because of circulation rules. Which is odd since this wasn't an issue while I tested these.
Comment 13 Emmi Takkinen 2022-11-07 10:51:29 UTC
Created attachment 143349 [details] [review]
Bug 31447: Add unit tests

To test prove t/db_dependent/Circulation.t

Sponsored-by: Koha-Suomi Oy
Comment 14 Emmi Takkinen 2022-11-07 10:53:51 UTC
The problem was actually new item group level holds feature (bug 24860). Just needed to add items item_group value to reserves item_group_id.
Comment 15 David Nind 2022-11-07 14:06:35 UTC
Tests still failing - they pass if I change the number of tests from 67 to 64.

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 62/67 Odd number of elements in anonymous hash at t/db_dependent/Circulation.t line 5705.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
t/db_dependent/Circulation.t .. 64/67 # Looks like you planned 67 tests but ran 64.
t/db_dependent/Circulation.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 3/67 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 65280 Tests: 64 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 67 tests but ran 64.
Files=1, Tests=64, 50 wallclock secs ( 0.12 usr  0.01 sys + 42.00 cusr  4.12 csys = 46.25 CPU)
Result: FAIL

Test results if I change the number of tests to 64 (from 67):

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 63/64 Odd number of elements in anonymous hash at t/db_dependent/Circulation.t line 5705.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
t/db_dependent/Circulation.t .. ok     
All tests successful.
Files=1, Tests=64, 44 wallclock secs ( 0.11 usr  0.01 sys + 36.17 cusr  3.52 csys = 39.81 CPU)
Result: PASS
Comment 16 Emmi Takkinen 2022-11-08 06:51:42 UTC
Created attachment 143386 [details] [review]
Bug 31447: Add unit tests

To test prove t/db_dependent/Circulation.t

Sponsored-by: Koha-Suomi Oy
Comment 17 Emmi Takkinen 2022-11-08 06:54:18 UTC
My bad. I added those four tests under new subtest to count instead of one. This is now fixed, hopefully they pass now.
Comment 18 David Nind 2022-11-11 11:25:59 UTC
Created attachment 143776 [details] [review]
Bug 31447: Use holds pick up branch in "Please confirm checkout" message

When one tries to check out item which has hold in it,
"Please confirm checkout" message uses patrons home library
instead of holds pick up library. It would be more logical
to use latter here.

To test:
1. Find record with holds.
2. For first priority hold, change it's pick up library to differ from patrons homebranch if needed.
3. Check out records item for a different patron.
=> Note that notice reads: "Item ... has been on hold for ... at [patrons homebranch] since ...".
4. Apply this patch.
5. Repeat steps 2 and 3.
=> Notice should now read: "Item ... has been on hold for ... at [holds pick up branch] since ...".

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2022-11-11 11:26:04 UTC
Created attachment 143777 [details] [review]
Bug 31447: Add unit tests

To test prove t/db_dependent/Circulation.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2022-11-11 11:34:02 UTC
Note that tests were failing before and after the patches were applied.

I have signed off and created bug 32177.

Before
======

root@kohadevbox:koha(master)$ prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 62/63 
    #   Failed test 'Checkouts with auto-renewal can be renewed earliest on due date if no renewalbefore'
    #   at t/db_dependent/Circulation.t line 5683.
    #          got: '2022-11-11T11:13:53'
    #     expected: '2022-11-25T00:00:00'

    #   Failed test 'Checkouts with auto-renewal can be renewed earliest on due date if no renewalbefore'
    #   at t/db_dependent/Circulation.t line 5689.
    #          got: '2022-11-11T11:13:53'
    #     expected: '2022-11-25T23:59:00'
    # Looks like you failed 2 tests of 5.

#   Failed test 'GetSoonestRenewDate tests'
#   at t/db_dependent/Circulation.t line 5694.
# Looks like you failed 1 test of 63.
t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/63 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 256 Tests: 63 Failed: 1)
  Failed test:  63
  Non-zero exit status: 1
Files=1, Tests=63, 44 wallclock secs ( 0.12 usr  0.01 sys + 36.52 cusr  3.69 csys = 40.34 CPU)
Result: FAIL

After
=====

prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 62/64 
    #   Failed test 'Checkouts with auto-renewal can be renewed earliest on due date if no renewalbefore'
    #   at t/db_dependent/Circulation.t line 5683.
    #          got: '2022-11-11T11:23:12'
    #     expected: '2022-11-25T00:00:00'

    #   Failed test 'Checkouts with auto-renewal can be renewed earliest on due date if no renewalbefore'
    #   at t/db_dependent/Circulation.t line 5689.
    #          got: '2022-11-11T11:23:12'
    #     expected: '2022-11-25T23:59:00'
    # Looks like you failed 2 tests of 5.

#   Failed test 'GetSoonestRenewDate tests'
#   at t/db_dependent/Circulation.t line 5694.
Odd number of elements in anonymous hash at t/db_dependent/Circulation.t line 5705.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
Use of uninitialized value $rentalCharge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 1119.
t/db_dependent/Circulation.t .. 64/64 # Looks like you failed 1 test of 64.
t/db_dependent/Circulation.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/64 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 256 Tests: 64 Failed: 1)
  Failed test:  63
  Non-zero exit status: 1
Files=1, Tests=64, 43 wallclock secs ( 0.10 usr  0.02 sys + 36.13 cusr  3.76 csys = 40.01 CPU)
Result: FAIL
Comment 21 Katrin Fischer 2022-11-13 14:39:32 UTC
The Circulation tests are a quite picky. I often only get them to pass completely after a fresh reset_all. But in this case, these unrelated tests keep failing:

t/db_dependent/Circulation.t .. 62/64 
    #   Failed test 'Checkouts with auto-renewal can be renewed earliest on due date if no renewalbefore'
    #   at t/db_dependent/Circulation.t line 5683.
    #          got: '2022-11-13T14:26:32'
    #     expected: '2022-11-27T00:00:00'

    #   Failed test 'Checkouts with auto-renewal can be renewed earliest on due date if no renewalbefore'
    #   at t/db_dependent/Circulation.t line 5689.
    #          got: '2022-11-13T14:26:32'
    #     expected: '2022-11-27T23:59:00'
    # Looks like you failed 2 tests of 5.
t/db_dependent/Circulation.t .. 63/64 
#   Failed test 'GetSoonestRenewDate tests'
#   at t/db_dependent/Circulation.t line 5694.
Comment 22 Katrin Fischer 2022-11-13 14:41:45 UTC
Created attachment 143813 [details] [review]
Bug 31447: Use holds pick up branch in "Please confirm checkout" message

When one tries to check out item which has hold in it,
"Please confirm checkout" message uses patrons home library
instead of holds pick up library. It would be more logical
to use latter here.

To test:
1. Find record with holds.
2. For first priority hold, change it's pick up library to differ from patrons homebranch if needed.
3. Check out records item for a different patron.
=> Note that notice reads: "Item ... has been on hold for ... at [patrons homebranch] since ...".
4. Apply this patch.
5. Repeat steps 2 and 3.
=> Notice should now read: "Item ... has been on hold for ... at [holds pick up branch] since ...".

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Axelle Clarisse <axelle.clarisse@univ-amu.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2022-11-13 14:41:49 UTC
Created attachment 143814 [details] [review]
Bug 31447: Add unit tests

To test prove t/db_dependent/Circulation.t

Sponsored-by: Koha-Suomi Oy

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2022-11-13 14:42:38 UTC
No string changes :)
Comment 25 Tomás Cohen Arazi 2022-11-14 12:27:30 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 26 Jonathan Druart 2022-11-18 14:09:31 UTC
+        item_group_id => $item->item_group

in t/db_dependent/Circulation.t

Looks wrong.

And please, perltidy your code!
Comment 27 Jonathan Druart 2022-11-18 14:12:02 UTC
(In reply to Jonathan Druart from comment #26)
> +        item_group_id => $item->item_group
> 
> in t/db_dependent/Circulation.t

This is the cause of the new warning "Odd number of elements in anonymous hash"
Comment 28 Jonathan Druart 2022-11-23 09:02:44 UTC
Emmi, did you see the last comments?
Comment 29 Emmi Takkinen 2022-11-23 09:17:40 UTC
(In reply to Jonathan Druart from comment #28)
> Emmi, did you see the last comments?

I did, but haven't had time to look into this. I'll try to get onto this at some point today. And with perltidy, I'm guessing you mean at least this fraction of tests:

my $hold = $builder->build_object({ class => 'Koha::Holds', value => {
        biblionumber => $item->biblionumber,
        branchcode => $library->branchcode,
        itemnumber => undef,
        itemtype => undef,
        priority => 1,
        found => undef,
        suspend => 0,
        item_group_id => $item->item_group
}});
Comment 30 Jonathan Druart 2022-11-23 09:32:34 UTC
Yes, all the subtest actually. Which IDE do you use?

If vi, you can:
1. install perltidy
2. Define a vmap on F8 (or whatever) in your .vimrc
  vmap <F8> :!perltidy -q<CR>
3. Open a new vi instance, select text in visual mode, press F8
Comment 31 Emmi Takkinen 2022-11-23 11:50:00 UTC
(In reply to Jonathan Druart from comment #30)
> Yes, all the subtest actually. Which IDE do you use?
> 
> If vi, you can:
> 1. install perltidy
> 2. Define a vmap on F8 (or whatever) in your .vimrc
>   vmap <F8> :!perltidy -q<CR>
> 3. Open a new vi instance, select text in visual mode, press F8

Thanks, this worked fine. I was able to fix tests and perltidy them, should I just at the patch here or should I make a new bug?
Comment 32 Jonathan Druart 2022-11-25 06:30:38 UTC
(In reply to Emmi Takkinen from comment #31)
> (In reply to Jonathan Druart from comment #30)
> > Yes, all the subtest actually. Which IDE do you use?
> > 
> > If vi, you can:
> > 1. install perltidy
> > 2. Define a vmap on F8 (or whatever) in your .vimrc
> >   vmap <F8> :!perltidy -q<CR>
> > 3. Open a new vi instance, select text in visual mode, press F8
> 
> Thanks, this worked fine. I was able to fix tests and perltidy them, should
> I just at the patch here or should I make a new bug?

It's too late, but keep it in mind for your next patches.

For the test a follow-up here should be fine, but it's release today so that should be discussed with Tomas.
Comment 33 Emmi Takkinen 2022-11-25 11:13:09 UTC
Created attachment 144241 [details] [review]
Bug 31447: Fix test

Tests for this feature raised a new warning
"Odd number of elements in anonymous hash".
Setting $item->item_group as seperated variable
removes this warning.

To test prove t/db_dependent/Circulation.t.

Sponsored-by: Koha-Suomi Oy
Comment 34 Jonathan Druart 2022-11-25 12:00:06 UTC
(In reply to Emmi Takkinen from comment #33)
> Created attachment 144241 [details] [review] [review]
> Bug 31447: Fix test
> 
> Tests for this feature raised a new warning
> "Odd number of elements in anonymous hash".
> Setting $item->item_group as seperated variable
> removes this warning.
> 
> To test prove t/db_dependent/Circulation.t.
> 
> Sponsored-by: Koha-Suomi Oy

Are you sure?
Did you just try

  item_group_id => $item->item_group->item_group_id

?
Comment 35 Jonathan Druart 2022-11-25 12:02:43 UTC
Ok got it, nevermind, I will deal with it.
Comment 36 Jonathan Druart 2022-11-25 12:06:06 UTC
See bug 32353.
Comment 37 Emmi Takkinen 2022-11-25 12:07:18 UTC
(In reply to Jonathan Druart from comment #34)
> (In reply to Emmi Takkinen from comment #33)
> > Created attachment 144241 [details] [review] [review] [review]
> > Bug 31447: Fix test
> > 
> > Tests for this feature raised a new warning
> > "Odd number of elements in anonymous hash".
> > Setting $item->item_group as seperated variable
> > removes this warning.
> > 
> > To test prove t/db_dependent/Circulation.t.
> > 
> > Sponsored-by: Koha-Suomi Oy
> 
> Are you sure?
> Did you just try
> 
>   item_group_id => $item->item_group->item_group_id
> 
> ?

Just tried it, cannot call item_group_id of undefined value. So either it should be undef or there should be mocked item group.
Comment 38 Emmi Takkinen 2022-11-25 12:08:21 UTC
(In reply to Jonathan Druart from comment #36)
> See bug 32353.

Ok, makes sense.