Benjamin Rokseth [Tue, 13 Dec 2016 12:38:19 +0000 (13:38 +0100)]
Bug 17758 - SIP checkin does not handle holds correctly
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>
Fridolin Somers [Fri, 24 Mar 2017 15:07:16 +0000 (16:07 +0100)]
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>
Marcel de Rooy [Tue, 28 Mar 2017 14:39:25 +0000 (16:39 +0200)]
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>
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>
Jonathan Druart [Fri, 10 Mar 2017 13:59:41 +0000 (10:59 -0300)]
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>
Jonathan Druart [Sun, 6 Nov 2016 19:41:47 +0000 (19:41 +0000)]
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>
Jonathan Druart [Sun, 6 Nov 2016 18:55:22 +0000 (18:55 +0000)]
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>
Jonathan Druart [Thu, 3 Nov 2016 16:12:38 +0000 (16:12 +0000)]
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>
Jonathan Druart [Thu, 3 Nov 2016 13:20:17 +0000 (13:20 +0000)]
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>
Marcel de Rooy [Fri, 6 Jan 2017 12:36:35 +0000 (13:36 +0100)]
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>
Marcel de Rooy [Fri, 6 Jan 2017 12:22:07 +0000 (13:22 +0100)]
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>
Marcel de Rooy [Fri, 6 Jan 2017 11:21:56 +0000 (12:21 +0100)]
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>
Marcel de Rooy [Fri, 6 Jan 2017 11:03:27 +0000 (12:03 +0100)]
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>
Marcel de Rooy [Fri, 6 Jan 2017 10:45:17 +0000 (11:45 +0100)]
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>
Marcel de Rooy [Fri, 6 Jan 2017 10:21:10 +0000 (11:21 +0100)]
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>
Marcel de Rooy [Fri, 6 Jan 2017 09:48:48 +0000 (10:48 +0100)]
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>
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>
Jonathan Druart [Wed, 7 Dec 2016 02:29:01 +0000 (02:29 +0000)]
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>
Jonathan Druart [Wed, 7 Dec 2016 15:38:38 +0000 (16:38 +0100)]
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>
Jonathan Druart [Mon, 27 Feb 2017 09:41:55 +0000 (09:41 +0000)]
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>
Jonathan Druart [Fri, 24 Mar 2017 01:03:19 +0000 (22:03 -0300)]
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>
Jonathan Druart [Wed, 2 Nov 2016 10:31:43 +0000 (10:31 +0000)]
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>
Jonathan Druart [Wed, 2 Nov 2016 08:42:11 +0000 (08:42 +0000)]
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>
Jonathan Druart [Wed, 6 Jan 2016 16:36:54 +0000 (16:36 +0000)]
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>
Jonathan Druart [Wed, 2 Nov 2016 08:28:01 +0000 (08:28 +0000)]
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>
Jonathan Druart [Tue, 17 Jan 2017 15:18:54 +0000 (16:18 +0100)]
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>
Jonathan Druart [Sat, 21 Jan 2017 13:26:27 +0000 (14:26 +0100)]
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>
Jonathan Druart [Sat, 21 Jan 2017 13:13:49 +0000 (14:13 +0100)]
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>
Jonathan Druart [Sat, 21 Jan 2017 13:13:36 +0000 (14:13 +0100)]
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>
Jonathan Druart [Mon, 16 Jan 2017 13:46:12 +0000 (14:46 +0100)]
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>
Jonathan Druart [Mon, 13 Mar 2017 21:17:13 +0000 (18:17 -0300)]
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>
Jonathan Druart [Tue, 14 Mar 2017 21:10:08 +0000 (18:10 -0300)]
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>
Jonathan Druart [Wed, 30 Nov 2016 19:33:44 +0000 (19:33 +0000)]
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>
Jonathan Druart [Wed, 4 Jan 2017 14:41:22 +0000 (15:41 +0100)]
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>
Jonathan Druart [Wed, 4 Jan 2017 12:43:27 +0000 (13:43 +0100)]
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>
Jonathan Druart [Wed, 4 Jan 2017 10:38:34 +0000 (11:38 +0100)]
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>
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>
Jonathan Druart [Mon, 13 Mar 2017 18:27:06 +0000 (15:27 -0300)]
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>
Jonathan Druart [Wed, 15 Mar 2017 12:30:19 +0000 (09:30 -0300)]
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>
Jonathan Druart [Tue, 14 Feb 2017 14:39:48 +0000 (14:39 +0000)]
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>
Jonathan Druart [Tue, 21 Mar 2017 15:24:28 +0000 (12:24 -0300)]
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>
Jonathan Druart [Wed, 15 Feb 2017 16:14:13 +0000 (17:14 +0100)]
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>
Marcel de Rooy [Thu, 16 Feb 2017 10:59:12 +0000 (11:59 +0100)]
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>
Jonathan Druart [Wed, 15 Feb 2017 16:14:13 +0000 (17:14 +0100)]
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>
Jonathan Druart [Tue, 21 Mar 2017 13:52:42 +0000 (10:52 -0300)]
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>
Mark Tompsett [Mon, 20 Feb 2017 06:37:49 +0000 (01:37 -0500)]
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: (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.
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>
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>
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>
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>
Jesse Weaver [Wed, 14 Dec 2016 15:30:10 +0000 (12:30 -0300)]
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>
Jesse Weaver [Wed, 14 Dec 2016 15:29:10 +0000 (12:29 -0300)]
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>
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>
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>
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>
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>
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>
Jonathan Druart [Thu, 9 Feb 2017 11:13:07 +0000 (12:13 +0100)]
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>
projects / koha.git / log