Good catch from Jonathan. See comment11.
Authorities detail should pass a CSRF token to authorities-home when
deleting a record without linked biblios.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Passing the token with GET is not a good way to do, but nothing quick to
replace that.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Without this patch, it is possible to delete authority records with URL
manipulation.
Like: /cgi-bin/koha/authorities/authorities-home.pl?op=delete&authid=[XXX]
Test plan:
[1] Go to Authorities. Search for some authorities (without links).
[2] Delete an authority. Should work.
[3] Apply patch.
[4] Construct an URL like above to delete another authority. Should fail.
Under Plack this results in an internal server error, the log tells
you: Wrong CSRF token.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Amended the test plan.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Instead of passing the homebranch to the slip, let's pass
TransferWaitingAt (the value we display)
To test:
1 - Check in an item that will initiate a transfer (hold or automatic
item return)
2 - Note the transfer message and receipt match
3 - Remove the reason for transfer (bot don't cancel transfer)
i.e. cancel hold or change homebranch of item to current library
4 - Check the item in again
5 - Note the message displays the transfer destination and the slip
shows the homebranch
6 - Apply patch
7 - Repeat 5 but note slip and message match
8 - Test any edge cases for correctness
9 - sign off
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>
- Remove useless comments
- Use Koha::Objects::find instead of Koha::Objects::search
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Test plan:
* run without the patch, the test will fail
* run with the patch, the test will pass
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Without this patch, there is an inconsistency in subscriptions:
* a new subscription has no history end date set (as expected)
* if you renew it, a date is set.
This patch removes the setting of the history end date during renewal
Test plan:
* create a subscription, go to subscription-detail.pl page, tab "Summary". No History end date is set
* renew the subscription => a date has been set
* apply the patch
* create another subscription
* renew it => no history end date has been set
* sign-off
Signed-off-by: beroud <isabelle.beroud@univ-lyon3.fr>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Fields with underscore like B_address do not print on patron cards.
To reproduce:
- Create patron card layout using fields with underscore in their name
(e.g. <surname><B_address> )
- Print (export) patron card
- Verify that fields without underscore are replaced by their value,
but fields with underscore do not replace but show the field name
To test:
- Apply patch
- Try to reproduce and verify that fields with underscore are replace
as expected
Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
The combobox on the left side of late orders is filled by sub
GetBooksellersWithLateOrders. The same change as in the first patch
must be made here to include suppliers with late orders without a
price.
Bonus: Sort the list.
Test plan:
[1] Run t/db_dependent/Bookseller.t.
[2] Go to late orders. Use the filter on suppliers.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch removes the filter of unitpricesupplier <> 0 and
unitpricelib <> 0 from GetLateOrders as used in lateorders.pl.
This allows you to claim late orders without a price entered.
Test plan:
[1] Run t/db_dependent/Acquisition.t
[2] Check if you see an order with no price in lateorders.pl. (If needed,
remove prices from a few orders.)
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Sounds more appropriate and consistent with existing action logs.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
For an unknown reason, the use_ok('Circulation') does not work as
intended (see 3660c451a3).
With the new use of C4::Log, the trick does no longer work.
It does not make sense to add the use_ok('C4::Log') in Circulation.t,
removing it.
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
20/02/17 : added the syspref RenewalLog
24/20/17 : added a test for the syspref Renewal Log
test plan
1 - Chose a Borrower and have him renewing an item
2 - Check the renew logs : they should be empty
3 - Apply patch and set the syspref RenewalLog to 1
4 - Have the Borrower renewing a new item
5 - Check the renew logs : there should be your renew
I called the function logaction, which is in charge of modifying the
logs, within the function which adds a new renewal at the list.
Signed-off-by: Julien Comte <julien.comte@u-psud.fr>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch adds a datatable to the circ overdue report. Since the
datatable makes sorting easy, I've also removed the sort by filter on
the side.
To test:
1) Go to Circulation -> Overdues
2) Notice overdues report shows as datatable
3) Confirm sorting on all columns works
4) Test the Filters and confirm they all still work as expected
Sponsored-by: Catalyst IT
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
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 updates SIP checkin to use reserve_id as param, to fix regression
introduced by Bug 14695.
To test:
Given 1 patron and 1 biblio with two items attached:
1) create two record level holds on one biblio on same user
Using SIP client or telnet:
2) checkin item x on pickup branch, observe item is waiting and hold 1 is filled
3) checkin item y on pickup branch, observe item y now is waiting and x is available
4) apply this patch and repeat 1-3, now expected behaviour should be
that both holds are filled and both items waiting
Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Hie, Tools > Batch record deletion seems broken.
Any deletion returns error :
Bibliographic record YYY was not deleted. An error occurred. (The error was: {UNKNOWN}: DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1560. at /home/koha/src/C4/Biblio.pm line 3468 , see the Koha log file for more information).
Looks like it is because of Bug 18242 which added a transaction in C4::Biblio::_koha_delete_biblio_metadata : $schema->txn_do.
The script batch_delete_records created a transaction with $dbh->{AutoCommit} = 0;
This patch fixes by using also Koha::Schema in batch_delete_records to manage transaction.
It also removes "$dbh->{RaiseError} = 1", this behavior is managed in Koha::Database.
Test plan :
- Go to Staff interface : Tools > Batch record deletion
- Enter a few existing biblionumbers
- Click on "Continue"
- Click on "Delete selected records"
=> Without patch you get a DB error
=> With patch you get confirmation message
- Try to get the biblios to confirm they are deleted : /cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx
- Test with and without Plack
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>
The zero status should be added in @notforloans. It is a legitimate status
for available books. The zero status description can be translated in
the template.
The 'Change item status' is confusing. We mean that we saw a notforloan
status that we do not expect (read: does not exist in the table).
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
In inventory tool, there is a test on scanned items notforloan value : if the value is not one of authorized values, the inventory results displays the problem "Change item status".
The bug is that if notforloan filter is not used, the notforloan values array is not filled and so all scanned items are marked with this problem.
Test plan :
- Check you have in default framework an item subfield mapped with items.notforloan and with an authorized value catergory, for example 'NOTFORLOAN'
- Check the authorized value category has at least value '0'
- Create a new value in this category, for example '9'
- Create a biblio with default framework
- Create an item with barcode='000AAA1', callnumber='ZZZAAA1' and notforloan=0
- Create an item with barcode='000AAA2', callnumber='ZZZAAA2' and notforloan=9
- Delete new value created above, for example '9'
- Create a file 'barecodes.txt' containing 2 lines '000AAA1' and '000AAA2'
- Go to inventory tool : /cgi-bin/koha/tools/inventory.pl
- Choose file in 'Barcode file'
- Enter item callnumber between 'ZZZ' and 'ZZZZ'
- Check 'Compare barcodes list to results'
- Submit
=> without patch, you get 2 results with problem 'Change item status' even for item with notforloan=0
=> with patch, you get 1 result with 'Change item status' on item with notforloan=9
Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I removed several lines of code in the checkpw_ldap subroutine where
LDAP authentication takes place, in the "else" part of the conditional
that checks for the auth_by_bind config parameter. I added several lines
to check whether the user can log in to LDAP using their DN and the
password supplied in the login form. If they are able to bind, login
contiues as normal and the LDAP attributes can be harvested as normal if
the update options are turned on. The routine that was in place was
failing because it was trying to check against a non-existent LDAP entry
attribute called 'userpassword'. Instead of checking against a
'userpassword' attribute, the routine really should be checking to make
sure the user can actually bind to LDAP. That's what I set up, and it is
a safer way to test authentication against LDAP.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended as per comment33 of Alex Sassmannshausen.
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch is a QA follow-up to fix several issues:
- 1 call to GetReserveFee was wrong in ModReserveFill
- Update DB entry was wrong and insufficient
- Add robustness to the tests in sco-main
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Sponsored-by: Cheshire Libraries
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Sponsored-by: Cheshire Libraries
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Currently, Koha charges all patrons a hold fee in all circumstances, if
a hold fee is applicable to their patron category.
This is immediately applied at point of request.
However, it would be useful to let patrons make requests without a
charge
being incurred until they physically have the item in their hands and
checked out to their cards.
The hold fee will only be added to the account as soon as the item is
checked out to the requesting patron.
With this scenario, we will be certain that patrons have the correct
item, and they are happy with what has been supplied.
It also means that patrons can place holds via the OPAC without reaching
the usage limit that has been selected.
Test plan:
0/ All the following steps must be done with a patron using a patron category with a hold fee
1/ Make sure that the existing options for HoldFeeMode work as before
2/ Select the third option "any time a hold is collected"
3/ Place a hold on an item
4/ Note that the patron has not been charged
5/ Check this item from the staff interface
6/ Note that the patron has been charged
7/ Place another hold
8/ Use the self checkout feature at the OPAC for the checkin
9/ Note that the patron has been charged and a message is displayed to
inform about the fee.
Sponsored-by: Cheshire Libraries
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch updates the current code to make it works with the new
option's name of the syspref.
It also refactor the tests to make them more reusable and robust.
Sponsored-by: Cheshire Libraries
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch adds a new 'any_time_is_collected' option to the HoldFeeMode
syspref.
The existing 'always' option is renamed with 'any_time_is_placed''
Sponsored-by: Cheshire Libraries
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
while ->new is terrible, let's call that a typo...
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
opac-detail works again !
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Several warnings like:
Variable "$confirm_pref" is not available at /usr/share/koha/masterclone/acqui/basket.pl line 507.
Primarily caused by sub edi_close_and_order.
Easy fix.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
The solution of Jonathan can be applied in two other cases, effectively
making GetItemHolds obsolete.
Test plan:
[1] Git grep on GetItemHolds
[2] Add an order, place a hold, delete order.
[3] Add an order, receive, place hold, delete order.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Crash in basket.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/basket.pl line 466.
Comes from a biblionumber == NULL in aqorders where I cancelled the order and deleted the biblio.
Crash in parcel.pl:
Can't call method "holds" on an undefined value at /usr/share/koha/masterclone/acqui/parcel.pl line 246.
Similar fix.
Trivial fixes indeed.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Trivial fix.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
FAIL t/db_dependent/Holds.t
"my" variable $hold masks earlier declaration in same scope
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Like:
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetMarcBiblio() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.
Use of inherited AUTOLOAD for non-method Koha::Biblio::GetRecordValue() is deprecated at /usr/share/koha/masterclone/Koha/Biblio.pm line 60.
Resolved by not importing them but fully qualifying them.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
It is not about when the hold was 'placed' but if the hold pertains to
the future or not.
Test plan:
[1] Git grep on holds_placed_before_today.
[2] Run t/db_dependent/Koha/Biblios.t
[3] Run t/db_dependent/Reserves.t
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
At this point, there should not be any occurrences of
GetReservesFromBiblionumber left in the codebase
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
The C4::Reserve::GetReservesFromBiblionumber took 3 parameters, the
biblionumber, an optional itemnumber and a "all_dates" flag.
If set, the subroutine returned all the holds placed on a given bibliographic
record, even the ones placed in the future. Almost all of the calls had this
flag set, they will be replaced with a call to Koha::Biblio->holds.
But 5 did not have it:
- C4::Biblio::DelBiblio
-tools/batch_delete_records.pl
=> These 2 were wrong, we want to retrieve the holds to cancel them
before deleting the record. We need to get all the holds, even the ones
placed in the future /!\ CHANGE IN THE BEHAVIOR
- acqui/parcel.pl
=> 1 call per item were made to this subroutine. They have been replaced
with only 1 call to the new method Koha::Biblios->holds_placed_before_today
Then we filter on the itemnumbers.
I think this is wrong: we need the number of holds to know if the record
can be deleted, so even if future holds exist, the deletion should not
be possible.
- serials/routing-preview.pl
- C4::ILSDI::Services::GetRecords
- C4::SIP::ILS::Item->new
=> Seems ok, we just one to display holds placed before today
Test plan:
I would suggest to test this patch with patches from bug 17737 and bug 17738,
to place different kind of holds (biblio and item level, future and
past).
Then do a whole workflow to detect bug, view a record, delete record,
order, place a hold on an item which has been ordered, etc.
The hold's informations should always be the same without or without
these patches.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
The DB field issues.return has never really been used apparently.
I found the first occurrence of this field in
commit eac3a7b19a
CommitDate: Mon Mar 12 22:43:47 2001 +0000
Database definition file, checked into cvs to make keeping database
current easier
Since I did not find any use of this field.
I guess it can be removed safely.
There is no proper test plan here. Just make sure this field has never
been used.
Signed-off-by: Magnus Enger <magnus@libriotech.no>
I have not found any use of issues.return or old_issues.return. In
all my live instances the column is always NULL. Issuing and returning
seems to work as expected after the columns have been removed.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
We should call slice in list context with the correct indexes.
Test plan:
Run t/db_dependent/Koha/Objects.t
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
DBIx::Class does not provide such method, but it can be handy in some
cases.
Test plan:
prove t/db_dependent/Koha/Objects.t
should return green
Test returned green.
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>
Adds ExportRemoveFields to sysprefs.sql
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
