Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
The expected behaviour for "On shelf holds allowed" setting for the circulation rules (Koha administration > Patrons and circulation > Circulation and fines rules):
- Allow holds only on items that are currently checked out or otherwise unavailable.
- If set to "Yes", patrons can place holds on items currently checked in.
- If set to "If any unavailable", patrons can only place holds on items that are not unavailable.
- If set to "If all unavailable", patrons can only place holds on items where *all* items on the record are unavailable.
(Adapted from https://bywatersolutions.com/education/preparing-for-library-closures)
These rules should also work when using ILS-DI, but currently they don't. This bug makes sure that the "On shelf holds allowed" rules work correctly when using ILS-DI to place holds.
Test plan:
1. Enable ILS-DI (set the ILS-DI system preference to Enable).
2. Go to Koha administration > Patrons and circulation > Circulation and fines rules.
3. Work through steps 4-5 for each of the settings for "On shelf holds allowed" for all libraries/patron categories/item types:
. "Yes", "If any unavailable", and "If all unavailable"
4. Staff interface - place a hold on a record with items available for loan, the rules should work as expected before and after the patch is applied:
. "Yes"
==> information column in the item table displays "Not on hold", the hold is placed, cancel the hold
. "If any unavailable" and "If all unavailable"
==> the hold is not placed, message is "Cannot place hold. No items are available to be placed on hold.", red "X" in the hold column and the information column displays "Not on hold".
5. ILS-DI - place a hold on a record with items available for loan (note: without the patch, holds can be placed):
. Query to place a hold using ILS-DI on a title that have all its items available,
example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1
==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
. Query to place a hold using ILS-DI on an available item,
example query: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
==> Without the patch the hold is placed but it shouldn't be allowed, cancel the hold
6. Run the tests prove t/db_dependent/Reserves.t - these should pass.
7. Apply the patch (and flush_memcached and restart_all if using koha-testing-docker).
8. Run through steps 3-6 again, and note the changes when "If any unavailable" and "If all unavailable" options are used:
. For the staff interface: there should be no change in behavour and should work as expected, for the red "X" in the items table additional text is added "onShelfHoldsNotAllowed".
. For ILS-DI: these should now work as expected, with holds not placed, and this message in the results returned <code>onShelfHoldsNotAllowed</code> (check to confirm no holds place for either the patron or the item)
. Tests: should still pass.
9. Sign off.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
The HOLD MODIFY log at the end of ModReserveAffect is not using an
up-to-date $hold object.
$hold is modified at
1201 $hold->set_waiting($desk_id);
But not refreshed before logged (and so the timestamp is not logged
correctly).
Test plan:
Turn on HoldsLog
Place an item on hold
Check it in to mark it waiting
Confirm that the timestamp logged is the one from the check in, not when
you created the hold
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
The patch "Bug 19116: Hold not set to waiting after transfer" added a
new meaning to 'Reserved' return value of C4::Reserves::CheckReserves
function. Let's remove double usage and have separate Transferred
return value so we can differentiate between attached and non-attached
holds. This will come useful in future refactorings.
This patch does no changes to the logic except in the
/cgi-bin/koha/circ/branchtransfers.pl and circulation.pl we now give
similarly to waiting state notice about hold being transferred.
To test:
1) Apply this patch
2) Create a new item level hold so that pickup library is different
than where the item is currently. Then return the item so that hold
is being attached and transferred.
3) Go to branchtransfers.pl and try to create a new transfer: it
should prompt you with message "Item is attached to a hold and
being transferred for XXX" and provide you with option to cancel
the hold or to ignore the transfer.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
With the introduction of datecancellation we now have two fields to
check to varify if a transfer is 'complete' or not. I forgot to update
these tests at the time to ensure the datecancellation field was not set
when adding the mock transfer.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This does a basic check whether a hold gets moved to another biblio
when MergeHolds() is called.
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
After looking at Marcel's comments, the problem is in our matching
to hold_fill_targets - rather than adjusting to find filled/waiting holds we
could ensure that hold_fill_targets only refers to the specific hold it
is intended to
This patch is clearer, if slightly less performant than last (we now return all
the reserves and have to find the 'highest')
Test Plan:
1 - Create and use a patron that can place multiple record level holds per record
2 - Create a record with X items, each at a different library
3 - Place X 'Next available' holds on the record for the patron using the 'Holds to place' box
4 - perl misc/cronjobs/holds/build_holdsqueue.pl
5 - Check in LibraryA's copy as LibraryA and confirm the hold
6 - Revisit request.pl for the record, notice the next hold in line is now item-specific
7 - Checkout the item to the patron, notice the remaining hold is marked waiting
8 - Attempt to place another hold for your patron, notice that it requires an item-specific hold
8 - Apply this patch
9 - Repeat steps 1-5
10 - Revisit request.pl for the record, notice the next hold in line has *not* become item-specific
11 - Checkout the item to the patron, ensure the first hold is filled and the second remains record level
12 - Repeat whole test plan without building holds queue to confirm holds are still treated correctly
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Most of the time the tests are failing because the item is not created
correctly (missing biblio and/or biblioitem).
The usual error is:
t/db_dependent/selenium/regressions.t ..... 5/5 Can't call method "leader" on an undefined value at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch.pm line 534.
In this patch we are making sure $builder->build({ source => 'Item' })
is replace with $builder->build_sample_item
Test plan:
Turn on Elastic and confirm that all the tests pass!
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Since bug 21206, C4::Reserves::_get_itype is not longer used and should
be removed.
commit 31c29fd31f
Bug 21206: Replace C4::Items::GetItem
UPDATE: In the meanwhile another occurrence was added to Reserves.t, but
easy to replace
Test plan:
% git grep _get_itype
must not return any occurrences.
If one needs it, Koha::Item->effective_itemtype must be used instead.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Starting to replace the ModItem calls with Koha::Item->store
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
The number of parameters of AddReserve makes it hard to read and
maintain.
This patch replace it with a hashref, which will make the calls more
readable.
Moreover the bibitems has been removed as it was not used by the
subroutine.
Test plan:
- Make sure the tests pass
- Read the diff and search for typos
- Place a hold on few items
Note for QA: reservation_date and expiration_date do not match the DB column's names,
should we?
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This necessitates moving the circ rules from using '*' to using
undef/NULL.
Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This patch updates all cases where debit_type_code was 'Res' to
'RESERVE' which is more informative and consistent with the new style
guidelines.
The patch also ensure all calls to add_debit with a type or 'reserve'
now pass the capitalised code instead.
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
* Update C4::Accounts::chargelostitem
* Update C4::Accounts::manualinvoice
* Update C4::Circulation::_FixOverduesOnReturn
* Update C4::Circulation::_FixAccountForLostAndReturned
* Update C4::Overdues::UpdateFine
* Update C4::Overdues::GetFine
* Update C4::Overdues::GetOverduesForBranch
* Update Koha::Account->pay
* Update Koha::Account->add_debit
* Update Koha::Account->non_issues_charges
* Update Koha::Account::Line->apply
* Update Koha::Account::Line->adjust
* Update controller scripts
* Update reports scripts
* Update tests
Test Plan
1) Run the test suit and ensure everything still passes
2) Test reports/cash_register_stats still works
3) Test that adding manual invoices still works
4) Test that making payments still works
5) Test that lost item fee handling still works
6) Test that invoice printing still works
7) Test that the sco still works
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Test Plan:
1) Apply dependancies
2) Apply this patch set
3) Run updatedatabase.pl
4) Ensure holdallowed and hold_fulfillment_policy rules behavior remains unchanged
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Almost everywhere we call IsAvailableForItemLevelRequest we already have
a Koha::Patron and Koha::Item object. It makes sense to use them to
avoid a refetch
Test plan:
It would be good to test this patch on top of 19300 and 19301 and make
sure everything works as expected
Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
This patch fixes remaining cases of the list => scalar change.
To test:
- Run:
$ kshell
k$ prove t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Reserves.t
=> FAIL: Tests fail
- Apply this patch
- Run:
k$ prove t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Reserves.t
=> SUCCESS: Tests pass
- Sign off :-D
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Update the relevant test to check for 'description eq title' as aposed
to the old 'description eq "Reserve Charge - title"'.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Test plan:
prove t/db_dependent/Reserves.t
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Branch transfer limits are respected for placing holds in the OPAC but nowhere else. This should be remedied.
Test Plan:
1) Set up a branch transfer limit from Library A to Library B
2) Verify you cannot set up a hold for an item from Library A for pickup at Library B from the staff interface ( without overriding )
3) Verify you cannot place that hold via ILS-DI
4) Verify you cannot place that hold via SIP
4) Verify a forced hold from Library A to Library B will not show up in the holds queue
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
This patch set moves maxissueqty and maxonsiteissueqty to the
circulation_rules table.
Test Plan:
1) Apply this patch
2) Run updatedatabase
3) prove t/db_dependent/Circulation.t
4) prove t/db_dependent/Circulation/Branch.t
5) prove t/db_dependent/Circulation/GetHardDueDate.t
6) prove t/db_dependent/Circulation/Returns.t
7) prove t/db_dependent/Circulation/SwitchOnSiteCheckouts.t
8) prove t/db_dependent/Circulation/TooMany.t
9) prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
10) prove t/db_dependent/Reserves.t
11) Note no changes in circulation behavior related to check out limis
both on and off site
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Note: This is here for information purpose, feel free to test it if you
wan to play with it.
TODO: C4::Reserves::_get_itype is not longer in use
No more GetItem must be returned by:
git grep GetItem|grep -v GetItemsAvailableToFillHoldRequestsForBib|grep
-v GetItemsForInventory|grep -v GetItemsInfo|grep -v
GetItemsLocationInfo|grep -v GetItemsInCollection|grep -v
GetItemCourseReservesInfo|grep -v GetItemnumbersFromOrder|grep -v
GetItemSearchField|grep -v GetItemTypesCategorized|grep -v
GetItemNumbersFromImportBatch|cut -d':' -f1|sort|uniq
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
This patch adds some tests that cover functions changed by this
patchset. A bug in ChargeReserveFee is highlighted.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
This patch adds a new method mock_userenv from t::lib::Mocks in order to
simplify the mock of the userenv.
Test plan:
prove all the test files modified by this patch
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
The following test can fail if the hold has been generated with found => 'W':
# Failed test 'No tests run for subtest "_koha_notify_reserve() tests"'
# at t/db_dependent/Reserves.t line 675.
Can't call method "to_address" on an undefined value at
t/db_dependent/Reserves.t line 661.
# Looks like your test exited with 255 just after 56.
We should call AddReserve instead.
Test plan:
0. Do not apply this patch
1. Do the following change:
my $hold = $builder->build({
source => 'Reserve',
value => {
borrowernumber=>$hold_borrower,
found => 'W', # This line is added, do not forget the comma above
}
});
2. Prove it makes the test fail
3. stash the changes and apply this patch
4. Make sure the tests pass
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Basically the idea is:
1. Undefined subroutine &C4::Items::ModZebra called at /home/vagrant/kohaclone/C4/Items.pm line 302.
=> Then use C4::Items before C4::Biblio
2. Undefined subroutine &C4::Circulation::GetItem called at /home/vagrant/kohaclone/C4/Circulation.pm line 1290
=> Then use C4::Circulation before C4::Items
And sometimes these 2 rules do not work...
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Joonas Kylmälä
Jesse Weaver
Alex Arnaud
Kyle M Hall