Bug 17712 - Move availability calculation to the Koha namespace
Summary: Move availability calculation to the Koha namespace
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 16826
  Show dependency treegraph
 
Reported: 2016-12-02 11:14 UTC by Lari Taskula
Modified: 2017-08-25 10:11 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Diagram to proposal for new classes and their relations (206.12 KB, image/jpeg)
2016-12-02 11:14 UTC, Lari Taskula
Details
Diagram to proposal for new classes and their relations (241.14 KB, image/jpeg)
2016-12-02 14:58 UTC, Lari Taskula
Details
Bug 17712: Add new Koha::Exceptions (14.95 KB, patch)
2016-12-27 16:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Koha::Availability, a base for availability information (13.26 KB, patch)
2016-12-27 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Add base for item and biblio availability (23.18 KB, patch)
2016-12-27 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Centralize availability-related checks (145.13 KB, patch)
2016-12-27 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item availability for hold (63.61 KB, patch)
2016-12-27 16:20 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Biblio availability for hold (19.83 KB, patch)
2016-12-27 16:20 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item availability for checkout (25.44 KB, patch)
2016-12-27 16:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item and biblio availability in search context (29.49 KB, patch)
2016-12-27 16:21 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Add new Koha::Exceptions (14.95 KB, patch)
2017-02-13 12:17 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Koha::Availability, a base for availability information (13.26 KB, patch)
2017-02-13 12:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Add base for item and biblio availability (23.18 KB, patch)
2017-02-13 12:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Centralize availability-related checks (145.35 KB, patch)
2017-02-13 12:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item availability for hold (63.61 KB, patch)
2017-02-13 12:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Biblio availability for hold (19.92 KB, patch)
2017-02-13 12:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item availability for checkout (25.53 KB, patch)
2017-02-13 12:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item and biblio availability in search context (29.89 KB, patch)
2017-02-13 12:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Centralize availability-related checks (145.27 KB, patch)
2017-02-15 16:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item availability for hold (63.61 KB, patch)
2017-02-15 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Biblio availability for hold (19.92 KB, patch)
2017-02-15 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item availability for checkout (25.57 KB, patch)
2017-02-15 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item and biblio availability in search context (29.94 KB, patch)
2017-02-15 16:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Add new Koha::Exceptions (14.99 KB, patch)
2017-04-26 12:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Koha::Availability, a base for availability information (13.93 KB, patch)
2017-04-26 12:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Add base for item and biblio availability (23.18 KB, patch)
2017-04-26 12:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Centralize availability-related checks (145.64 KB, patch)
2017-04-26 12:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item availability for hold (63.61 KB, patch)
2017-04-26 12:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Biblio availability for hold (19.93 KB, patch)
2017-04-26 12:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item availability for checkout (25.57 KB, patch)
2017-04-26 12:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Item and biblio availability in search context (29.95 KB, patch)
2017-04-26 12:30 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17712: Add new Koha::Exceptions (15.06 KB, patch)
2017-05-10 13:39 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17712: Koha::Availability, a base for availability information (14.00 KB, patch)
2017-05-10 13:39 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17712: Add base for item and biblio availability (23.25 KB, patch)
2017-05-10 13:39 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17712: Centralize availability-related checks (145.71 KB, patch)
2017-05-10 13:40 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17712: Item availability for hold (63.68 KB, patch)
2017-05-10 13:40 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17712: Biblio availability for hold (20.00 KB, patch)
2017-05-10 13:40 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17712: Item availability for checkout (25.64 KB, patch)
2017-05-10 13:40 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17712: Item and biblio availability in search context (30.02 KB, patch)
2017-05-10 13:40 UTC, Benjamin Rokseth
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2016-12-02 11:14:14 UTC
Created attachment 57921 [details]
Diagram to proposal for new classes and their relations

Currently some availability-related calculations are scattered around in different modules (which alone is not bad) but also in Perl-scripts, like opac-reserve.pl that has some patron related checks, maxreserves and some other.

With this problem I think it is confusing for someone to figure out how availability is calculated. This also makes it difficult to implement something new that needs to find out different levels of availabilities. An example of "anything new" would be REST API, which needs to be able to tell different availabilities (biblio/item holdability, checkout availability both anonymously and with patron restrictions etc). As of now, if we simply ask CanBookBeReserved in REST controller, we are completely ignoring patron debarments and fines, and maxreserves system preference.

Another problem is that currently there is no unified way of reasoning unavailability between holds and checkouts. There are strings from CanItemBeReserved, then there are HASHrefs with additional parameters from CanBookBeIssued, then some simple checks that may be simply 1 or 0, like IsAvailableForItemLevelRequest. REST API should however return uniform error codes. Also CanItemBeReserved has the problem that it returns the first reason in case of unavailability, and not all of them, if there would be more. An end-user would probably want to know all problems at once instead of fixing one to only find out there is another.

My proposal is to move availability-related calculation and checks into Koha namespace. I propose adding a class, Koha::Availability, that is able to contain availability-related information. It would contain methods to easily find out availability status and any reasons to unavailabilities. To represent reasons for unavailability/additional messages/needs for confirmation, I propose objects instead of strings or hashes. These objects could be Koha::Exceptions. Availability calculation could be categorized into Koha::Availability::Calculation's subclasses. Answers to item availability could be located in Koha::Item::Availability::Hold and Koha::Item::Availability::Checkout.

This setup would let us have an unified way for handling availability the same way everywhere.

How would it work in practice? I will describe my idea with "ingredients" and "recipes":

- "ingredient": anything availability-related sub-check or sub-calculation (e.g. item withdrawn, reservesallowed in issuing rules, age restriction etc.) that returns a Koha::Exception::* if it spots a problem. These could be located in Koha::Availability::Calculation subclasses.

- "recipe": a set of ingredients that cooks an answer to question "is something available in some way", for example:
** "Q: does an anonymous user see an item available in search result? -> A: no, unavailability reason: Koha::Exceptions::Item::NotForLoan"
** "Q: is an item available for hold for this patron? -> A: yes, additional note: Koha::Exceptions::Item::NotForLoan with parameter code => 'Ordered'" 
** "Q: is an item available for checkout for this patron by a librarian -> A: possibly; librarian needs to confirm Koha::Exceptions::Patron::Debt, exception includes field max_outstanding => 10.00, current_outstanding => 15.00"
These could be located in Koha::Item/Biblio::Availability::Hold/Checkout

The ingredient would contain all logic to spot a problem. The recipe would use what ever ingredients it needs to, and decide if a problem is worth of unavailability, additional note or perhaps ask a confirmation. This is somewhat same as CanBookBeIssued, but the point is to move all calculations away from the recipe so that we can re-use the same logic in another availability recipe.

Any thoughts on this idea and current availability in general?
Comment 1 Marcel de Rooy 2016-12-02 12:36:58 UTC
I stumbled across item status also in bug 17624 in particular referring to the Availability line in the OPAC XSLT results.
Comment 2 Marcel de Rooy 2016-12-02 12:45:33 UTC
(In reply to Lari Taskula from comment #0)
> 
> Any thoughts on this idea and current availability in general?

Fist, I very much welcome the idea.

Just in first glance, I would not choose for an Availablity object. We also need to define what we mean with "available" exactly: available for loan for instance?

I would probably opt for Koha::Item->is_available methods and similar. Or perhaps is_available_for_loan, can_be_reserved etc. Same for Koha::Biblio.
Shared code in this regard might perhaps go in Koha::Util::xxx ?
Comment 3 Lari Taskula 2016-12-02 14:58:28 UTC
Created attachment 57923 [details]
Diagram to proposal for new classes and their relations
Comment 4 Lari Taskula 2016-12-02 16:30:54 UTC
(In reply to Marcel de Rooy from comment #2)
> Just in first glance, I would not choose for an Availablity object. We also
> need to define what we mean with "available" exactly: available for loan for
> instance?
Thanks for the comment Marcel! Indeed, availability needs a proper definition to have a meaning. Also considering the perspective; Available for hold to patron, by who? Patron themselves? A librarian? If a patron asks if they can place a hold, they get an unavailability reason for having too much fines. If a librarian asks the same for patron, they get the same reason but additionally the information that it is possible to override it by confirmation.

> I would probably opt for Koha::Item->is_available methods and similar. Or
> perhaps is_available_for_loan, can_be_reserved etc. Same for Koha::Biblio.
> Shared code in this regard might perhaps go in Koha::Util::xxx ?
Just curious, how would you represent the answer to availability (what would these return)? Initially I was considering the same solution, but the complexity of availability eventually made me think of putting them it into own classes.

I think that all types of availability answers should always be represented the same, uniform way. It could be a HASHref like in CanBookBeIssued, but I would prefer an object that contains the availability information (Koha::Availability in my proposal) but doesn't define and care about the type of availability; it exists to ensure that we always have the same operations on availability information (to contain yes/maybe/no value to availability, to contain and return additional notes, reasons to ask for confirmation and unavailabilities).
Comment 5 Marcel de Rooy 2016-12-05 12:34:48 UTC
(In reply to Lari Taskula from comment #4)

> Just curious, how would you represent the answer to availability (what would
> these return)? Initially I was considering the same solution, but the
> complexity of availability eventually made me think of putting them it into
> own classes.
> 
> I think that all types of availability answers should always be represented
> the same, uniform way. It could be a HASHref like in CanBookBeIssued, but I
> would prefer an object that contains the availability information
> (Koha::Availability in my proposal) but doesn't define and care about the
> type of availability; it exists to ensure that we always have the same
> operations on availability information (to contain yes/maybe/no value to
> availability, to contain and return additional notes, reasons to ask for
> confirmation and unavailabilities).

Since you spent more time on this already, it might well be that I would also arrive at some unified structure for these responses :)
Koha::Availability just seems (at 'first' glance) to be a too prominent place for such a structure (imho).
Comment 6 Lari Taskula 2016-12-27 16:18:50 UTC
Created attachment 58446 [details] [review]
Bug 17712: Add new Koha::Exceptions

To describe problems or notes to availability, we will be using Koha::Exceptions.
This patch adds many useful Koha::Exceptions that we can use for this purpose.
Comment 7 Lari Taskula 2016-12-27 16:19:13 UTC
Created attachment 58447 [details] [review]
Bug 17712: Koha::Availability, a base for availability information

This patch adds a new class, Koha::Availability, which is designed to contain
availability information for each type of availability in an uniform way. Because
of this, Koha::Availability has no actual availability calculation logic, but
simply methods to get and set availability information into the object.

With such design, we can next create new classes Koha::Availability::Hold and
Koha::Availability::Checkout and let them inherit Koha::Availability so that they
are able to handle availaibility information in an uniform way.

       Koha::Availability::Hold          Koha::Availability::Checkout
                  |                                   |
                   \_________________________________/
                                   |
                          Koha::Availability

Koha::Availability can represent four levels of availability statuses:
1. available
2. available, with an additional note
3. available, but requires confirmation
4. unavailable

Additional notes, reasons for a need to confirm and reasons for unavailabilities
are kept in a HASHref, where each value in my proposal is a Koha::Exceptions::*.
This allows us to easily store any additional data directly into the reason. For
example, if we want to check biblio availability for hold and find out it is not
available, the HASHref for unavailabilities has a Koha::Exceptions::Patron::Debt
that contains parameters "current_outstanding" and "max_outstanding" which lets
us pick up the information easily later on without making new queries.

To test:
1. Run t/Koha/Availability.t
Comment 8 Lari Taskula 2016-12-27 16:19:35 UTC
Created attachment 58449 [details] [review]
Bug 17712: Add base for item and biblio availability

This patch adds two new classes:
- Koha::Biblio::Availability
- Koha::Item::Availability

These classes represent biblio- and item-level availabilities and the purpose
of this patch is simply to provide constructors that validate given biblio/item
and patron parameters.

Hold / checkout / search view availabilities (the actual answers to availability)
will be provided in the next patches.

To test:
1. Run t/db_dependent/Koha/Item/Availability.t
2. Run t/db_dependent/Koha/Biblio/Availability.t
Comment 9 Lari Taskula 2016-12-27 16:19:55 UTC
Created attachment 58450 [details] [review]
Bug 17712: Centralize availability-related checks

There are some problems to our current approach for "availability" which makes
it complicated to integrate with e.g. REST API.

Firstly, there has been no unified way of representing reasons for
unavailability. Previously, each implementation of availability calculation has
chosen its own way for describing the reasons. For example, CanItemBeReserved
string "ageRestricted" vs. CanBookBeIssued key "AGE_RESTRICTION" in a HASHref.

Secondly, some of the availability logic is contained outside centralized
methods like CanItemBeReserved which is missing checks e.g. for maxreserves
(checked instead in opac-reserve.pl) and patron fines. Perhaps this issue could
be fixed in another Bug, but I propose we squash it at the same time with this
Bug. Currently, because of this, we will not get reliable responses to
holdability from CanItemBeReserved alone.

This causes problems for API integration where we need to be able to describe
the reason in an uniform way and additionally provide information on what needs
to be done to fix the issue. Also, we do not want to duplicate all the external
checks from .pl files into our REST controllers.

Instead of modifying the old methods, I propose a new structure for the whole
availability-problem and let us deal with proper "status-codes" to easily
integrate availability & additional availability-related information into e.g.
REST API.

My proposal is an approach to centralize the availability-related checks and have
them return uniform reasons to describe reasons for availability problems.

Ultimately, we could ask availability something like this:
my $holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => 'CPL',
})->in_opac;

...and $holdability->unavailabilities HASHref would contain Koha::Exceptions
possibily with additional parameters to let us know why this biblio is not
holdable in OPAC.

This patch adds all availability related logic with centralization in mind. From
these individual methods we are able to construct full availability queries in
order to determine if something is actually available in some way and also
describe the problems with the help of Koha::Exceptions.

Since availability is a mixture of multiple different smaller checks from multiple
categories like item, patron, issuing rules, etc, they can be categorized into
their own subclasses. This lets us centralize availability-related methods per
category into their own modules.

To test:
1. prove t/db_dependent/Koha/Availability/Checks/*
Comment 10 Lari Taskula 2016-12-27 16:20:34 UTC
Created attachment 58451 [details] [review]
Bug 17712: Item availability for hold

Adds Koha::Item::Availability::Hold class for item hold availability.

This patch adds item holdability queries in two contexts:
1. in_opac     (perspective of patron themselves)
2. in_intranet (perspective of a librarian to patron)

Returns a Koha::Item::Availability::Hold object, which contains all information on
availability for hold in that context. This may include additional notes, reasons
to ask for confirmation or reasons for unavailability.

Example: To find out if patron can hold an item in OPAC, we will write:

my $availability = Koha::Availability::Hold->item({
    patron => $patron,
    item => $item,
    to_branch => $branchcode, # transfer allowed from holdingbranch to to_branch?
})->in_opac;
if ($availability->available) {
    # yes!
} else {
    foreach my $reason (keys %{$availability->unavailabilities}) {
        # each reason for unavailability
    }
    foreach my $reason (keys %{$availability->confirmations}) {
        # each reason that requires confirmation
    }
    foreach my $reason (keys %{$availability->notes}) {
        # each additional note
    }
}
Comment 11 Lari Taskula 2016-12-27 16:20:53 UTC
Created attachment 58452 [details] [review]
Bug 17712: Biblio availability for hold

Usage:

$holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => "CPL",
})->in_opac;
if ($holdability->available) {
  # yup!
  # arrayref of Koha::Item::Availability::Hold -objects:
  # 1. all available items (items may contain confirmations / notes)
  my $item_availabilities = $holdability->item_availabilities;
  # 2. all unavailable items
  my $item_unavailabilities = $holdability->item_unavailabilites;
}

Returns a Koha::Biblio::Availability::Hold -object.
Comment 12 Lari Taskula 2016-12-27 16:21:12 UTC
Created attachment 58453 [details] [review]
Bug 17712: Item availability for checkout

Usage:
my $issuability = Koha::Availability::Checkout->item({
  item => $item,
  patron => $patron,
  duedate => $duedate      # custom due date
})->in_intranet;
if ($issuability->available) {
  # yes!
} else {
  # not available for checkout. $issuability contains all the reasons for this.
}
Comment 13 Lari Taskula 2016-12-27 16:21:32 UTC
Created attachment 58454 [details] [review]
Bug 17712: Item and biblio availability in search context

Usage:
my $availability = Koha::Availability::Search->item({
  item => $item,
})->in_opac;
if ($availability->available) {
  # yes
}
Comment 14 Lari Taskula 2016-12-27 16:32:49 UTC
I set this Bug to "In discussion" and provided my proposal as a (crude) set of patches so that hopefully it helps to understand my idea better. I am open to renaming the classes if Koha::Availability doesn't feel right and also open for alternative solutions. We need to be able to integrate availability into REST API asap. and essentially these patches make that integration less painless.
Comment 15 Lari Taskula 2016-12-27 16:35:29 UTC
(In reply to Lari Taskula from comment #14)
> integration less painless.
/painless/painful

Happy new year!
Comment 16 Lari Taskula 2017-02-13 12:17:51 UTC
Created attachment 60147 [details] [review]
Bug 17712: Add new Koha::Exceptions

To describe problems or notes to availability, we will be using Koha::Exceptions.
This patch adds many useful Koha::Exceptions that we can use for this purpose.
Comment 17 Lari Taskula 2017-02-13 12:18:04 UTC
Created attachment 60148 [details] [review]
Bug 17712: Koha::Availability, a base for availability information

This patch adds a new class, Koha::Availability, which is designed to contain
availability information for each type of availability in an uniform way. Because
of this, Koha::Availability has no actual availability calculation logic, but
simply methods to get and set availability information into the object.

With such design, we can next create new classes Koha::Availability::Hold and
Koha::Availability::Checkout and let them inherit Koha::Availability so that they
are able to handle availaibility information in an uniform way.

       Koha::Availability::Hold          Koha::Availability::Checkout
                  |                                   |
                   \_________________________________/
                                   |
                          Koha::Availability

Koha::Availability can represent four levels of availability statuses:
1. available
2. available, with an additional note
3. available, but requires confirmation
4. unavailable

Additional notes, reasons for a need to confirm and reasons for unavailabilities
are kept in a HASHref, where each value in my proposal is a Koha::Exceptions::*.
This allows us to easily store any additional data directly into the reason. For
example, if we want to check biblio availability for hold and find out it is not
available, the HASHref for unavailabilities has a Koha::Exceptions::Patron::Debt
that contains parameters "current_outstanding" and "max_outstanding" which lets
us pick up the information easily later on without making new queries.

To test:
1. Run t/Koha/Availability.t
Comment 18 Lari Taskula 2017-02-13 12:18:17 UTC
Created attachment 60149 [details] [review]
Bug 17712: Add base for item and biblio availability

This patch adds two new classes:
- Koha::Biblio::Availability
- Koha::Item::Availability

These classes represent biblio- and item-level availabilities and the purpose
of this patch is simply to provide constructors that validate given biblio/item
and patron parameters.

Hold / checkout / search view availabilities (the actual answers to availability)
will be provided in the next patches.

To test:
1. Run t/db_dependent/Koha/Item/Availability.t
2. Run t/db_dependent/Koha/Biblio/Availability.t
Comment 19 Lari Taskula 2017-02-13 12:18:30 UTC
Created attachment 60150 [details] [review]
Bug 17712: Centralize availability-related checks

There are some problems to our current approach for "availability" which makes
it complicated to integrate with e.g. REST API.

Firstly, there has been no unified way of representing reasons for
unavailability. Previously, each implementation of availability calculation has
chosen its own way for describing the reasons. For example, CanItemBeReserved
string "ageRestricted" vs. CanBookBeIssued key "AGE_RESTRICTION" in a HASHref.

Secondly, some of the availability logic is contained outside centralized
methods like CanItemBeReserved which is missing checks e.g. for maxreserves
(checked instead in opac-reserve.pl) and patron fines. Perhaps this issue could
be fixed in another Bug, but I propose we squash it at the same time with this
Bug. Currently, because of this, we will not get reliable responses to
holdability from CanItemBeReserved alone.

This causes problems for API integration where we need to be able to describe
the reason in an uniform way and additionally provide information on what needs
to be done to fix the issue. Also, we do not want to duplicate all the external
checks from .pl files into our REST controllers.

Instead of modifying the old methods, I propose a new structure for the whole
availability-problem and let us deal with proper "status-codes" to easily
integrate availability & additional availability-related information into e.g.
REST API.

My proposal is an approach to centralize the availability-related checks and have
them return uniform reasons to describe reasons for availability problems.

Ultimately, we could ask availability something like this:
my $holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => 'CPL',
})->in_opac;

...and $holdability->unavailabilities HASHref would contain Koha::Exceptions
possibily with additional parameters to let us know why this biblio is not
holdable in OPAC.

This patch adds all availability related logic with centralization in mind. From
these individual methods we are able to construct full availability queries in
order to determine if something is actually available in some way and also
describe the problems with the help of Koha::Exceptions.

Since availability is a mixture of multiple different smaller checks from multiple
categories like item, patron, issuing rules, etc, they can be categorized into
their own subclasses. This lets us centralize availability-related methods per
category into their own modules.

To test:
1. prove t/db_dependent/Koha/Availability/Checks/*
Comment 20 Lari Taskula 2017-02-13 12:18:43 UTC
Created attachment 60151 [details] [review]
Bug 17712: Item availability for hold

Adds Koha::Item::Availability::Hold class for item hold availability.

This patch adds item holdability queries in two contexts:
1. in_opac     (perspective of patron themselves)
2. in_intranet (perspective of a librarian to patron)

Returns a Koha::Item::Availability::Hold object, which contains all information on
availability for hold in that context. This may include additional notes, reasons
to ask for confirmation or reasons for unavailability.

Example: To find out if patron can hold an item in OPAC, we will write:

my $availability = Koha::Availability::Hold->item({
    patron => $patron,
    item => $item,
    to_branch => $branchcode, # transfer allowed from holdingbranch to to_branch?
})->in_opac;
if ($availability->available) {
    # yes!
} else {
    foreach my $reason (keys %{$availability->unavailabilities}) {
        # each reason for unavailability
    }
    foreach my $reason (keys %{$availability->confirmations}) {
        # each reason that requires confirmation
    }
    foreach my $reason (keys %{$availability->notes}) {
        # each additional note
    }
}
Comment 21 Lari Taskula 2017-02-13 12:18:55 UTC
Created attachment 60152 [details] [review]
Bug 17712: Biblio availability for hold

Usage:

$holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => "CPL",
})->in_opac;
if ($holdability->available) {
  # yup!
  # arrayref of Koha::Item::Availability::Hold -objects:
  # 1. all available items (items may contain confirmations / notes)
  my $item_availabilities = $holdability->item_availabilities;
  # 2. all unavailable items
  my $item_unavailabilities = $holdability->item_unavailabilites;
}

Returns a Koha::Biblio::Availability::Hold -object.
Comment 22 Lari Taskula 2017-02-13 12:19:09 UTC
Created attachment 60153 [details] [review]
Bug 17712: Item availability for checkout

Usage:
my $issuability = Koha::Availability::Checkout->item({
  item => $item,
  patron => $patron,
  duedate => $duedate      # custom due date
})->in_intranet;
if ($issuability->available) {
  # yes!
} else {
  # not available for checkout. $issuability contains all the reasons for this.
}
Comment 23 Lari Taskula 2017-02-13 12:19:21 UTC
Created attachment 60154 [details] [review]
Bug 17712: Item and biblio availability in search context

Usage:
my $availability = Koha::Availability::Search->item({
  item => $item,
})->in_opac;
if ($availability->available) {
  # yes
}
Comment 24 Lari Taskula 2017-02-13 12:25:16 UTC
Fixed failing tests. To test these patches, I recommend applying patches in Bug 16826 on top of these and testing it via REST API. It demonstrates the unavailability reasons nicely as it will include any additional parameter (like "max_outstanding", "current_outstanding" in Patron::Debt) in the response JSON.
Comment 25 Lari Taskula 2017-02-15 16:18:45 UTC
Created attachment 60261 [details] [review]
Bug 17712: Centralize availability-related checks

There are some problems to our current approach for "availability" which makes
it complicated to integrate with e.g. REST API.

Firstly, there has been no unified way of representing reasons for
unavailability. Previously, each implementation of availability calculation has
chosen its own way for describing the reasons. For example, CanItemBeReserved
string "ageRestricted" vs. CanBookBeIssued key "AGE_RESTRICTION" in a HASHref.

Secondly, some of the availability logic is contained outside centralized
methods like CanItemBeReserved which is missing checks e.g. for maxreserves
(checked instead in opac-reserve.pl) and patron fines. Perhaps this issue could
be fixed in another Bug, but I propose we squash it at the same time with this
Bug. Currently, because of this, we will not get reliable responses to
holdability from CanItemBeReserved alone.

This causes problems for API integration where we need to be able to describe
the reason in an uniform way and additionally provide information on what needs
to be done to fix the issue. Also, we do not want to duplicate all the external
checks from .pl files into our REST controllers.

Instead of modifying the old methods, I propose a new structure for the whole
availability-problem and let us deal with proper "status-codes" to easily
integrate availability & additional availability-related information into e.g.
REST API.

My proposal is an approach to centralize the availability-related checks and have
them return uniform reasons to describe reasons for availability problems.

Ultimately, we could ask availability something like this:
my $holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => 'CPL',
})->in_opac;

...and $holdability->unavailabilities HASHref would contain Koha::Exceptions
possibily with additional parameters to let us know why this biblio is not
holdable in OPAC.

This patch adds all availability related logic with centralization in mind. From
these individual methods we are able to construct full availability queries in
order to determine if something is actually available in some way and also
describe the problems with the help of Koha::Exceptions.

Since availability is a mixture of multiple different smaller checks from multiple
categories like item, patron, issuing rules, etc, they can be categorized into
their own subclasses. This lets us centralize availability-related methods per
category into their own modules.

To test:
1. prove t/db_dependent/Koha/Availability/Checks/*
Comment 26 Lari Taskula 2017-02-15 16:19:00 UTC
Created attachment 60262 [details] [review]
Bug 17712: Item availability for hold

Adds Koha::Item::Availability::Hold class for item hold availability.

This patch adds item holdability queries in two contexts:
1. in_opac     (perspective of patron themselves)
2. in_intranet (perspective of a librarian to patron)

Returns a Koha::Item::Availability::Hold object, which contains all information on
availability for hold in that context. This may include additional notes, reasons
to ask for confirmation or reasons for unavailability.

Example: To find out if patron can hold an item in OPAC, we will write:

my $availability = Koha::Availability::Hold->item({
    patron => $patron,
    item => $item,
    to_branch => $branchcode, # transfer allowed from holdingbranch to to_branch?
})->in_opac;
if ($availability->available) {
    # yes!
} else {
    foreach my $reason (keys %{$availability->unavailabilities}) {
        # each reason for unavailability
    }
    foreach my $reason (keys %{$availability->confirmations}) {
        # each reason that requires confirmation
    }
    foreach my $reason (keys %{$availability->notes}) {
        # each additional note
    }
}
Comment 27 Lari Taskula 2017-02-15 16:19:15 UTC
Created attachment 60263 [details] [review]
Bug 17712: Biblio availability for hold

Usage:

$holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => "CPL",
})->in_opac;
if ($holdability->available) {
  # yup!
  # arrayref of Koha::Item::Availability::Hold -objects:
  # 1. all available items (items may contain confirmations / notes)
  my $item_availabilities = $holdability->item_availabilities;
  # 2. all unavailable items
  my $item_unavailabilities = $holdability->item_unavailabilites;
}

Returns a Koha::Biblio::Availability::Hold -object.
Comment 28 Lari Taskula 2017-02-15 16:19:30 UTC
Created attachment 60264 [details] [review]
Bug 17712: Item availability for checkout

Usage:
my $issuability = Koha::Availability::Checkout->item({
  item => $item,
  patron => $patron,
  duedate => $duedate      # custom due date
})->in_intranet;
if ($issuability->available) {
  # yes!
} else {
  # not available for checkout. $issuability contains all the reasons for this.
}
Comment 29 Lari Taskula 2017-02-15 16:19:44 UTC
Created attachment 60265 [details] [review]
Bug 17712: Item and biblio availability in search context

Usage:
my $availability = Koha::Availability::Search->item({
  item => $item,
})->in_opac;
if ($availability->available) {
  # yes
}
Comment 30 Julian Maurice 2017-04-11 07:19:30 UTC
Why is this bug 'In Discussion' ? Shouldn't it be 'Needs Signoff' ?
Comment 31 Lari Taskula 2017-04-11 07:52:24 UTC
(In reply to Julian Maurice from comment #30)
> Why is this bug 'In Discussion' ? Shouldn't it be 'Needs Signoff' ?

Hi Julian! I left this in discussion to get more feedback on the idea and the patches were attached as an example implementation. But you are right, Needs Signoff is a better status now that there are attached patches implementing this suggestion.

We are actually in process of testing this feature together with VuFind via Koha REST API (Bug 16826). So far we received some valuable test feedback, and I have follow-up bugfixes coming that I will squash soon. I shall set this to needs signoff then :) Thanks for the comment!
Comment 32 Julian Maurice 2017-04-11 07:58:41 UTC
Nice! I haven't had a chance to look at the code yet but the idea is great IMO :)
Comment 33 Lari Taskula 2017-04-26 12:28:19 UTC
Created attachment 62719 [details] [review]
Bug 17712: Add new Koha::Exceptions

To describe problems or notes to availability, we will be using Koha::Exceptions.
This patch adds many useful Koha::Exceptions that we can use for this purpose.
Comment 34 Lari Taskula 2017-04-26 12:28:44 UTC
Created attachment 62720 [details] [review]
Bug 17712: Koha::Availability, a base for availability information

This patch adds a new class, Koha::Availability, which is designed to contain
availability information for each type of availability in an uniform way. Because
of this, Koha::Availability has no actual availability calculation logic, but
simply methods to get and set availability information into the object.

With such design, we can next create new classes Koha::Availability::Hold and
Koha::Availability::Checkout and let them inherit Koha::Availability so that they
are able to handle availaibility information in an uniform way.

       Koha::Availability::Hold          Koha::Availability::Checkout
                  |                                   |
                   \_________________________________/
                                   |
                          Koha::Availability

Koha::Availability can represent four levels of availability statuses:
1. available
2. available, with an additional note
3. available, but requires confirmation
4. unavailable

Additional notes, reasons for a need to confirm and reasons for unavailabilities
are kept in a HASHref, where each value in my proposal is a Koha::Exceptions::*.
This allows us to easily store any additional data directly into the reason. For
example, if we want to check biblio availability for hold and find out it is not
available, the HASHref for unavailabilities has a Koha::Exceptions::Patron::Debt
that contains parameters "current_outstanding" and "max_outstanding" which lets
us pick up the information easily later on without making new queries.

To test:
1. Run t/Koha/Availability.t
Comment 35 Lari Taskula 2017-04-26 12:29:15 UTC
Created attachment 62721 [details] [review]
Bug 17712: Add base for item and biblio availability

This patch adds two new classes:
- Koha::Biblio::Availability
- Koha::Item::Availability

These classes represent biblio- and item-level availabilities and the purpose
of this patch is simply to provide constructors that validate given biblio/item
and patron parameters.

Hold / checkout / search view availabilities (the actual answers to availability)
will be provided in the next patches.

To test:
1. Run t/db_dependent/Koha/Item/Availability.t
2. Run t/db_dependent/Koha/Biblio/Availability.t
Comment 36 Lari Taskula 2017-04-26 12:29:27 UTC
Created attachment 62722 [details] [review]
Bug 17712: Centralize availability-related checks

There are some problems to our current approach for "availability" which makes
it complicated to integrate with e.g. REST API.

Firstly, there has been no unified way of representing reasons for
unavailability. Previously, each implementation of availability calculation has
chosen its own way for describing the reasons. For example, CanItemBeReserved
string "ageRestricted" vs. CanBookBeIssued key "AGE_RESTRICTION" in a HASHref.

Secondly, some of the availability logic is contained outside centralized
methods like CanItemBeReserved which is missing checks e.g. for maxreserves
(checked instead in opac-reserve.pl) and patron fines. Perhaps this issue could
be fixed in another Bug, but I propose we squash it at the same time with this
Bug. Currently, because of this, we will not get reliable responses to
holdability from CanItemBeReserved alone.

This causes problems for API integration where we need to be able to describe
the reason in an uniform way and additionally provide information on what needs
to be done to fix the issue. Also, we do not want to duplicate all the external
checks from .pl files into our REST controllers.

Instead of modifying the old methods, I propose a new structure for the whole
availability-problem and let us deal with proper "status-codes" to easily
integrate availability & additional availability-related information into e.g.
REST API.

My proposal is an approach to centralize the availability-related checks and have
them return uniform reasons to describe reasons for availability problems.

Ultimately, we could ask availability something like this:
my $holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => 'CPL',
})->in_opac;

...and $holdability->unavailabilities HASHref would contain Koha::Exceptions
possibily with additional parameters to let us know why this biblio is not
holdable in OPAC.

This patch adds all availability related logic with centralization in mind. From
these individual methods we are able to construct full availability queries in
order to determine if something is actually available in some way and also
describe the problems with the help of Koha::Exceptions.

Since availability is a mixture of multiple different smaller checks from multiple
categories like item, patron, issuing rules, etc, they can be categorized into
their own subclasses. This lets us centralize availability-related methods per
category into their own modules.

To test:
1. prove t/db_dependent/Koha/Availability/Checks/*
Comment 37 Lari Taskula 2017-04-26 12:29:39 UTC
Created attachment 62723 [details] [review]
Bug 17712: Item availability for hold

Adds Koha::Item::Availability::Hold class for item hold availability.

This patch adds item holdability queries in two contexts:
1. in_opac     (perspective of patron themselves)
2. in_intranet (perspective of a librarian to patron)

Returns a Koha::Item::Availability::Hold object, which contains all information on
availability for hold in that context. This may include additional notes, reasons
to ask for confirmation or reasons for unavailability.

Example: To find out if patron can hold an item in OPAC, we will write:

my $availability = Koha::Availability::Hold->item({
    patron => $patron,
    item => $item,
    to_branch => $branchcode, # transfer allowed from holdingbranch to to_branch?
})->in_opac;
if ($availability->available) {
    # yes!
} else {
    foreach my $reason (keys %{$availability->unavailabilities}) {
        # each reason for unavailability
    }
    foreach my $reason (keys %{$availability->confirmations}) {
        # each reason that requires confirmation
    }
    foreach my $reason (keys %{$availability->notes}) {
        # each additional note
    }
}
Comment 38 Lari Taskula 2017-04-26 12:29:50 UTC
Created attachment 62724 [details] [review]
Bug 17712: Biblio availability for hold

Usage:

$holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => "CPL",
})->in_opac;
if ($holdability->available) {
  # yup!
  # arrayref of Koha::Item::Availability::Hold -objects:
  # 1. all available items (items may contain confirmations / notes)
  my $item_availabilities = $holdability->item_availabilities;
  # 2. all unavailable items
  my $item_unavailabilities = $holdability->item_unavailabilites;
}

Returns a Koha::Biblio::Availability::Hold -object.
Comment 39 Lari Taskula 2017-04-26 12:30:03 UTC
Created attachment 62725 [details] [review]
Bug 17712: Item availability for checkout

Usage:
my $issuability = Koha::Availability::Checkout->item({
  item => $item,
  patron => $patron,
  duedate => $duedate      # custom due date
})->in_intranet;
if ($issuability->available) {
  # yes!
} else {
  # not available for checkout. $issuability contains all the reasons for this.
}
Comment 40 Lari Taskula 2017-04-26 12:30:16 UTC
Created attachment 62726 [details] [review]
Bug 17712: Item and biblio availability in search context

Usage:
my $availability = Koha::Availability::Search->item({
  item => $item,
})->in_opac;
if ($availability->available) {
  # yes
}
Comment 41 Lari Taskula 2017-04-26 12:34:47 UTC
Rebased on master. Squashed some small bug fixes into the commits. Like before, I recommend testing it with the REST API (Bug 16826).
Comment 42 Benjamin Rokseth 2017-05-10 13:34:00 UTC
Sorry to see you're so alone on this, as it sounds like a very good idea! I will sign it off, though I have some concerns, particularly on performance.

Passing koha objects around this way seems very javaish, and I don't really see the point, if all you really need is foreign keys. 

But then again, its circumstantial, and could be remedied at a later stage if needed. And it's a bit unfair on such a large codechange, which is actually relatively easy to read and follow.
Comment 43 Benjamin Rokseth 2017-05-10 13:39:43 UTC
Created attachment 63343 [details] [review]
Bug 17712: Add new Koha::Exceptions

To describe problems or notes to availability, we will be using Koha::Exceptions.
This patch adds many useful Koha::Exceptions that we can use for this purpose.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 44 Benjamin Rokseth 2017-05-10 13:39:51 UTC
Created attachment 63344 [details] [review]
Bug 17712: Koha::Availability, a base for availability information

This patch adds a new class, Koha::Availability, which is designed to contain
availability information for each type of availability in an uniform way. Because
of this, Koha::Availability has no actual availability calculation logic, but
simply methods to get and set availability information into the object.

With such design, we can next create new classes Koha::Availability::Hold and
Koha::Availability::Checkout and let them inherit Koha::Availability so that they
are able to handle availaibility information in an uniform way.

       Koha::Availability::Hold          Koha::Availability::Checkout
                  |                                   |
                   \_________________________________/
                                   |
                          Koha::Availability

Koha::Availability can represent four levels of availability statuses:
1. available
2. available, with an additional note
3. available, but requires confirmation
4. unavailable

Additional notes, reasons for a need to confirm and reasons for unavailabilities
are kept in a HASHref, where each value in my proposal is a Koha::Exceptions::*.
This allows us to easily store any additional data directly into the reason. For
example, if we want to check biblio availability for hold and find out it is not
available, the HASHref for unavailabilities has a Koha::Exceptions::Patron::Debt
that contains parameters "current_outstanding" and "max_outstanding" which lets
us pick up the information easily later on without making new queries.

To test:
1. Run t/Koha/Availability.t

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 45 Benjamin Rokseth 2017-05-10 13:39:57 UTC
Created attachment 63345 [details] [review]
Bug 17712: Add base for item and biblio availability

This patch adds two new classes:
- Koha::Biblio::Availability
- Koha::Item::Availability

These classes represent biblio- and item-level availabilities and the purpose
of this patch is simply to provide constructors that validate given biblio/item
and patron parameters.

Hold / checkout / search view availabilities (the actual answers to availability)
will be provided in the next patches.

To test:
1. Run t/db_dependent/Koha/Item/Availability.t
2. Run t/db_dependent/Koha/Biblio/Availability.t

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 46 Benjamin Rokseth 2017-05-10 13:40:04 UTC
Created attachment 63346 [details] [review]
Bug 17712: Centralize availability-related checks

There are some problems to our current approach for "availability" which makes
it complicated to integrate with e.g. REST API.

Firstly, there has been no unified way of representing reasons for
unavailability. Previously, each implementation of availability calculation has
chosen its own way for describing the reasons. For example, CanItemBeReserved
string "ageRestricted" vs. CanBookBeIssued key "AGE_RESTRICTION" in a HASHref.

Secondly, some of the availability logic is contained outside centralized
methods like CanItemBeReserved which is missing checks e.g. for maxreserves
(checked instead in opac-reserve.pl) and patron fines. Perhaps this issue could
be fixed in another Bug, but I propose we squash it at the same time with this
Bug. Currently, because of this, we will not get reliable responses to
holdability from CanItemBeReserved alone.

This causes problems for API integration where we need to be able to describe
the reason in an uniform way and additionally provide information on what needs
to be done to fix the issue. Also, we do not want to duplicate all the external
checks from .pl files into our REST controllers.

Instead of modifying the old methods, I propose a new structure for the whole
availability-problem and let us deal with proper "status-codes" to easily
integrate availability & additional availability-related information into e.g.
REST API.

My proposal is an approach to centralize the availability-related checks and have
them return uniform reasons to describe reasons for availability problems.

Ultimately, we could ask availability something like this:
my $holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => 'CPL',
})->in_opac;

...and $holdability->unavailabilities HASHref would contain Koha::Exceptions
possibily with additional parameters to let us know why this biblio is not
holdable in OPAC.

This patch adds all availability related logic with centralization in mind. From
these individual methods we are able to construct full availability queries in
order to determine if something is actually available in some way and also
describe the problems with the help of Koha::Exceptions.

Since availability is a mixture of multiple different smaller checks from multiple
categories like item, patron, issuing rules, etc, they can be categorized into
their own subclasses. This lets us centralize availability-related methods per
category into their own modules.

To test:
1. prove t/db_dependent/Koha/Availability/Checks/*

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 47 Benjamin Rokseth 2017-05-10 13:40:12 UTC
Created attachment 63347 [details] [review]
Bug 17712: Item availability for hold

Adds Koha::Item::Availability::Hold class for item hold availability.

This patch adds item holdability queries in two contexts:
1. in_opac     (perspective of patron themselves)
2. in_intranet (perspective of a librarian to patron)

Returns a Koha::Item::Availability::Hold object, which contains all information on
availability for hold in that context. This may include additional notes, reasons
to ask for confirmation or reasons for unavailability.

Example: To find out if patron can hold an item in OPAC, we will write:

my $availability = Koha::Availability::Hold->item({
    patron => $patron,
    item => $item,
    to_branch => $branchcode, # transfer allowed from holdingbranch to to_branch?
})->in_opac;
if ($availability->available) {
    # yes!
} else {
    foreach my $reason (keys %{$availability->unavailabilities}) {
        # each reason for unavailability
    }
    foreach my $reason (keys %{$availability->confirmations}) {
        # each reason that requires confirmation
    }
    foreach my $reason (keys %{$availability->notes}) {
        # each additional note
    }
}

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 48 Benjamin Rokseth 2017-05-10 13:40:22 UTC
Created attachment 63348 [details] [review]
Bug 17712: Biblio availability for hold

Usage:

$holdability = Koha::Availability::Hold->biblio({
  biblio => $biblio,
  patron => $patron,
  to_branch => "CPL",
})->in_opac;
if ($holdability->available) {
  # yup!
  # arrayref of Koha::Item::Availability::Hold -objects:
  # 1. all available items (items may contain confirmations / notes)
  my $item_availabilities = $holdability->item_availabilities;
  # 2. all unavailable items
  my $item_unavailabilities = $holdability->item_unavailabilites;
}

Returns a Koha::Biblio::Availability::Hold -object.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 49 Benjamin Rokseth 2017-05-10 13:40:29 UTC
Created attachment 63349 [details] [review]
Bug 17712: Item availability for checkout

Usage:
my $issuability = Koha::Availability::Checkout->item({
  item => $item,
  patron => $patron,
  duedate => $duedate      # custom due date
})->in_intranet;
if ($issuability->available) {
  # yes!
} else {
  # not available for checkout. $issuability contains all the reasons for this.
}

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 50 Benjamin Rokseth 2017-05-10 13:40:37 UTC
Created attachment 63350 [details] [review]
Bug 17712: Item and biblio availability in search context

Usage:
my $availability = Koha::Availability::Search->item({
  item => $item,
})->in_opac;
if ($availability->available) {
  # yes
}

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 51 Benjamin Rokseth 2017-05-10 13:43:51 UTC
tests pass. works as described. Ran some manual tests by perl evaluated code
Comment 52 Lari Taskula 2017-05-10 14:31:55 UTC
(In reply to Benjamin Rokseth from comment #42)
> Sorry to see you're so alone on this, as it sounds like a very good idea! I
> will sign it off, though I have some concerns, particularly on performance.
> 
> Passing koha objects around this way seems very javaish, and I don't really
> see the point, if all you really need is foreign keys. 
> 
> But then again, its circumstantial, and could be remedied at a later stage
> if needed. And it's a bit unfair on such a large codechange, which is
> actually relatively easy to read and follow.

Thanks for the support, new ideas, and the sign-off, Benjamin, and glad to hear from you! I found this a very challenging project and surely there is room for further performance improvements as well as other changes - under heavy pressure some of the choices made may have not been optimal. Follow-ups are welcome :)

BTW, this is also under busy testing by our librarians and in the following days I'll have a desk-full of test reports and possibly follow-ups coming in for this Bug :)
Comment 53 Marcel de Rooy 2017-08-25 09:50:39 UTC
Koha::Exceptions::WrongParameter is already present, you add BadParameter
Comment 54 Marcel de Rooy 2017-08-25 09:55:22 UTC
(In reply to Lari Taskula from comment #52)
> BTW, this is also under busy testing by our librarians and in the following
> days I'll have a desk-full of test reports and possibly follow-ups coming in
> for this Bug :)

No reports or follow-ups were added ?

Needs feedback.
Comment 55 Marcel de Rooy 2017-08-25 09:55:38 UTC
#   Failed test 'No tests run for subtest "Attempt to instantiate holdability class with valid biblionumber"'
#   at t/db_dependent/Koha/Biblio/Availability.t line 52.
Cannot use "->find" in list context at /usr/share/koha/masterclone/Koha/Biblio/Availability.pm line 91.
Comment 56 Marcel de Rooy 2017-08-25 09:56:07 UTC
#   Failed test 'No tests run for subtest "Attempt to instantiate holdability class with valid itemnumber"'
#   at t/db_dependent/Koha/Item/Availability.t line 52.
Cannot use "->find" in list context at /usr/share/koha/masterclone/Koha/Item/Availability.pm line 85.
Comment 57 Marcel de Rooy 2017-08-25 09:59:07 UTC
t/db_dependent/Koha/Availability/Checks/Item.t ............. 1/14
#   Failed test 'checked_out'
#   at t/db_dependent/Koha/Availability/Checks/Item.t line 48.
Undefined subroutine &C4::Circulation::GetItemIssue called at /usr/share/koha/masterclone/Koha/Availability/Checks/Item.pm line 68.
Comment 58 Marcel de Rooy 2017-08-25 10:01:28 UTC
t/db_dependent/Koha/Item/Availability/Checkout.t (Wstat: 65280 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 10 tests but ran 2.

t/db_dependent/Koha/Item/Availability/Search.t  (Wstat: 256 Tests: 10 Failed: 1)
  Failed test:  6
  Non-zero exit status: 1
Comment 59 Marcel de Rooy 2017-08-25 10:11:27 UTC
Lari,
Looks impressive. At this point it fails for various problems reported.
The big discussion we face here, is: Do we want to add all this new code without using it yet? It is obviously very hard to start using it everywhere, but I guess we need some strategy in order to prevent ending up with a lot of unused code that is no longer in sync with the current routines.

And at that point it will of course need some really extensive testing.