https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 --- Comment #2 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |emmi.takkinen@koha-suomi.fi |ity.org | --- Comment #4 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 140000 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140000&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Axelle Aix-Marseille Université <axelle.clarisse@univ-amu.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |axelle.clarisse@univ-amu.fr Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #140338|0 |1 is obsolete| | --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 141292 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=141292&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Failed QA --- Comment #9 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 --- Comment #12 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #15 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #17 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- My bad. I added those four tests under new subtest to count instead of one. This is now fixed, hopefully they pass now. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #141292|0 |1 is obsolete| | --- Comment #18 from David Nind <david@davidnind.com> --- Created attachment 143776 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=143776&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 --- Comment #20 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #143777|0 |1 is obsolete| | --- Comment #23 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 143814 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=143814&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #26 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- + item_group_id => $item->item_group in t/db_dependent/Circulation.t Looks wrong. And please, perltidy your code! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #28 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Emmi, did you see the last comments? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 --- Comment #30 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 --- Comment #32 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 --- Comment #34 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Emmi Takkinen from comment #33)

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

Are you sure? Did you just try item_group_id => $item->item_group->item_group_id ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144241|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31447 --- Comment #37 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- (In reply to Jonathan Druart from comment #34)

(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

?

Just tried it, cannot call item_group_id of undefined value. So either it should be undef or there should be mocked item group. -- You are receiving this mail because: You are watching all bug changes.