Bug 18329 - Batch record deletion broken

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>

Bug 12913: [QA Follow-up] Reword 'change item status' and add zero status

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>

Bug 12913 - wrong inventory results when notforloan filter is not used

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>

Bug 6979 - Fix Already in a transaction error

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 6979 - Handle multiple branches in non-auth_by_bin

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 6979 - Update tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug #6979

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>

Bug 17560 - DBRev 16.12.00.020

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

Bug 17560: Improve wordings

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>

Bug 17560: Improve strenght of hold existence test

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>

Bug 17560: Add a message when a patron checking in using the self checkout

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>

Bug 17560: Update the wording when a patron places a hold at the OPAC

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>

Bug 17560: Hold fee placement at point of checkout

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>

Bug 17560: Update current code

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>

Bug 17560: Add a third option to HoldFeeMode

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>

Bug 17736: Fix typo ->new vs ->next

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>

Bug 17736: [QA Follow-up] Script basket.pl is not Plack compliant

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>

Bug 17736: [QA Follow-up] We do not need GetItemHolds in acqui

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>

Bug 17736: [QA Follow-up] Crashes in basket.pl and parcel.pl

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>

Bug 17736: [QA Follow-up] Wrong biblionumber in acqui/parcel

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>

Bug 17736: [QA Follow-up] Solve warning from qa tools on Holds.t

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>

Bug 17736: [Follow-up] Resolve inherited AUTOLOAD for non-method errors

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>

Bug 17736: [Follow-up] Rename to current_holds

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>

Bug 17736: Remove C4::Reserves::GetReservesFromBiblionumber

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>

Bug 17736: Replace GetReservesFromBiblionumber with Koha::Biblio->holds

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>

Bug 17736: Add the Koha::Biblio->holds_placed_before_today method

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>

Bug 18173 - DBRev 16.12.00.019

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

Bug 18173: DBIx schema changes

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 18173: Remove DB field issues.return

The DB field issues.return has never really been used apparently.
I found the first occurrence of this field in
commit eac3a7b19a8aa7cda34aac396f5093c213a4aa5a
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>

Bug 18332: [QA Follow-up] Adjust slice call in last method

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>

Bug 18332: Add the Koha::Objects->last method

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>

Bug 15498 - DBRev 16.12.00.018

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

Bug 15498: [QA Follow-up] Additional changes

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>

Bug 15498: Fix conflict with bug 17394

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 15498: Fix show/hide csv profile list

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>

Bug 15498: Do not display sql csv profiles

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>

Bug 15498: Add ExportCircHistory and remove ExportWithCsvProfile

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>

Bug 15498: Let the user choose the CSV profile to export circ history

The way the export options are displayed at the bottom of the checkouts table
was not consistent.
Prior to this patch set, they are display if ExportRemoveFields or
ExportWithCsvProfile is set.
It does not make any sense, the user could want to export the checkouts in
iso2709 format without having to define a csv profile and fill the pref.

Moreover the behavior of this pref did not match its description: it's used as
a default CSV profile when exporting records from the export tools or the
command line.

This patch set adds a new pref ExportCircHistory and remove
ExportWithCsvProfile. The new pref is set if ExportWithCsvProfile or
ExportRemoveFields were set.
A new dropdown list with the CSV profile list will be displayed in the
export area, at the bottom of the checkouts table.

Note that now --csv_profile_id is mandatory for the export command line
(misc/export_records.pl) if the export format is csv.

Test plan:
0/ Do not execute the DB entry
1/ Clear both ExportWithCsvProfile and ExportRemoveFields prefs
2/ Execute the DB entry
3/ ExportCircHistory should not be set and the export options should not
be displayed at the bottom of the checkouts table.
4/ Remove the pref
  DELETE FROM systempreferences WHERE variable='ExportCircHistory';
and reinsert the previous one, with a value:
  INSERT INTO systempreferences (variable, value) VALUES
  ('ExportWithCsvProfile', 'something');
Execute the DB entry again
=> The now pref should be now set
5/ Export some checkouts using the CSV entry
6/ Note that the export tool and commandline script still work using the
csv format. You have to provide a --csv_profile_id option to make it
work.

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>

Bug 17971 (QA Followup) Clarify comment

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

Bug 17971: Add support for objects represented by fk

For instance an issue is not fetch from its fk but using the fk
itemnumber.
We need to support them.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17971: TT syntax for notices - Add support for plurals

On of the awesome things we will be able to do with the TT syntax is the support of plurals.

For instance we will be able to send a list of items, checkouts, etc. to the notice template.
That way we will get rid of our custom syntax like <<items.content>> or <item></item> for instance.

The existing code already has the playground for that but it is not used.

Basically the idea is to add a "loops" key which can contain a list of
object to retrieve from the DB and send to the template.
For instance:
    loops => { overdues => [ $itemnumber_1, .., $itemnumber_N ] }

will send a variable "overdues" to the template. It will contain the
Koha::Checkout objects relative to the id passed.

There is one quite big inconvenient to this approach so far: since we
are still supporting the historical syntax, the objects can be fetch by
a script, then the script will send the id to GetPreparedLetter which
will refetch them.
This must be improved, but I suggest to do that later.

Test plan:
  prove t/db_dependent/Letters/TemplateToolkit.t
should return green

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute

From C4::Letters::GetPreparedLetter:

    my $tables = $params{tables};
    my $substitute = $params{substitute};

    $tables || $substitute || $repeat
       or carp( "ERROR: nothing to substitute - both 'tables' and 'substitute' are empty" ),
          return;

So if the parameter tables or substitute is passed but does not contain anything, it will not warn as expected.

Test plan:
1/ Apply the patch with tests
2/ Confirm that they do not pass
3/ Apply this patch
4/ Confirm that the tests now pass

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>

Bug 17970: Add tests to highlight the problem

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>

Bug 17963: TT syntax for notices - Prove that AR_* are compatible

Nothing new here since bug 17962, the AR_* notice messages are quite
simple. They send the article_request, patron, biblio, biblioitem, item and
library linked to the article request.

All the fields from these 6 tables should still be accessible using the
TT syntax.

Test plan:
Define TT notice templates for AR_PENDING, AR_PROCESSING, AR_COMPLETED
or AR_CANCELED.

You should manage to create a template to generate the same result as
the historical syntax.

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>

Bug 18269: Move field mappings related code to Koha::FieldMapping[s]

The 3 subroutines GetFieldMapping, SetFieldMapping and
DeleteFieldMapping from the C4::Biblio module were only used from the
field mappings admin page.
They can easily replaced with new packages Koha::FieldMappings based on
Koha::Object[s]

Test plan:
Add and delete field mappings (admin/fieldmapping.pl, Home ›
Administration › Keyword to MARC mapping).
Add an existing mapping > Nothing should be added

Note that this page has not been rewritten and you will not get any
feedbacks, but it's not the goal of this page to improve 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>

Bug 18269: Add Koha::FieldMapping[s] packages

Tested both patches together, 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>

Bug 14146 - DBRev 16.12.00.017

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

Bug 14146: Add the new pref CumulativeRestrictionPeriods

Sponsored-by: Orex Digital
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 14146: Clean a bit and make the code understandable

The code was a bit weird and this patch cleans it a bit by renaming
variables and adding a variable.

Sponsored-by: Orex Digital
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 14146: Add the ability to cumulate restriction periods

At the moment the default behaviour is not to cumulate the restriction
periods but to apply the longest one.
This patch set creates a new syspref CumulativeRestrictionPeriods. If
on, the behaviour changes and the restriction periods are cumulated: the
days of the second restriction are added to the actual restriction
period.

We could add a new circulation rule instead of a syspref, but I am not
sure it's very useful to have such granularity for this behaviour (can
be changed if needed).

How it works:
Let's take 2 items, A and B.
A is returned with Na days late, and B Nb days late
The grace period is Ng and there is 1 day of suspension charge per day
of overdue
The suspension period is until day D = Na - Ng + Nb - Ng

I would have expected D = Na + Nb - Ng but it's how it worked before
this patch.

Test plan:
Create several overdue for a given patron
Do the checkins and confirm that the period are added if the pref is on.
If the pref is off, you should not get any changes in the existing behaviour.

Sponsored-by: Orex Digital
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 14146: Add tests for AddReturn + CumulativeRestrictionPeriods

Sponsored-by: Orex Digital
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17847: Remove C4::Koha::GetAuthvalueDropbox

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>

Bug 17847: Replace C4::Koha::GetAuthvalueDropbox with Koha::AuthorisedValues

The C4::Koha::GetAuthvalueDropbox subroutine does the same job as
Koha::AuthorisedValues->search
We should then replace the different calls to this subroutine to finally
remove it.
There were 2 calls to this subroutine:
- from the AuthorisedValues TT plugin (called from av-build-dropbox.inc
and members/housebound.tt)
- from the acqui/ajax-getauthvaluedropbox.pl ajax script

To make sure that this patchset does not introduce regressions, we will have
to test that the TT plugin and the ajax script still behave as before.

Test plan:
1/ Test acqui/ajax-getauthvaluedropbox.pl
- Link a fund to an authorised value category
- Create a new order
=> When you select a fund linked to AV category, the sort1 (and/or
sort2, depending on what you set) should be replaced with a dropdown
list populated with the authorised values
2/ Test av-build-dropbox.inc
- Create some authorised values for Bsort1
- Edit a patron
=> The sort1 should be a dropdown list populated with the Bsort1 AV
3/ Test members/housebound.tt
- Enable the housebound module (pref HouseboundModule)
- On the patron detail page, click on the "Housebound" tab
=> The frequency dropdown list should be populated with the different
HSBND_FREQ AV

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>

Bug 17844: Replace C4::Koha::get_notforloan_label_of with Koha::AuthorisedValues

This patch is more a bugfix than a refactoring.
Indeed the C4::Koha::get_notforloan_label_of behaviors were buggy:
1/ It does not display the opac description at the OPAC, but always the
staff description
2/ It does not care of the framework of the biblio, but retrieve the
first row of the marc_subfield_structure mapped with items.notforloan

These 2 bugs can easily be fixed using the
Koha::AuthorisedValues->search_by_koha_field

Steps to recreate the issues:
- Create 2 authorised value categories for not for loan (NFL1 and NFL2)
with the same values. Define a different description for the OPAC.
- Define link 952$7 to NFL1 for the default framework and to NFL2 for
the BK framework
- Create 2 bibliographic records (B1 using NFL1 and B2 using NFL2) with
2 items each (1 item should have a not for loan value)
- Go to the "Place a hold" view for this record.
- In the item list, you should see the not for loan value
=> The staff description of NFL1 will always be used, even for the OPAC

Test plan:
- Recreate the issues without this patchset
- Apply this patchset
- Recreate the steps to recreate the issues
=> The staff description of NFL2 should be displayed for the B2 item
=> The opac description of NFL2 should be displayed for the B2 item at
the OPAC

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>

Bug 17844: Remove C4::Koha::get_notforloan_label_of

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>

Bug 18258: (QA followup) Use Koha::Subscriptions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 18258: Add the Koha::Biblio->subscriptions method

Test plan:
  prove t/db_dependent/Koha/Biblios.t
should return 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>

Bug 18270: Do not fetch the MARC record when deleting an item

$record is never used later, the call is superfluous.

Test plan:
Quick glance at the code should be enough

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>

Bug 18058: Allow borrower_message_preferences to be truncated

borrower_message_preferences cannot be truncated because of the foreign.
DBMS fails with
  "Cannot truncate a table referenced in a foreign key constraint"

To avoid that we should remove the FK check and truncate the other table
as well.

I am wondering if we really need a truncate here
  DELETE FROM borrower_message_preferences;
should do the job, but leave it as it because of the param name.

Test plan
  perl misc/maintenance/borrowers-force-messaging-defaults --doit --truncate
Should no longer raise the error message

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>

Bug 18266: Fix internal error when paying fine for lost item without.. item

If a fine is created for a lost item but the itemnumber is not supplied,
the system will return it.
The item should not be mark as returned if there is no item linked to
the fine.

Test plan:
1. Turn StoreLastBorrower on
2. Create a manual invoice for a lost item, do not supply a barcode
3. Pay the fines 'Pay fines > Pay'

=> Without this patch applied you get
Can't call method "last_returned_by" on an undefined value at
/home/marc/koha/C4/Circulation.pm line 2188.

=> With this patch applied, you must not get the error.

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>

Bug 18124: Change the calls to generate and check CSRF tokens

The parameter change in Koha::Token should be applied to the calling
scripts.

Test plan:
Confirm that the different forms of the scripts modified by this patch
still work correctly.

Test the problematic behavior:
Open 2 tabs with in same user's session, go on the edit patron page
(memberentry.pl).
Log out and log in from the other tab.
Submit the form
=> Wrong CSRF token should be raised

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>

Bug 18124: [Follow-up] Handle default parameters in a sub

Adds a internal routine to handle default values for the parameters
id and secret.
Also adds a parameter session_id for generate_csrf and check_csrf. This
session parameter is combined with the id parameter when generating or
checking a token.

Test plan:
Run t/Token.t

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>

Bug 18124: Restrict CSRF token to user's session

Currently the CSRF token generated is based on the borrowernumber, and
is valid across user's session.
We need to restrict the CSRF token to the current session.

With this patch the CSRF token is generated concatenating the id
(borrowernumber) and the CGISESSID cookie.

Test plan:
Run t/Token.t

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>

Bug 18312: Fix export unless a file is supplied

Bug 18087 breaks export unless a file is supplied.

Can't use an undefined value as a HASH reference at
/home/vagrant/kohaclone/tools/export.pl line 75.

Test plan:
Export records using a file of id that is not a valid file (not txt or
csv)
Export records using a valid file
Export records without supplying a file

=> The export should work or fail as expected.

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 18144: Restore pieces of C4/Auth to make Google OpenID Connect work

By restoring some pieces of logic, with the name changed from $persona
to $emailaddress, the openid will work again for OPAC logins.

See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10988#c68
for an excellent test plan.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Did not test it, but trust in author and signoffer

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

Bug 13757 - DBRev 16.12.00.016

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

Bug 13757: (QA followup) Filter out non-editable params before storing

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>

Bug 13757: (QA followup) Exclude empty attributes from rendering if non-editable

In self registration opac displayable (and not editable) attributes are
displayed as empty. This an empty value is passed to the template for
creating an empty input and it shouldn't when the attribute is not
editable.

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>

Bug 13757: Attribute with value 0 should be stored

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (regression test) Attribute with value 0 should be stored

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: Better display for attr changes in members-update.pl

This patch changes the way changed attributes are displayed for the
staff user to make the decision to approve (or not) the changes.

Regards

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: Make K::P::Modifications->pending return K::P::Attribute

This patch makes Koha::Patron::Modifications->pending return
Koha::Patron:Attribute objects. They are not stored on the DB but only
live in memory on runtime.

members-update.pl is the only place this is used, and this way we have
all we need for better rendering the UI.

Tests are added for the changed API.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (QA followup) Fix non-editable attrs on failed save

When a field is not editable but displayable in the OPAC, and you submit
an incomplete/wrong update, those attributes are displayed as empty.

This patch fixes that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (QA followup) Make opac-memberentry.pl handle attrs deletion

The original code on this bug skipped empty-valued attributes. But
emptying attributes is the only way to tell the controller script that
the user wants to delete them.

This patch makes opac-memberentry.pl check the existence of attributes
sharing the code of the empty for the given patron, and it stores the
deletion on the Koha::Patron::Modification as needed. Otherwise
deletions got skipped.

To test:
- Verify setting/deleting attributes that are opac-editable and verify
the results are sound.

https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: Make Koha::Patron::Modification->store del empty attrs

This patch makes Koha::Patron::Modification->store delete the passed
attributes that contain empty values.

This is the way it currently works, as all opac-editable attributes are
presented to the end-user and they are allowed to delete them, and the
best way I found to represent the deletion on the modification request
is by setting it to the empty string. I chose this way because it is how
the staff interface handles it, so it is consistent.

To test:
- Apply this patch
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> SUCCESS: This time tests pass!
- Verify comment #70 on the bug is fixed now
- Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (regression tests) Empty attributes should delete existing

This patch introduces tests for the required functionality.

To test:
- Run:
  $ prove t/db_dependent/Koha/Patron/Modifications.t
=> FAIL: The current code doesn't work like that

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
https://bugs.koha-community.org/show_bug.cgi?id=13737

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (QA followup) Check DB structure before altering table

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (followup) Fix authorized value display when opac_display & not opac_editable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (followup) Regression tests for ->approve changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (followup) Only touch opac_editable attributes

As reported by Owen, the members-update.pl was showing every attributes
the patron has (display issue) instead of showing only those affected by
the changes.

This patch fixes this by filtering the patron's attributes by opac
editability.

It also fixes Koha::Patron::Modification->approve so it only clears the
attributes with the updating 'code' and leaves the others untouched.
As its been coded so far (until someone refactors it all) the
Koha::Patron::Modification object needs to contain all the attributes
for a specific code. And it comes from parsing the UI's input.

Tests for Koha::Patron::Modification->approve to come.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (followup) Staff interface changes

This patch adds proper extended attributes display and handling on the
patron modifications moderation page (members-update.pl).

It also adds changes checking to the opac-memberentry.pl page so it
only saves a modification request if there are changes (it only checked
regular fields and not the extended ones).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: (followup) Remove warnings

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: Add extended attributes to the patron modification

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: DBIC update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: OPAC changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: DB update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 13757: Add the option to set patron attributes editable in the OPAC

Note: this is a squashed version of the original patchset, because it was needed

This patch adds an opac_editable property of borrower attribute types
that can be set in the interface. I'm removing work on OPAC and will
refactor it, keeping the author attribution.

Test plan:
  1. Repeat the following with a new and existing borrower attribute
     type:
  2. Verify that "Editable in OPAC" can only be checked if "Display in
     OPAC" is checked.
  3. Verify that this new property is correctly saved.

Signed-off-by: Aleisha <aleishaamohia@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17792: Add opac_editable and opac_display methods

This patch adds two methods to the Koha::Patron::Attribute:

- opac_display
- opac_editable

Both method just check the corresponding Koha::Patron::Attribute::Type
and return the values for those attributes. This is useful to avoid
checking that manually on the controller scripts.

To test:
- Run:
  $ prove t/db_dependent/Koha/Patron/Attributes.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17792: Introduce Koha::Patron::Attribute(s)

This patch introduces stub Koha::Object(s) for handling patron attributes.

Edit: amended the POD to fix C&p mistake

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17755: (followup) Override ->search to allow filtering by branchcode

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17755: (QA followup) Return $self when appropriate

As failure situations raise exceptions that should be handled outside
the object class, methods should return $self so successive calls can be
chained nicely.

This patch makes methods return $self and adjusts the tests to reflect
this change.

Make sure tests pass:
- Run:
  $ prove t/db_dependent/Koha/Patron/Attribute/Types.t
=> SUCCESS: Tests return green
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17755: Introduce Koha::Patron::Attribute::Type(s)

This patch introduces the Koha::Object-based classes for handling
patron attribute types.

It also adds branch limitation handling to the
Koha::Patron::Attribute::Type class.

It is built on top of the new Koha::Object::Library::Limit class
that extends Koha::Object so it handles library limits.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17755: Unit tests

This patch introduces unit tests for Koha::Object::Library::Limit. It is
done this way because it needs to be instantiated to be usable.

To test:
- Run:
  $ prove t/db_dependent/Koha/Patron/Attribute/Types.t
=> SUCCESS: Tests pass
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 17755: Introduce Koha::Object::Limit::Library

This patch introduces a new class for extending Koha::Object using
multiple inheritance. It cannot be used standalone, it needs to be
used in Koha::Object implementations like this:

use base qw( Koha::Object Koha::Object::Limit::Library );

Its goal is to provide a single way and place to deal with this common
pattern in Koha's codebase.

As it happened with Koha::Object, that needed to be tested in a real object
class, this work was done on top of Koha::Patron::Attribute::Type implementation
and it is fully covered by the tests that are introduced for it.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 18066: Update Schema

This patch basically reverts 85a21dd9715518746dd93defd4803a81d23a5b18

Bug 18066: Add the new geolocation column to kohastructure.sql

Fix updatedatabase.pl syntax error

Bug 15854: Use a READ and WRITE LOCK on message_queue

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.html
https://dev.mysql.com/doc/refman/5.7/en/lock-tables.html
https://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>

Bug 15854: Simplify the code to limit race conditions

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>

Bug 17605: [MASTER] Add currency to orders generated by quotes

Sets the vendors currency in the edi generated order
The currency used is agreed between vendor and library
and the value in the quote is optional

Edifact potentially allows the currency to be overwitten
by specifying another currency in the CUX segment but
we know currently of no supplier doing this

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>