Commit graph

6218 commits

Author SHA1 Message Date
cb58b11f67 Bug 18070: Extend sub merge to remove fields for deleted authorities
In order to accomplish this, we need to add some additional checks in
the merge routine. The actual change to remove the field, is quite
small.

Furthermore, we need to add a merge call in DelAuthority and adjust
the merge cron job accordingly.

The change is well supported by additional tests, including a simulation
of postponed removal via cron, if dontmerge is enabled.

Note: Deleting an authority with linked biblios is tested on the next
patch.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Delete an authority without linked biblios from the Authorities
    module. If the indexer is not fast enough, wait a bit and refresh to
    verify that the authority is gone on authorities-home.pl.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-03-03 18:12:09 +00:00
286be46e8a Bug 16966: Koha::Patrons - Move GetBorrowersWithIssuesHistoryOlderThan to search_patrons_to_anonymise
The C4::Members::GetBorrowersWithIssuesHistoryOlderThan subroutine is supposed
to return the patrons with an issue history older than a given date.

It would make more sense to return a list of Koha::Patrons.

On the way, the code from AnonymiseIssueHistory will be moved as well to
anonymise_issue_history.

Note that these 2 subroutines are strongly linked: one is used to know the
number of patrons we will anonymise the history, the other one is used to
anonymise the issues history. The problem is that the first one is not used to
do the action, but only for displayed purpose.

In some cases, these 2 values can differ, which could be confusing.
Case 1:
The logged in librarian is not superlibrarian and IndependentBranches is set:
if 2+ patrons from different libraries match the date parameter, the interface
will display "Checkout history for 2 patrons will be anonymized", when actually
only 1 will be.
Case 2:
If 2+ patrons match the date parameter but one of them has his privacy set to
forever (privacy=0), the same issue will appear.

This patch moves the code from C4::Members::GetBorrowersWithIssuesHistoryOlderThan
to Koha::Patrons->search_patrons_to_anonymise and from
C4::Circulation::AnonymiseIssueHistory to
Koha::Patrons->anonymise_issue_history

Test plan:
1/ Confirm the 2 issues and make sure they are fixed using the Batch
patron anonymization tool (tools/cleanborrowers.pl)
2/ At the OPAC, use the 'Immediate deletion' button to delete all your
reading history (regardless the setting of the privacy rule)
3/ Use the cronjob script (misc/cronjobs/batch_anonymise.pl) to
anonymise patrons.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-03-03 17:20:03 +00:00
Francesco Rivetti
35e3678f13 Bug 17941 avoid scanning the full cartesian product
when a item match a borrower, there is no point in checking the
other borrowers

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>
2017-03-03 16:50:26 +00:00
Kyle M Hall
8255344215 Revert "Bug 12461 - Add patron clubs feature"
This reverts commit 4f1eefdbb8.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-26 20:41:27 -05:00
4f1eefdbb8 Bug 12461 - Add patron clubs feature
This features would add the ability to create clubs which patrons may be
enrolled in. It would be particularly useful for tracking summer reading
programs, book clubs and other such clubs.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
2017-02-23 19:42:36 +00:00
Srdjan
e7aff8ea0d Bug 16034 follow-up: added WebService::ILS to PerlDependencies
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-21 19:58:21 +00:00
Luke Honiss
f9a4f5edb8 Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC'
--TEST PLAN--
1) View a subscription with no history end date
2) Search for an item with a subcription in OPAC
3) Under subscription tab click more details
4) The end date will be the current date
5) Apply patch and refresh
6) The end date will not be shown

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-21 19:55:13 +00:00
ad707d0274 Bug 17968: Remove useless variable $item_format in C4::Overdues::parse_overdues_letter
The variable $item_format is not used and should be removed from this
subroutine.

Moreover it the letter parameter, but it is never sent to this
subroutine. letter_code is expected instead.

Test plan:
No test plan, just read the code and `git grep `

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-17 15:43:30 +00:00
Mark Tompsett
d5446ed480 Bug 17846: Remove get_infos_of declaration
The function which was removed was still being exported.
This removes it completely.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-17 15:35:40 +00:00
8c8b17b114 Bug 17846: Remove C4::Koha::get_infos_of
This subroutine does not longer make any senses. The call to
get_infos_of can be replaced with $dbh->selectall_hashref.
The third argument of this subroutine was never used.

Test plan (for developer only):
Compare the 2 codes and confirm that they are equivalent

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-17 15:35:39 +00:00
a3d2273b35 Bug 17627: Move C4::Koha::GetItemTypesByCategory to Koha::ItemTypes
C4::Koha::GetItemTypesByCategory can be easily replaced with
Koha::ItemTypes->search({ searchcategory => ? });

So let's replace it where it is used.

Test plan:
Make sure this patch does not break the test plan of bug 10937

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-17 15:31:32 +00:00
2b9662e0ca Bug 17990: Refactor Perl module versions check
The code is duplicated, variable are not set ($_), code is hard to read,
not covered by tests and the subroutine has 2 completely different
behaviors depending on the presence of the "module" parameter.
No need more ti rewrite it.

Test plan:
- Use koha_perl_deps.pl with the different options (-u -m -a -i)
- Go on the about page, "Perl modules" tab

You should not see any differences from before and after this patch

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-17 12:13:44 +00:00
c6e0d42521 Bug 17962: TT syntax for notices - Prove that ACQ_NOTIF_ON_RECEIV is compatible
To make ACQ_NOTIF_ON_RECEIV TT compatible, we need to expose data from
the aqorders table. We already have a package for it in the Koha
namespace but it is based on Koha::Object[s].
The other path creates dummy Koha::Tmp::Order[s] packages to make it
usable. Of course we should use a valid Koha::Acquisition::Order[s]
based on Koha::Object, but it's outside the scope of this bug report.

This notice template is quite simple, and it's a good one to start.
From C4::Acq::NotifyOrderUsers, GetPreparedLetter is called with 4
elements: the library, the patron to notify, the biblio and the order
information.
Note that prior to this patch aqorders was filled from GetOrder, which
retrieved a lot of information from the acquisition table (aqbasket,
aqbookseller). The idea with the TT syntax is to access the data from
where it really exists. So if a user wants to display the basket name,
[% order.basket.basketname %] should be used instead.
Note that this will not work at the moment, the basket method is not
defined in the order package.

However the basic template should work as before.
The test added to TemplateToolkit proves that.

Test plan:
Use the default ACQ_NOTIF_ON_RECEIV to notify a patron that an order has
been received.
That generated template should be exactly the same as prior to this
patch.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-17 11:43:46 +00:00
5c1daf010e Bug 17982: Fix the use of uniq in sub themelanguage
Doing uniq( \@themes ) is useless. It will just return to you the only
reference you gave it.
List::MoreUtils::uniq requires a list instead of an arrayref.
So it is a trivial fix that makes sub themelanguage return one theme instead
of three themes like [ 'prog', 'prog',  'prog' ].
Note that Template->new inserts one or two include paths to TT for each of
these three identical themes.

Test plan:
[1] Run t/db_dependent/Templates.t (should no longer fail)
[2] Run t/db_dependent/Auth.t (triggering themelanguage)
[3] Open a page on OPAC or intranet. (Did you restart Plack?)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

EDIT (Marcel): Amended test plan for additional unit test.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-14 14:09:52 +00:00
ed5cb5fc97 Bug 18089 - All XSLT testing singleBranchMode = 0 fails to show even if install has only 1 branch
Due to the way it has been implemented, singleBranchMode is set to an
empty string rather than 0 if there is only one branch. This causes any
block that tests for singleBranchMOde to be 0 to never appear.

Test Plan:
1) Apply this patch set
2) prove t/XSLT.t

Signed-off-by: Jenny Schmidt <jschmidt@switchinc.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-14 14:07:02 +00:00
ad38b24308 Bug 18014: AddAuthority should respect AUTO_INCREMENT
Instead of using the MAX(authid)+1 logic, AddAuthority should just save
the record and get the new id. The authid column is an autoincrement.

This eliminates problems where a newly assigned authid also refers to a
previously deleted record. (And it will not cause problems when refining
the dontmerge functionality on report 9988.)

Note: ModAuthority also calls AddAuthority to update an existing record; in
that case we should not create a new record even if the record should not
be found any more (which should be exceptional).

This patch also simplifies handling of 001 in the authority record: in all
cases this field is updated now; no need to check its contents.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t
[2] Add a new authority record via the interface

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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>
2017-02-14 14:01:11 +00:00
137cbd8d09 Bug 17512: Improve handling dates in C4::Items
This is a follow-up on the internal server error on 0000-00-00 in the items
column onloan. This patch deals with preventing to have such dates at all
in the date fields of items.

It is accomplished by:
[1] Adding a (private) subroutine _mod_item_dates. It takes an item hash
    and replaces date values if needed.
[2] AddItem and ModItem call _koha_new_item resp. koha_modify_item. In these
    routines a call to the new _mod_item_dates is inserted.
[3] Although the routine is actually private, I have added some unit tests
    to Items.t.

Test plan:
[1] Add a new item. Fill a correct date in dateaccessioned and an invalid
    date in Price effective from (=replacementpricedate).
[2] Verify that dateaccessioned is saved correctly and replacementpricedate
    is still null (does not contain 0000-00-00).
[3] Edit the item again. Fill some text in dateaccessioned and put a correct
    date in replacementpricedate. Verify the results.
[4] Run t/db_dependent/Items.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>
2017-02-14 13:57:49 +00:00
Lari Taskula
21ac9fcdc2 Bug 16387: Fix default shortened loan period time
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>
2017-02-07 17:54:21 +00:00
Mirko Tietgen
844cf7a748 Bug 18015 - On shelf holds allowed > "If all unavailable" ignores notforloan
If in the circ rules matrix you set "On shelf holds allowed" to "If all unavailable",
items with status "Not for loan" are considered available and break the functionality.

Test plan:

- Set "On shelf holds allowed" to "If all unavailable" for your patron and item
  category (or everyone and everything)
- Have two items for a record. Check out one
- Set 7 - Not for loan: "Not For Loan" for the second item
- Try to place a hold. Does not work.

- Apply the patch
- Try to place a hold. Should work now.

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-07 17:51:51 +00:00
Chris Nighswonger
544cf17d6f Bug 18044: Label Batches not displaying
SQL expects lists to be comma separated. A trailing comma must also
be avoided.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-07 17:46:00 +00:00
Dobrica Pavlinusic
4740438b41 Bug 17775 - Add new user with LDAP not works under Plack
This patch fixes internal server error:

Undefined subroutine &C4::Auth_with_ldap::AddMember called at /srv/koha_ffzg/C4/Auth_with_ldap.pm line 213.

It occurs only under plack, and it's strange since C4::Members
does EXPORT AddMember and we are importing it into Auth_with_ldap.pm
(and it does work under CGI).

Signed-off-by: Liz Rea <liz@catalyst.net.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
I did not test but trust author and signoffer. The change cannot hurt.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-07 17:45:13 +00:00
Blou
1017edad1c Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value
This fixes the remaining fields from serials-edit.pl that were seeing their previously entered values
be oblitarated with each new edit.  The fields associated to a plugin (dateaccessioned and barcode) were
always displaying <empty> with each new edit, losing the previous effort.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-07 17:43:48 +00:00
Blou
4cdcdb3cb5 Bug 15030 - Certain values in serials' items are lost on next edit
When editing serials subscription, we can edit them but some values are not pulled from the DB correctly to be put in the edit box.  If not noticed, the value will be overwritten on the next save.

Test:
- Create a subscription
- Edit itemcallnumber (952o?) and make sure to have a different value than the default one.
- Save.
- Edit it again
- The saved value is not there.

This is true for itemcallnumber and a few other fields.

This was caused by calls to ->field($subfield).  This would always fail, of course.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-02-07 17:43:48 +00:00
Katrin Fischer
40cb8e3b75 Bug 17902: Follow-up fixing SQL statement
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 13:02:57 +00:00
f42dbd67d1 Bug 17902: Fix possible SQL injection in serials editing
/cgi-bin/koha/serials/serials-edit.pl?serstatus=*/+,2,3,'2016-12-12','2016-12-12',6,'jjj7','jjj8'%20--%20-&subscriptionid=1+and+1%3d2+Union+all+select+111+/*

The SQL query is not constructed correctly, placeholders must be used.
Subscription id and status list can be provided by the user.

This vulnerability has been reported by MDSec.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 13:02:56 +00:00
4dc7c32a3d Revert "Bug 17902: Fix possible SQL injection in serials editing"
This reverts commit 904716f581.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 12:12:08 +00:00
904716f581 Bug 17902: Fix possible SQL injection in serials editing
/cgi-bin/koha/serials/serials-edit.pl?serstatus=*/+,2,3,'2016-12-12','2016-12-12',6,'jjj7','jjj8'%20--%20-&subscriptionid=1+and+1%3d2+Union+all+select+111+/*

The SQL query is not constructed correctly, placeholders must be used.
Subscription id and status list can be provided by the user.

This vulnerability has been reported by MDSec.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 12:08:31 +00:00
e2d1bafa22 Revert "Bug 17902: Fix possible SQL injection in serials editing"
This reverts commit 8924439054.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:52:56 +00:00
8924439054 Bug 17902: Fix possible SQL injection in serials editing
/cgi-bin/koha/serials/serials-edit.pl?serstatus=*/+,2,3,'2016-12-12','2016-12-12',6,'jjj7','jjj8'%20--%20-&subscriptionid=1+and+1%3d2+Union+all+select+111+/*

The SQL query is not constructed correctly, placeholders must be used.
Subscription id and status list can be provided by the user.

This vulnerability has been reported by MDSec.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:52:38 +00:00
93cc0956a9 Bug 9569: Security patch for AutoLocation
If a patron is not allowed to access the staff interface because its IP
address in the authorised range of IPs, the cookie should not contain
the CGISESSID.
If it is, the patron is logged in and will be able to access the staff
interface if he reload the page (or hit another one).

Test plan:
Confirm the that AutoLocation feature is now working as expected.

Note: It seems that this feature has never really worked as intended.
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:25:06 +00:00
af0af36bb9 Bug 9569: Do not check the IP for login at the OPAC
At the OPAC, the AutoLocation feature should not be taken into account:
login to the OPAC from outside the IP range should work

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:25:06 +00:00
acabdc87c9 Bug 9569: AutoLocation should not depend on IndependentBranches
Those 2 prefs can be independent and it does not make sense to consider
AutoLocation only if IndependentBranches is set.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:25:05 +00:00
a8fdac38d8 Bug 9569: Fix AutoLocation - handle .* for subnets
The example in branches.tt is:
  Can be entered as a single IP, or a subnet such as 192.168.1.*

But actually the regex in C4::Auth does not handle subnets.

Test plan:
0/ Apply all the patches
1/ Switch AutoLocation on
2/ Define a subnet (192.168.0.* if your ip is like 192.168.0.X) in the IP
range of your library
3/ Log in on the staff interface
=> Should work

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:25:05 +00:00
b0bb1b0aa6 Bug 17904: Fix possible SQL injection in late orders
To recreate:
/cgi-bin/koha/acqui/lateorders.plop=send_alert&ordernumber=1)and%20(select*from(select(sleep(20)))a)--%20&letter_code=0

Notice the delay.

The SQL query is not constructed correctly, placeholders must be used.

This vulnerability has been reported by MDSec.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:22:33 +00:00
179ff58b09 Bug 17903: Fix possible SQL injection in serial claims
To recreate:
/cgi-bin/koha/serials/claims.pl?serialid=1)and%20(select*from(select(sleep(20)))a)--%20&letter_code=0

Notice the delay.

The SQL query is not constructed correctly, placeholders must be used.

This vulnerability has been reported by MDSec.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:21:19 +00:00
a70980d825 Bug 17900: Fix possible SQL injection in patron cards template editing
To recreate:
/cgi-bin/koha/patroncards/edit-template.pl?op=edit&element_id=23%20and%201%3d2+union+all+select+1,user(),@@version+--%20

Look at the Profile dropdown list.

To fix this problem and to make sure it does not appears anywhere else
in the label and patroncards modules, I have refactored the way the
queries are built in C4::Creators::Lib
Now all of the subroutine takes a hashref in parameters with a 'fields'
and 'filters' parameters.
From these 2 parameters the new internal subroutine _build_query will
build the query and use placeholders.

Test plan:
1/ Make sure you do not recreate the vulnerability with this patch
applied.
2/ With decent data in the labels and patroncards modules, compare all
the different view (undef the New and Manage button groups) with and
without this patch applied.
=> You should not see any differences.

This vulnerability has been reported by MDSec.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:19:55 +00:00
4ff78a9a0d Bug 17986: Perl dependency evaluation incorrect
It looks like I made a copy/paste error in a previous patch.

While the fix was working when you pass the param "module" to
version_info, it wasn't populating the version correctly
for the "all" param, which causes koha_perl_deps.pl to
think all OK modules actually need an upgrade.

TEST PLAN

0) Be on a system where you know your Koha Perl dependencies are
mostly up-to-date

1) Run ./koha_perl_deps.pl -a -c
2) Note that most modules say they need an upgrade even when
the installed version is the same as the minimum version

3) Apply patch

4) Run ./koha_perl_deps.pl -a -c
5) Note that most moduls say they're OK, especially when the
installed version is the same or greater than the minimum version

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Running koha_perl_deps.pl -u convinced me.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-30 11:18:27 +00:00
bcf9fdafab Bug 17588: ->get_issues has been replaced with ->checkouts
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 14:25:35 +00:00
90f9a3c6ac Bug 17588: get_account_lines->get_balance has been replace with account->balance
On previous bugs

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 14:25:35 +00:00
45cee0cec8 Bug 17588: Koha::Patrons - Move GetMemberIssuesAndFines
The GetMemberIssuesAndFines subroutine used to retrieve the issues,
overdues and fines for a given patron. Most of the time, only 1 or 2 of
these values were used.
This patch removes this subroutine and uses the new get_issues,
get_overdues and get_balance method from Koha::Patron and Koha::Account::Lines.

Test plan:
1/ Add overdues, issues and fines to different patrons
2/ On the checkout, checkin and patron search result and the patron
detail pages, these 3 informations, if displayed before this patch, must be
correctly displayed.
3/ Use the batch patron deletion tool and make sure that patrons with a
balance > 0 are not deleted

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 14:25:34 +00:00
42bef19fe9 Bug 17824: Remove C4::Members::GetBorrowersWhoHaveNeverBorrowed
This subroutine is no longer in used and can be removed.

Test plan:
  git grep GetBorrowersWhoHaveNeverBorrowed
must not return any results

NOTE: grep -i getborrowerswhohave `find . -type f`
      works well enough to find the cleanborrowers.pl too.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 14:24:09 +00:00
158442eb9e Bug 17501: Remove Koha::Upload::get from Koha::Upload
The get routine actually returns records from uploaded_files. It should be
possible to replace its calls by direct calls of Koha::UploadedFiles.

This patch is the crux of this patch set. It deals with all scripts that
use Koha::Upload.

In the process we do:
[1] Add a file_handle method to Koha::UploadedFile. This was previously
    arranged via the fh parameter of get.
[2] Add a full_path method to UploadedFile. Previously returned in the
    path hash key of get. (Name is replaced by filename.)
[3] Add a search_term method too (implementing get({ term => .. }).
    This logic came from _lookup.
[4] Add a keep_file parameter to delete method. Only used in test now.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Go to Tools/Upload. Add an upload, download and delete.
[3] Add another public upload , search for it.
    Use the hashvalue to download via opac with URL:
        cgi-bin/koha/opac-retrieve-file.pl?id=[hashvalue]
[4] Go to Tools/Stage MARC for import. Import a marc file.
[5] Go to Tools/Upload local cover image. Import an image file.
    Enable OPACLocalCoverImages to see result.
[6] Test uploading a offline circulation file:
    Enable AllowOfflineCirculation, and create a koc file (plain text):
    Line1: Version=1.0\tA=1\tB=2
    Line2: 2016-11-23 16:00:00 345\treturn\t[barcode]
    Note: Replace tabs and barcode. The number of tabs is essential!
    Checkout the item with your barcode.
    Go to Circulation/Offline circulation file upload.
    Upload and click Apply directly.
    Checkout again. Repeat Offline circulation file upload.
    Now click Add to offline circulation queue.
[7] Connect the upload plugin to field 856$u.
    Enable HTML5MediaEnabled.
    Upload a webm file via the plugin. Click Choose to save the URL,
    and put 'video/webm' into 856$q. Save the biblio record.
    Check if you see the media tab with player on staff detail.
    (See also: Bug 17673 about empty OPACBaseURL.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 14:20:05 +00:00
6ddd4b2f5f Bug 17913: [Follow-up] Fix duplicate $9s after merging in loose mode
We need to add $9 to the skip_subfields hash too. Formerly, it was
added to $exclude as well.
Thanks, Julian, for catching this one.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 13:55:13 +00:00
dd1c63230f Bug 17913: Run perltidy on the inner foreach loop
Kept the same number of lines.
You could verify with diff -w.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] As the last patch in this series, also test the interface:
    Set AuthorityMergeMode to loose. Set dontmerge to Do.
    Modify an authority record attached to multiple biblios.
    Edit a subfield, clear a subfield and add a subfield.
    Save. Wait a bit for the merge and Zebra update.
    Verify that the changes are merged properly into biblio records.
[3] Repeat step 2 with AuthorityMergeMode to strict.
    Remember that this affects the extra subfields in biblio records.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 13:55:12 +00:00
28b74224d3 Bug 17913: We always need some housekeeping
Remove some commented warnings
Remove the commented old code at the end of sub merge
Explicitly set merge mode in the first subtest
Move the return to loose mode from the second subtest to the third

Test plan:
Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 13:55:12 +00:00
ede08d725e Bug 17913: Remove possible duplicates in strict merge mode
Since strict mode does not allow additional subfields that would make
identical fields linked to the same authority different, there is no
need to keep them while merging.

We achieve this goal by simply:
[1] Count the number of same fields linked to mergefrom in strict mode to
    eliminate duplicates.
[2] Replaces the if-statement on auth_number by a next. (Tidy follows.)

Test plan:
Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 13:55:12 +00:00
681ae8430e Bug 17913: Do not keep a cleared subfield in loose merge mode
If you modify an authority and clear a specific subfield, you expect that
merge respects your edit and clears this subfield too in the biblio
records. It does in the new strict mode, but it does not yet in the
default loose mode.

This patch fixes that by adjusting the code around $exclude so that it
uses a new hash skip_subfields, built from the reporting tags from the old
and the new authority record.

This is supported again by some changes in the unit test.

Test plan:
Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 13:55:11 +00:00
8cde85451e Bug 17913: Fix the new field tag in merge when changing type
Originally aimed for 9988, adjusted for this report.

Old behavior was: pick the first tag. This is definitely wrong.
If you (would) merge 610 to 611, you don't want to get a 111.

This patch resolves the problem by determining the new tag in a small
helper routine _merge_newtag, and corrects the position of the new field
in the MARC record with append_fields_ordered. Too bad that MARC::Record
does not have such a function; it looks like insert_fields_ordered, but
it is different in case of multiple fields with the same tag.

Note: These two small helper functions are not tested separately, since they
should not be called outside of merge. They are implicitly tested by the
adjusted tests in Merge.t.

Note: In adding tests for this fix, I chose to simplify compare_field_count
(no need for the pass parameter), and replace the pass parameter of sub
compare_field_order by an exclude parameter, a hash of fields to exclude in
counting fields.

Test plan:
Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 13:55:11 +00:00
Frédérick
79c81920ab Bug 17913: Use AuthorityMergeMode pref in sub merge
Original fix from a patch on bug 11315.
Amended by Marcel de Rooy January 2017.

Test plan:
If you set mode to loose, the test will still pass.
If you set mode to strict, one test will fail. (Fixed later.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 13:55:10 +00:00
ffe1f6555c Bug 17913: Use replace_with instead of insert_grouped_field
Original fix from a patch on bug 5572.
Amended by Marcel de Rooy January 2017.

Note: This does not yet resolve the field order when merging to another
auth type, but is a good start.

Test plan:
Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
2017-01-20 13:55:10 +00:00