To test:
1 - Set 'OpacRenewalBranch' to various settings
2 - Renew an item for a ptron under each setting
3 - Confirm action_log entries reflect the correct branch for each
secnario
4 - prove t/db_dependent/Circulation/issue.t
Signed-off-by: David Kuhn <techservspec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(cherry picked from commit e9374dcf63)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
(cherry picked from commit d8722dde9b)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables.
For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table.
That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242.
The goal was to prevent data lost.
Data lost may happens in this case:
Check an item out (issue_id = 1)
Check an item in (issue_id = 1)
Restart MySQL (reset auto increment for issue_id to 1)
Check an item out (issue_id = 1)
Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails.
Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues.
That has been fixed using a transaction.
This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected.
A good way to fix that would have been to LOCK the tables:
1- Start a transaction
2- LOCK the table to make sure nobody will read id and avoid race conditions
3- Move the content from one table to the other, dealing with ids
4- UNLOCK the table
5- Commit the transaction
But there were problems using LOCK and DBIx::Class (See commit 905572910b - Do no LOCK/UNLOCK the table).
Finally the solution implemented is not acceptable for several reasons:
- batch checkins may fail
- issue_id will always stay out of sync (between issues and old_issues)
See 18651 comment 66.
Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to:
1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup
=> It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions,
2- Display a warning on the checkin page and link to a solution/explanation
3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix
4- Display a warning on the about page for corrupted data (see bug 18931)
5- Write and make available a maintenance script to fix corrupted data (TODO LATER)
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
The test in SendCirculationAlert is extended by adding an env var
called KOHA_NO_TABLE_LOCKS. If this var is set to a true value,
the table locking is skipped too.
This is useful when running a test without prove. The variable could be
set in a shell profile.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit d51538cb82)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
From the MySQL doc:
"LOCK TABLES is not transaction-safe and implicitly commits any active transaction before attempting to lock the tables."
If the LOCK/UNLOCK statements are executed from tests, the current transaction will be committed.
To avoid that we need to guess if this code is execute from testsa or not (yes it is a bit hacky)
Better ideas are welcome!
Another fix would have been to revert
commit be156d9ad9
Bug 15854: Use a READ and WRITE LOCK on message_queue
but theorically a race is still possible.
Existing tests seem to be safe, to test this patch you will need new
tests from bug 17964.
Test plan:
prove t/db_dependent/Letters/TemplateToolkit.t
twice, and notice that changes have been comitted.
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit b40456f7dd)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Silly error here, the $switch_onsite_checkout is not correctly
calculated, it should not only be set to the value of the syspref, but
meet all the conditions.
Test plan:
Set a circ rule with number of on-site checkout = 2 and number of
checkouts = 2.
Switch on both ConsiderOnSiteCheckoutsAsNormalCheckouts and SwitchOnSiteCheckouts
Do a regular checkout
Do another regular checkout
At this point you have reach the maximum number of checkouts allowed.
Not try another one.
=> Without this patch you will not get the warning
=> With this patch you will see it
Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit f04c5d5a2c)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
The AddRenewal subroutine currently uses the circulation rules for the branch
stored in the Issues table (which is the holding branch) when calculating the new due date.
This patch replaces using the branch from the Issues table with the branch
specified by the HomeOrHoldingBranch syspref.
To test:
1. Set up 2 branches, Branch1 and Branch2
2. Set up a loan rule in Branch1 for DVDs with a 21 day loan period and a
21 day renewal period.
3. Set up a loan rule in Branch2 for DVDs with a 14 day loan period and a
14 day renewal period.
4. Checkout a DVD belonging to Branch1 while logged into Branch2. It will
receive the correct 21 day loan period.
5. Renewing the same DVD while logged into either Branch1 or Branch2
will give a 14 day due date, rather than 21 days.
6. Checkout a DVD belonging to Branch2 while logged into Branch1. It will
receive the correct 14 day loan period.
7. Renewing the same DVD while logged into either Branch1 or Branch2
will give a 21 day due date, rather than 14 days.
8. Apply the patch and repeat steps 4-7. The correct due date should be given
when the item is renewed, regardless of where it is checked out or renewed.
This update removes reassignment of $branch variable.
Signed-off-by: Cédric Vita <cedric.vita@dracenie.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit 7d35bdf58a)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
The table old_issues has a primary key defined on the issue_id column.
This issue_id comes from the issues table when an item is checked in.
In some case the value of issue_id already exists in the table
Basically this happens when an item is returned and mysqld is restarted:
The auto increment value for issues.issue_id will be reset to
MAX(issue_id)+1 (which is the value of the last entry of old_issues).
See also the description of bug 18003 for more informations.
In this solution the change is done at code level instead of DB
structure: If old_issues.issue_id already exists before moving from
the issues table, the issue_id is updated (not on cascade for
accountlines.issue_id, should it?) before the move.
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
(cherry picked from commit 7aa0808ff2)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
To make sure we will not never get a race conditions for these kinds of
notices, we need to add a LOCK on the message_queue table.
This does not smell the best way to do that, but I faced deadlock issues
when I tried to use "UPDATE FOR"
https://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.htmlhttps://dev.mysql.com/doc/refman/5.7/en/lock-tables.htmlhttps://dev.mysql.com/doc/refman/5.7/en/commit.html
To test this patch, or another solution, you need to apply manually this
change:
my $message = C4::Message->find_last_message($borrower, $type, $mtt);
unless ( $message ) {
+ sleep(1);
C4::Message->enqueue($letter, $borrower, $mtt);
} else {
And repeat the test plan from first patch.
Do not forget to truncate the message_queue table.
Followed test plans, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
(cherry picked from commit be156d9ad9)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
There is an obvious race condition when CHECKIN and RENEWAL are
generated from circulation.pl calling svc/renew or svc/checkin in AJAX.
The 2 first queries will try to get the id of the last message
(find_last_message) and if it does not exist, they will insert it.
Theorically that could be lead to have several "digest" messages for a
given patron.
I did not recreate more than 2 messages, from the third one at least one
of the two firsts existed in the DB already.
This patch just simplifies the code to make the SELECT and INSERT or
UPDATE closer and limit the race condition possibilities.
Test plan:
0. Set RenewalSendNotice and circ rules to have a lot of renewals available
1. Use batch checkouts (or one by one) to check out several items to a
patron
2. Empty message_queue (at least of this patron)
3. Renew them all at once ("select all" link, "renew or check in"
button)
4. Check the message_queue
Without this patch you have lot of chances to faced a race condition and
get at least 2 messages for the same patron. This is not expected, we
expect 1 digest with all the messages.
With this patch apply you have lot of chances not to face it, but it's
not 100% safe as we do not use a mechanism to lock the table at the DBMS
level.
Tested both patches together, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
(cherry picked from commit 607b14516a)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Charges should not include elements less than a penny/cent
they are not displayed but can be saved to the database
causing "odd" behaviour down the line
Make the routine round the resultant charge to nearest cent,
so consistent values are returned.
Removed the one case where it was rounded post call.
Although the main danger is values generated by the discount
calculation apply the rounding to all returned charges in case
the item charge is defined using the 3rd or 4th decimal
places.
NOTE: prove -v t/db_dependent/Circulation.t triggers the change.
Though, all the returned amounts are 0.00 only.
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit d84ed89557)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
When a loan period is shortened due to using decreaseLoanHighHolds* the time is
always set to the current time in X days, even if the original loan period is
given in days and not in hours.
It should default to 23:59 as is normal for loan periods given in days.
As original due date time defaults to 23:59 when given in days, this patch
modifies the hours and minutes of shortened due date to be equal to original due
date.
To test:
1. prove t/db_dependent/DecreaseLoanHighHolds.t
Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit 21ac9fcdc2)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
We should require a circulation rule to allow checkouts and reject them
if no rules are defined.
Test plan:
- Delete all issuing rules
- Check an item out
=> Without this patch the checkout is allowed
=> With this patch applied it is rejected
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit 11dfb2e0b2)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
For no discernable reason, when AddIssue calls AddRenewal, it passes the
branchcode generated from _GetCircControlBranch. Assume
_GetCircControlBranch is set to return items.homebranch. So:
1) If an item owned by LibraryA is checked out at LibraryB, the
statistic line branchcode will be LibraryB
2) If an item is renewed via the ajax datatables renewal function, the
statistic line branchcode will be LibraryB the
3) If an item is renewed via scanning the item into the checkout again,
statistic line branchcode will be *LibraryA*
This is clearly improper behavior. The renewal is taking place at
LibraryB, so the branchcode passed to AddRenewal should be LibraryB,
the logged in library. This also jives with the documentation for
the subroutine.
Test Plan:
1) Set CircControl to "the library the item is from" aka ( ItemHomeLibrary )
2) Set HomeOrHoldingBranch to 'The library the items is from" ( aka homebranch )
3) Create item with homebranch of LibraryA and holdingbranch of LibraryB
4) Set the logged in library to LibraryB
4) Check the item out to a patron at LibraryB
5) Note the statistics line has a branchcode of LibraryB
6) Check the item out again to trigger a renewal, renew the item
7) Note the statistic line has a branchcode of LibraryA!
8) Apply this patch
9) Repeat step 6
10) Note the statistics line has a branchcode of LibraryB!
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: David Kuhn <kuhn@monterey.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(cherry picked from commit 2c67648093)
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
If the AllowRenewalIfOtherItemsAvailable sys pref is set to allow, and a
borrower has an item checked out that has many items {30+} AND many
holds {70+) on it, loading the checkouts table for this borrower takes
FOREVER to load. The load time takes forever, because of the
factoring that happens to determine if an item is truly
available for renewal.
This patch swaps the use of GetMemberDetails for GetMember
and reorders the subroutine calls to check each items' renewability
from fastest to slowest.
In a test case, the results of pre patch were:
Start of loop: 2016-08-24T11:05:14
End of loop: 2016-08-24T11:05:29
Resulting in 15 seconds being spent in the loop
Post patch results were:
Start of loop: 2016-08-24T11:08:43
End of loop: 2016-08-24T11:08:48
Resulting in only 5 seconds being spent in the loop!
Test Plan:
1) Apply this patch
2) Note there are no changes in functionality for the renewals column of
the patron checkouts table.
If you wish to go further and test the performance benefit:
1) Create a record with 50 items and 100 holds ( 50 waiting, 50 unfilled )
2) Check out one of the waiting holds to a patron
3) Time the amount of time it takes for the checkouts table to load
4) Apply this patch
5) Repeat step 3, you should see an improvement in load time
Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
If an issue marked as auto_renew is renewed manually, we want to display
the latest auto renew date possible.
Test plan:
1/ Define circ rules as in the previous patch.
2/ Check a item out, mark it as an auto renewal
3/ Back date the issuedate and make sure it will be too late to renew it
4/ Use the Circulation > renew page (circ/renew.pl) to manually renew
this issue.
You should get a warning "You barcode has been scheduled for automatic renewal
and cannot be renewed anymore since DATE."
If the pref AllowRenewalLimitOverride is set, you will be allowed to
renew it anyway.
Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch adds a new circulation rule (no_auto_renewal_after) to block/allow
auto renewals after a given delay.
For instance, if the issue date is 10 days before today, and
no_auto_renewal_after is set to 10, tomorrow the issue won't be auto
renewed.
Test plan:
0/ Execute the update DB entry
Note: You will have to manually change data in your DB, make sure you
have access to the sql cli.
1/ Define a rule with no_auto_renewal_after (10 for instance) and
norenewalbefore (5 for instance).
(This new rule will behave the same as norenewalbefore: the unit depends
on the lengthunit value).
The automatic renewals will be done from 5 to 10 days ahead.
2/ Modify the issues.issuedate, to simulate a checkout in the past:
UPDATE issues
SET issuedate = "yyyy-mm-dd hh:mm:ss"
WHERE itemnumber = YOUR_ITEMNUMBER;
with issuedate = 2 days before for instance
3/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too soon)
4/ Repeat step 2 with a due date set as 11 days before
5/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has not been renewed (too late)
6/ Repeat step 2 with a due date set as 7 days before
7/ Execute the automatic renewals cronjob script (misc/cronjobs/automatic_renewals.pl)
Confirm that the issue has been renewed (issues.renewals has been
incremented and date_due has been updated according your circ rules).
Sponsored-by: University of the Arts London
Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch makes C4::Circulation::AddReturn correctly store the itemtype
on the 'statistics' table.
To reproduce:
- Checkout master.
- Make a checkout.
- Check the 'statistics' table and notice the itemtype is correctly set
> SELECT * FROM statistics;
- Check the item in.
- Check the 'statistics' table and notice the itemtype is not set
> SELECT * FROM statistics WHERE type="return";
=> FAIL: itemtype is set to NULL
To test:
- Apply the regression tests patch
- Run the tests:
$ prove t/db_dependent/Circulation/Returns.t
=> FAIL: Tests fail
- Apply this patch
- Run the regression tests:
$ prove t/db_dependent/Circulation/Returns.t
=> SUCCESS: Tests now pass.
- Repeat the 'To reproduce' steps
=> SUCCESS: itemtype is now correctly set (in real life)
- Happily sign off :-D
Sponsored-by: Universidad Empresarial Siglo 21
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
The subroutine C4::Koha::GetAuthorisedValueByCode returned the
description (staff or opac) for a given authorised value.
Note that we may need a unique key to ->find instead of ->search.
Test plan:
- Checkin an item that cannot be checked in because it's lost, the
message should display the AV description
- Generate a letter with borrowers.streettype equals an ROADTYPE AV, the
description should be displayed.
- Edit a patron attribute type, the AV dropdown list should be
displayed
- Create the PA_CLASS AV category (see bug 7154) and make sure it
behaves as before when editing a patron
- The checkout list should display descriptions for LOC, LOST and
DAMAGED
Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
The subroutine C4::Koha::GetAuthValCode returned the authorised value
category for a given kohafield.
This can be acchieve easily using a new Koha::AuthorisedValues->search_by_koha_field
method which will mimic search_by_marc_field.
Test plan:
Confirm that the description is correctly displayed on the following
pages:
- detail and moredetail of a bibliographic page (itemlost, damaged, materials)
- Set AcqCreateItem=ordering and receiving items.
The description for notforloan, restricted, location, ccode, etc.
field should be displayed.
- Items search form
- On the checkout list from the circulation.pl and returns.pl
pages, the description for "materials" should be displayed
Note that GetKohaAuthorisedValuesMapping is going to be removed on bug
17251.
Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This subroutine is only called once and can be replaced with a call to
is_debarred and has_overdues.
Note that prior to this patch, IsMemberBlocked copy/paste code from
HasOverdues, which did not make sense.
Test plan:
Debar a patron and make sure he is not able to checkout (the librarian
is asked to overwrite if OverduesBlockCirc is set to 'confirmation')
Remove the debarment and add overdues to this patron, same as
previously, the checkout should be blocked
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
The subroutine C4::Koha::GetKohaAuthorisedValueLib just retrieves a description
(lib) for a given authorised value.
We can easily replace it using:
Koha::AuthorisedValues->search({ category => $cat, authorised_value => $av })->lib
or
Koha::AuthorisedValues->search({ category => $cat, authorised_value => $av })->opac_description
Test plan:
- On the detail page of a bibliographic record, the description for notforloan,
restricted and stack (?) should be correctly displayed
- View a shelf, the location (LOC) description should be displayed
- On the search result page, the location description should be displayed in the
facets
- Set AcqCreateItem=ordering and receiving items.
The description for notforloan, restricted, location, ccode, etc. field
should be displayed.
- When creating item in the acquisition module, the dropdown list for
field linked to AV should display the AV' descriptions
- On the transfers page, the description of the location should be
displayed.
- On the checkout list from the circulation.pl and returns.pl pages, the
description for "materials" should be displayed
- Fill some OPAC_SUG AV and create a suggestion, the reason dropdown
list should display the description of OPAC_SUG
Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Since the initial submission of this patch, the module Koha::Borrowers
has been moved to Koha::Patrons. This patch changes the call from
Koha::Borrowers to Koha::Patrons.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Test plan:
1) Apply this patch
2) prove t/db_dependent/Circulation.t
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Same as previous patch but if another circ rule exists
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
In the case on-site checkouts are considered as a regular checkout in issuing
rules (i.e. ConsiderOnSiteCheckoutsAsNormalCheckouts is on):
When after the on-site checkout the maximum limit of checkouts is
reached and the patron wants to switch an on-site checkout to a regular
checkout, the C4::Circulation::TooMany subroutine will return a
TOO_MANY_CHECKOUTS error.
To avoid that, we need to allow an extra checkout in this subroutine.
Test plan:
0/ Switch ConsiderOnSiteCheckoutsAsNormalCheckouts and
SwitchOnSiteCheckouts on
1/ In the issuing rules, set the total number of checkouts (maxissueqty)
to 2 and the number of on-site checkouts to 2 (maxonsiteissueqty)
2/ Check 2 items out ticking the 'on-site checkout' checkbox
3/ Check one of these items out, to automatically switch it to a regular
checkout
=> The checkout should be allowed.
Sponsored-by: BULAC - http://www.bulac.fr/
Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Use case: A patron checks some items out on-site and want to take it home.
To facilitate the librarian work the checkout is directly switched from
on-site to regular when checked out if the new pref SwitchOnSiteCheckouts is on.
Test plan:
0/ Let the new pref SwitchOnSiteCheckouts off
1/ Checkout one items to a patron and tick the "on-site checkout"
checkbox
2/ Check the same item out without ticking the "on-site checkout"
checkbox
=> You should get "This item can not be renewed, it's an on-site checkout"
3/ Switch the pref on
4/ Repeat 2
=> The on-site checkout should be automatically switched to a regular checkout
Sponsored-by: BULAC - http://www.bulac.fr/
Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
With small changes to apply to master.
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch just move C4::Members::HasOverdues to
Koha::Patron->has_overdues and updated callers
Test plan:
No change in behavior is expected.
1/ If a patron is debarred and does not have overdues and
AutoRemoveOverduesRestrictions is on, the debarment will be removed on
checkin
2/ Add overdues and make sure the patron cannot renew
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This is the fourth and last patch set to remove C4::Branch.
The real purpose of this patch is to standardise and refactor some code
which is related to the libraries selection/display.
Its unconfessed purpose is to remove the C4::Branch package.
Before this patch set, only 6 subroutines still existed in the C4::Branch
package:
- GetBranchName
- GetBranchesLoop
- mybranch
- onlymine
- GetBranches
- GetBranch
GetBranchName basically returns the branchname for a given branchcode.
The branchname is only used for a display purpose and we don't need to
retrieve it in package or pl scripts (unless for a few exceptions).
We have a `Branches` template plugin with a `GetName` method which does
exactly this job.
To achieve this removal, we will use this template plugin and delete the
GetBranchName from pl and pm files.
The `Branches.all()` will now select the library of the logged in user
if no `selected` parameter has been passed.
This new behavior could cause regressions, for instance there are some
places where we do not want an option preselected (batch item
modification for instance), keep that in mind when testing.
GetBranchesLoop took 3 parameters: $branch and $onlymine.
The first one was used to set a "selected" flag, for a display purpose:
select an option in the libraries dropdown lists.
The second one was useless: If not passed or set to 0, the
`C4::Branch::onlymine` subroutine was called.
This onlymine flag was use to know if the logged in user was able to see
other libraries infos.
A patron can see the infos from other libraries if IndependentBranches
is not set OR if he has the superlibrarian permission.
Prior to this patch set, the "onlymine test" was done on different
places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the
Branches TT plugin. In this patch set, this test is only done on one
place (C4::Context::only_my_library, code moved from
C4::Branch::onlymine).
To accomplish the same job as this subroutine, we just need to call the
`Branches.all()` method from the `Branches` TT plugin. It already
accepts a `selected` parameter to set a flag on the option to select.
To avoid the repetitive
[% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %]
pattern, a new `html_helpers` TT include file has been created, it
defines an `options_for_libraries` block, which takes a `selected`
parameter. We could imagine to use this include file for other
selects.
The 'mybranch` and `onlymine` subroutines of the C4::Branch package have
been moved to C4::Context. onlymine has been renamed with
only_my_library. There are only 4 occurrences of it, against 11 before
this patch set.
There 2 subroutines are Context-centric and it makes sense to put them
in `C4::Context` (at least it's the least worst place!)
GetBranches is the tricky part of this patch set: It retrieves all the
libraries, independently of the value of IndependentBranches.
To keep the same way as the existing calls of `Branches.all()`, I have
added a `unfiltered` parameter. If set, the `Branches.all()` will call
a usual Koha::Libraries->search method, otherwise
Koha::Libraries->search_filtered will be called. This new method will
check if the logged in user is allowed to see other libraries or only
its library.
Note that this `GetBranches` subroutine also created a `category` key:
it allowed to get the list of groups (of libraries) where this library
existed. Thanks to a previous patch set (bug 15295), this value was
not used anymore (I may have missed something!).
Note that the only use of `GetBranch` was buggy (see bug 15746).
Test plan (for the whole patch set):
The best way to test this whole patch set is to test with 2 instances: 1
with the patch set applied, 1 using master, to be sure there is no
regression.
It would be good to test the same with `IndependentBranches` and the
without `IndependentBranches`.
No difference should be found.
The tester must focus on the library dropdowns on as many forms as
possible.
You will notice changes in the order of the options: the libraries will
now be ordered by branchname (instead of branchcode in some places).
A special attention will be given to the following page:
- acqui/neworderempty.pl
- catalogue/search.pl
- members/members-home.pl (header?)
- opac/opac-topissues.pl
- tools/holidays.pl
- admin/branch_transfer_limits.pl
- admin/item_circulation_alerts.pl
- rotating_collections/transferCollection.pl
- suggestion/suggestion.pl
- tools/export.pl
Notes for QA:
- There are 2 FIXMEs in the patch set, I have kept the existing behavior,
but I am not sure it's the good one. Feel free to open a bug report and
I will fill a patch if you think it's not correct. Otherwise, remove the
FIXME lines in a follow-up patch.
- The whole patch set is huge and makes a lot of changes.
But it finally will tremendously reduce the number of lines:
716 insertions for 1910 deletions
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Renewing an overdue would not work.
Log shows:
renew: Can't use string ("2144746608") as a HASH ref while "strict refs" in use at C4/Overdues.pm line 508., referer: /cgi-bin/koha/circ/circulation.pl?borrowernumber=1
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Test Plan:
1) Find an overdue checkout with a fine
2) Renew item, note fine is not closed out (Account type F)
3) Apply this patch
4) Find another overdue checkout with a fine
5) Renew item, note fine is now correctly closed out
6) Backdate a checkout to be already overdue ( but not have a fine since
fines.pl hasn't run yet )
7) Renew item, note a closed out fine is created
Signed-off-by: Sean Minkel <sminkel@rcplib.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
To verify:
- git grep itemissues
Result: Appears in C4/Circulation.pm only
To test:
- apply patch
- git grep itemissues
Expected result: No occurences
- prove t/db_dependent/Circulation.t
Expected result: Pass OK
Signed-off-by: Claire Gravely <c.gravely@arts.ac.uk>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
In order to move IsMemberBlocked to Koha::Patron it makes sense to move
the code from Koha::Patron::Debarments::IsDebarred to
Koha::Patron->is_debarred.
Test plan:
1/ Add a restriction to a patron
2/ make sure he is not able to checkout items any more
3/ Make sure he cannot get a discharge
4/ Put a hold and make sure you get "Patron has restrictions"
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
* Koha/Patron.pm (wantsCheckPrevCheckout, doCheckPrevCheckout): Rename
to...
(wants_check_for_previous_checkout, do_check_for_previous_checkout):
...this.
* C4/Circulation.pm: Use new names.
* t/db_dependent/Patron/CheckPrevCheckout.t: Renamed to...
* t/db_dependent/Patron/Borrower_PrevCheckout.t: ...this.
Bug 6906: Fix POD reference to old method name.
* Koha/Patron.pm (wants_check_for_previous_checkout): Fix POD.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
New feature: provide granular means to configure warnings about items
that have been issued to a particular borrower before, according to
their checkout history.
- Global syspref ('CheckPrevCheckout'), set to 'hardno' by default,
allows users to enable this feature library wide.
- Per patron category pref allows libraries to create overrides per
category, falling back on the global setting by default.
- Per patron pref allows switching the functionality on at the level
of patron. Fall-back to category settings by default.
* Koha/Patron (wantsCheckPrevCheckout, doCheckPrevCheckout): New
methods.
* C4/Circulation.pm (CanBookBeIssued): Introduce CheckPrevCheckout
check.
* admin/categories.pl: Pass along checkprevcheckout.
* koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt: Expose
CheckPrevCheckout per category setting.
* koha-tmpl/intranet-tmpl/prog/en/modules/preferences/patrons.pref:
Expose CheckPrevCheckout syspref.
* koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt:
Expose per patron CheckPrevCheckout preference.
* koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Expose
per patron CheckPrevCheckout preference.
* koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add
'CHECKPREVCHECKOUT' confirmation message.
* installer/data/mysql/kohastructure.sql: Modify structure of
'categories', 'borrowers', 'oldborrowers'.
* installer/data/mysql/sysprefs.sql: Add 'CheckPrevCheckout'.
* installer/data/mysql/atomicupdate/checkPrevCheckout.sql: New file.
* t/db_dependent/Patron/CheckPrevCheckout.t: New file with unit tests.
Test plan:
- Apply patch.
- Run updatedatabase.
- Regenerate Koha Schema files.
- Run the unit tests.
- Verify 'CheckPrevCheckout' is visible in Patrons sysprefs and can be
switched to 'hardyes', 'softyes', 'softno' and 'hardno'.
+ Check out previously checked out items to a patron, checking the
message appears as expected.
- Verify no 'Check previous checkouts' setting appears on the borrower
category pages if the syspref is set to a 'hard' option.
- Verify 'Check previous checkouts' setting appears on the borrower
category pages and can be modified per borrower category.
+ Issue previously issued items to a borrower, checking the message
appears as expected (This setting should override the default
setting if that is set to a 'soft' option).
- Verify no 'Check previous checkouts' setting appears on the individual
borrower pages if the syspref is set to a 'hard' option.
- Verify 'Check previous checkouts' setting appears on individual
borrower pages and can be modified.
+ Issue previously issued items to a borrower, checking the message
appears as expected (This setting should override the category
setting and the default setting if the latter is set to a 'soft'
option).
Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch picks the item's holding branch *before* it gets fixed
by using the checkin library instead. This way the RefundLostOnReturnControl
syspref set to ItemHoldingBranch is respected (otherwise, as Nick explained
this behaves just like if CheckinLibrary was set)
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org>
Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org>
Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
This patch makes AddIssue and AddReturn use the new implementation
The behaviour should respect the specification.
Sponsored-by: DoverNet
Sponsored-by: South-East Kansas Library System
Sponsored-by: SWITCH Library Consortium
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Jennifer Schmidt <jschmidt@switchinc.org>
Signed-off-by: Margaret Thrasher <p.thrasher@dover.nh.gov>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
To make sure the return can be done, AddIssue must not trust callers (they
should have done their job, but we are not sure) and check that the issue can
be returned before issuing to the patron.
There is no test plan here, this should not be possible from the Koha
interface.
However, looking at the code, it may be possible using SIP.
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
If an issue is already checked out, CanBookBeIssued must check if the
issue can be checked in before processing the return.
In such cases (depending of the AllowReturnToBranch pref), the issue
should not be allowed.
Prior to this patch, the checkin was not done and the checkout failed
with "Duplicate entry '1204321' for key 'itemnumber'". Indeed since bug
14978, there is an uniq key on issues.itemnumber. Before bug 14978 the
issue existed but was hidden (and some weird behaviors certainly
happened!).
To avoid Koha to crash, a check is added to CanBookBeIssued (call to
CanBookBeReturned) and the librarian is not able to process the
checkout.
Test plan:
- Set AllowReturnToBranch to anywhere
- Check an item (homebranch Library 1, holding branch Library 1) out from Library 1
- Check the item out from Library 2
=> Confirm the checkout (should work with and without this patch)
- Set AllowReturnToBranch to holdinbranch ("only the library the item
was checked out from").
- Check an item (homebranch Library 1, holding branch Library 1) out from Library 1
- Check the item out from Library 2
=> Without this patch, Koha crashed
=> With this patch, you will be warned that the checkin is not possible.
Try other combinations of the AllowReturnToBranch syspref
Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nick Clemens
Mason James
Jonathan Druart
Marcel de Rooy
David Kuhn
Colin Campbell
Lari Taskula
Kyle M Hall
Tomas Cohen Arazi
Brendan A Gallagher
Marc Véron
Alex Sassmannshausen