|
Description
Emmi Takkinen
2022-08-24 06:15:13 UTC
Hi Emmi, I am not aware of a "Please confirm checkout" notice - which letter code would that be? (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. (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! 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 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> 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. 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> I believe the fix is correct, but I am not sure how that escaped so long? 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! Created attachment 143112 [details] [review] Bug 31447: Add unit tests To test prove t/db_dependent/Circulation.t Sponsored-by: Koha-Suomi Oy 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
(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. Created attachment 143349 [details] [review] Bug 31447: Add unit tests To test prove t/db_dependent/Circulation.t Sponsored-by: Koha-Suomi Oy 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. 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 Created attachment 143386 [details] [review] Bug 31447: Add unit tests To test prove t/db_dependent/Circulation.t Sponsored-by: Koha-Suomi Oy My bad. I added those four tests under new subtest to count instead of one. This is now fixed, hopefully they pass now. 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> 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> 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 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.
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> 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> No string changes :) Pushed to master for 22.11. Nice work everyone, thanks! + item_group_id => $item->item_group in t/db_dependent/Circulation.t Looks wrong. And please, perltidy your code! (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" Emmi, did you see the last comments? (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 }}); 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 (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? (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. 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 (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 ? Ok got it, nevermind, I will deal with it. See bug 32353. (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. (In reply to Jonathan Druart from comment #36) > See bug 32353. Ok, makes sense. |