Bug 36271

Hi Martin,

When I was testing bookings, I was thinking of a separate parameter for the booking period. However, using the loan length makes sense. I'm not sure about the renewals. Usually, people can renew IF no one else needs the item (i.e. no holds).  But booking for the entire renewal period would remove the option of someone placing a hold on it and getting it after a single loan period, no?

I feel like whatever we choose (loan length or separate rule), someone will want the other option lol! Maybe we could do a separate parameter whose default option is the items loan length?

I'd be curious to know what people using it IRL think. I seem to remember Bywater had libraries who were anxiously awaiting this feature, maybe they would have a more informed opinion than mine.

Hello :) 

I'm actually working on that, for now, I defined a CanItemBeBookedInAdvance function in Bookings to deal with some rules, 4 in fact : 
- booking_total_allowed
- booking_per_item
- booking_per_itemtype ( related to bug 35906 )
- booking_period_length

Does this seem useful to you?

For us (one of the ByWater libraries), we would picture this as tied in to the circulation rules for item types. 

We have item types set up for the portions of our collection that can be booked. A kit that staff book for story hours, for example, has an item type of Staff Program Support and that item type has its own circ period (1 day plus built in prep time from the booking module) and number of renewals allowed (0).

Thanks everyone, this is great feedback.

Really interested to see your work once there's things to look at Thibuad. I'm happy with the direction the itemtype bookable code has taken, though of course we need a signoff there before it can reach the next steps.

I've been looking into the lead-in and trail-out times work on another bug and trying to work through where to place those rules too.. circ level, system preference level etc.

The combination of patron/branch/itemtype could be rather fun to deal with performance-wise with the current datepicker handling so that needs some considerations here whilst we're digging through.

It would be great to see bug 35906 (bookable at itemtype level) tested before we progress further here in my opinion.

Created attachment 163546 [details] [review]
Bug 36271 : Add circulation rules for booking

Welcome to 4 new circulation rules :)

- Bookings allowed (total) / bookings_allowed_total
- Bookings per item (total) / bookings_per_item
- Bookings per itemtype (total) / bookings per itemtype
- Bookings period length (day) / bookings_period_length

Test plan:
1) Apply this patch
2) Set values for these new rules
** Note that rules respect the item setting bookable:
   - bookable to "yes" => bookings per item rule
   - bookable to "follow item type" => bookings per itemtype rule
   - bookable to 'no' => no "Bookings" section on menu
3) Try perform a booking now
4) Define a total to 1, 2 per item and 3 per itemtype with 2 days for
   example for booking period.
5) After 1 an error message showing related to total allowed
6) Set total to 5 and retry on item bookable sets to "Yes"
7) Normally it's works but do it again (to have 3 bookings and an error
   message appears)
8) Do the same thing on item bookable sets to "Follow item type", the
   maximum will be 3 now before see the error.
** Note that if bookings total and booking per item/itemtype is set it
will be the minimum number.
9)You can also try to test with a longer period than bookings period
  length rule, an another error will be displayed.

Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS)

Hi !

It's a first proposal, I haven't yet written unit tests because (unless I'm mistaken) they're currently KO (Booking.t) 

If you'd like to give me some feedback on what's been done, I'd be happy to hear from you.

Perhaps we could change the patch title to broaden the scope of the rules if you agree

Nice one.. I'd just started to look again at this and you posted a patch.. I'll take a look now :)

Test plan passed OK if one rule is defined for one itemtype

But with several rules : 
All / Mixed materials : 3 bookings, 2 per item, 5 days
All / All : 1 booking, 1 per item, 2 days

If I have on my account :  
3 bookings for mixed materials on two items / two records
I cannot place a booking on an item of Reference type in the same record as above, or in another record : because of maximum booking rule =1
As I have no booked itemtype other than Mixed materials, I should not be blocked for another itemtype.
If I set All / All : 4 bookings, I can add the booking on itemtype Reference

unless I misunderstand, All is not a general quota, but used if no precise rule is defined

=> the counting of booked items should be done per itemtype.

> Bug 36271 : xxx
Please fix commit message, no space before semicolon.

Dependent bug 35906 no longer applies 8-(..

The patches no longer apply 8-(..

git bz apply 36271

Bug 36271 Depends on bug 35906 (Signed Off)
Follow? [(y)es, (n)o] y

Bug 35906 - Add bookable option on itemtypes

166866 - Bug 35906: Add bookable option on itemtype
166867 - Bug 35906: Add unit test
166868 - Bug 35906: Remove preference and add override handling
166869 - Bug 35906: Schema dump
166870 - Bug 35906: (QA follow-up) Fix item-type schema
166871 - Bug 35906: (follow-up) rebased it

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 35906: Add bookable option on itemtype
Applying: Bug 35906: Add unit test
Applying: Bug 35906: Remove preference and add override handling
Applying: Bug 35906: Schema dump
Applying: Bug 35906: (QA follow-up) Fix item-type schema
Applying: Bug 35906: (follow-up) rebased it

Bug 36271 - Bookings should respect circulation rules for max loan periods

166809 - Bug 36271: Add circulation rules for booking
166907 - Bug 36271: (follow-up) Add switch for join table according to item-level_itypes syspref

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 36271: Add circulation rules for booking
Using index info to reconstruct a base tree...
M	Koha/Booking.pm
M	Koha/REST/V1/Bookings.pm
M	admin/smart-rules.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
A	koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js deleted in HEAD and modified in Bug 36271: Add circulation rules for booking. Version Bug 36271: Add circulation rules for booking of koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js left in tree.
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging admin/smart-rules.pl
Auto-merging Koha/REST/V1/Bookings.pm
Auto-merging Koha/Booking.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 36271: Add circulation rules for booking

Created attachment 166933 [details] [review]
Bug 36271: (follow-up) Add switch for join table according to item-level_itypes syspref

The patches now apply, however there is an error when restarting everything - see [1]. I think I managed tow ork around this, but what I did may be incorrect.

I tested using koha-testing-docker (KTD).

I have changed the status to Failed QA:

1. Have set the rules as per the test plan.

2. I can make one booking, then when I go to make another for the same patron, I get a failure error (as expected, as Bookings per item (total) = 1).

3. I changed the rules so that they are 2, 2, and 2 - with the expectation that the same patron could make 2 bookings for the item, at different times. I get "Failure" in a red box at the top of the dialogue box if I try to make two bookings.

4. I have not changed any other settings, or made any changes to the item type:
   . Under the items section for the record where you change it to bookable it says "Item type bookable: No"
   . I don't know whether that causes an issue or not
   . If you go to change the bookable setting, then if the item type says it isn't bookable, then you shouldn't be able to override and make an individual item bookable(?) Not sure if this is the way it should work, or I have misunderstood.
   . Probably not really understanding what the interactions should be here:
     . Set as bookable at the item type level, override at the individual record/item level?
     . Set as bookable at the item type level only? Then having bookable showing at the individual record/item level doesn't make sense to me.
   . Maybe some more explanation is required in the commit message and test plan of the intent may help
   . (I haven't read through the bug, and related bugs, history)

5. I'm not a fan of generic "Failure" messages. If there are a set of rules (as in this case), then I think any error message should indicate the reason for the failure so you can correct things. In this case, I don't know what the reason for the failure is.

[1] Error when restarting everything
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

After the patch is applied, I get this error message when restarting everything (apply patch, updatedatabase, flush_memcached, restart_all):

restart_all
Restarting Apache httpd web server: apache2.
Restarting Koha ILS: koha-commonStopping Plack daemon for kohadev:.
Stopping Z39.50/SRU daemon for kohadev:.
Stopping Koha worker daemon for kohadev (default):.
Stopping Koha worker daemon for kohadev (long_tasks):.
Stopping Koha indexing daemon for kohadev:.
Successfully stopped all services for instance koha-common:.
Starting Plack daemon for kohadev:.
Starting Z39.50/SRU daemon for kohadev:syntax error at /kohadevbox/koha/Koha/Booking.pm line 118, near "if if"
Compilation failed in require at /kohadevbox/koha/Koha/Bookings.pm line 23.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Bookings.pm line 23.
Compilation failed in require at /kohadevbox/koha/Koha/Biblio.pm line 46.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Biblio.pm line 46.
Compilation failed in require at /kohadevbox/koha/Koha/Biblios.pm line 25.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Biblios.pm line 25.
Compilation failed in require at /kohadevbox/koha/Koha/Libraries.pm line 25.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Libraries.pm line 25.
Compilation failed in require at /kohadevbox/koha/Koha/Object/Limit/Library.pm line 22.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Object/Limit/Library.pm line 22.
Compilation failed in require at /kohadevbox/koha/Koha/AuthorisedValue.pm line 26.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/AuthorisedValue.pm line 26.
Compilation failed in require at /kohadevbox/koha/Koha/AuthorisedValues.pm line 25.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/AuthorisedValues.pm line 25.
Compilation failed in require at /kohadevbox/koha/C4/Koha.pm line 27.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Koha.pm line 27.
Compilation failed in require at /kohadevbox/koha/C4/Biblio.pm line 88.
BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Biblio.pm line 88.
Compilation failed in require at /kohadevbox/koha/Koha/Z3950Responder.pm line 22.
BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Z3950Responder.pm line 22.
Compilation failed in require at /kohadevbox/koha/misc/z3950_responder.pl line 27.
BEGIN failed--compilation aborted at /kohadevbox/koha/misc/z3950_responder.pl line 27.
 failed!
Starting Koha worker daemon for kohadev (default):.
Starting Koha worker daemon for kohadev (long_tasks):.
Starting Koha indexing daemon for kohadev:.

At line 118 of Koha/Booking.pm there is an "if if" - if I remove one of the ifs, then everything restarts properly.

(In reply to David Nind from comment #22)
> The patches now apply, however there is an error when restarting everything
> - see [1]. I think I managed tow ork around this, but what I did may be
> incorrect.
> 
> I tested using koha-testing-docker (KTD).
> 
> I have changed the status to Failed QA:
> 
> 1. Have set the rules as per the test plan.
> 
> 2. I can make one booking, then when I go to make another for the same
> patron, I get a failure error (as expected, as Bookings per item (total) =
> 1).
> 
> 3. I changed the rules so that they are 2, 2, and 2 - with the expectation
> that the same patron could make 2 bookings for the item, at different times.
> I get "Failure" in a red box at the top of the dialogue box if I try to make
> two bookings.
> 
> 4. I have not changed any other settings, or made any changes to the item
> type:
>    . Under the items section for the record where you change it to bookable
> it says "Item type bookable: No"
>    . I don't know whether that causes an issue or not
>    . If you go to change the bookable setting, then if the item type says it
> isn't bookable, then you shouldn't be able to override and make an
> individual item bookable(?) Not sure if this is the way it should work, or I
> have misunderstood.
>    . Probably not really understanding what the interactions should be here:
>      . Set as bookable at the item type level, override at the individual
> record/item level?
>      . Set as bookable at the item type level only? Then having bookable
> showing at the individual record/item level doesn't make sense to me.
>    . Maybe some more explanation is required in the commit message and test
> plan of the intent may help
>    . (I haven't read through the bug, and related bugs, history)
> 
> 5. I'm not a fan of generic "Failure" messages. If there are a set of rules
> (as in this case), then I think any error message should indicate the reason
> for the failure so you can correct things. In this case, I don't know what
> the reason for the failure is.
> 
> [1] Error when restarting everything
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> After the patch is applied, I get this error message when restarting
> everything (apply patch, updatedatabase, flush_memcached, restart_all):
> 
> restart_all
> Restarting Apache httpd web server: apache2.
> Restarting Koha ILS: koha-commonStopping Plack daemon for kohadev:.
> Stopping Z39.50/SRU daemon for kohadev:.
> Stopping Koha worker daemon for kohadev (default):.
> Stopping Koha worker daemon for kohadev (long_tasks):.
> Stopping Koha indexing daemon for kohadev:.
> Successfully stopped all services for instance koha-common:.
> Starting Plack daemon for kohadev:.
> Starting Z39.50/SRU daemon for kohadev:syntax error at
> /kohadevbox/koha/Koha/Booking.pm line 118, near "if if"
> Compilation failed in require at /kohadevbox/koha/Koha/Bookings.pm line 23.
> BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Bookings.pm line
> 23.
> Compilation failed in require at /kohadevbox/koha/Koha/Biblio.pm line 46.
> BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Biblio.pm line 46.
> Compilation failed in require at /kohadevbox/koha/Koha/Biblios.pm line 25.
> BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Biblios.pm line
> 25.
> Compilation failed in require at /kohadevbox/koha/Koha/Libraries.pm line 25.
> BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Libraries.pm line
> 25.
> Compilation failed in require at
> /kohadevbox/koha/Koha/Object/Limit/Library.pm line 22.
> BEGIN failed--compilation aborted at
> /kohadevbox/koha/Koha/Object/Limit/Library.pm line 22.
> Compilation failed in require at /kohadevbox/koha/Koha/AuthorisedValue.pm
> line 26.
> BEGIN failed--compilation aborted at
> /kohadevbox/koha/Koha/AuthorisedValue.pm line 26.
> Compilation failed in require at /kohadevbox/koha/Koha/AuthorisedValues.pm
> line 25.
> BEGIN failed--compilation aborted at
> /kohadevbox/koha/Koha/AuthorisedValues.pm line 25.
> Compilation failed in require at /kohadevbox/koha/C4/Koha.pm line 27.
> BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Koha.pm line 27.
> Compilation failed in require at /kohadevbox/koha/C4/Biblio.pm line 88.
> BEGIN failed--compilation aborted at /kohadevbox/koha/C4/Biblio.pm line 88.
> Compilation failed in require at /kohadevbox/koha/Koha/Z3950Responder.pm
> line 22.
> BEGIN failed--compilation aborted at /kohadevbox/koha/Koha/Z3950Responder.pm
> line 22.
> Compilation failed in require at /kohadevbox/koha/misc/z3950_responder.pl
> line 27.
> BEGIN failed--compilation aborted at
> /kohadevbox/koha/misc/z3950_responder.pl line 27.
>  failed!
> Starting Koha worker daemon for kohadev (default):.
> Starting Koha worker daemon for kohadev (long_tasks):.
> Starting Koha indexing daemon for kohadev:.
> 
> At line 118 of Koha/Booking.pm there is an "if if" - if I remove one of the
> ifs, then everything restarts properly.

Hello David,

Thanks for your feedback. I remove the typo issue ("if if") and rebase my commit into one unique commit by the way.

For the failure message its the original behavior when errors comes but not necessary from circulation rules. I'll look into the details of why it's happening

I miss some js code on my last rebase (due to place_booking.js modal renaming).

Created attachment 166962 [details] [review]
Bug 36271: Add circulation rules for booking

Welcome to 3 new circulation rules :)

- Bookings allowed (total) / bookings_allowed_total
- Bookings per item (total) / bookings_per_item
- Bookings period length (day) / bookings_period_length

Test plan:
1) Apply this patch
2) Set values for these new rules
3) Define a total to 2, 1 per item with 2 days for
   example for booking period.
4) Book an item and retry on the same item, normally an error message is
   showing in the pop up.
6) Book another item, and again another one, on the third an error
   message is showing (always in pop up)
7) You can also try to test with a longer period than bookings period
  length rule, an another error will be displayed.

Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS)

I've renamed this bug to highlight the change in scope.

I'm really interested in the direction here and I think we should continue to refine the rule options in this bug.

However, I'm also keen to submit my original proposal to respect existing rules for loan length and renewals.  I've cloned the bug to bug 37354 and will submit there so we don't end up with conflicting alternative patches here.

Thanks that's a good thing !
I will take a look at the bug you mentioned ;)

I think this needs a rebase:

error: sha1 information is lacking or useless (Koha/Items.pm).
error: could not build fake ancestor

I'm getting:

Missing right curly or square bracket at /kohadevbox/koha/Koha/Item.pm line 2686, at end of line
syntax error at /kohadevbox/koha/Koha/Item.pm line 2686, at EOF

when running reset_all.

Take a look at commit 7608e0ae6d36ad849851e975202b53ca6d674229 and its changes to Koha/Item.pm

Seems something went off there.

Created attachment 170197 [details] [review]
Bug 36271: Add circulation rules for booking

Welcome to 3 new circulation rules :)

- Bookings allowed (total) / bookings_allowed_total
- Bookings per item (total) / bookings_per_item
- Bookings period length (day) / bookings_period_length

Test plan:
1) Apply this patch
2) Set values for these new rules
3) Define a total to 2, 1 per item with 2 days for
   example for booking period.
4) Book an item and retry on the same item, normally an error message is
   showing in the pop up.
6) Book another item, and again another one, on the third an error
   message is showing (always in pop up)
7) You can also try to test with a longer period than bookings period
  length rule, an another error will be displayed.

Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS)

(In reply to Paul Derscheid from comment #31)
> I'm getting:
> 
> Missing right curly or square bracket at /kohadevbox/koha/Koha/Item.pm line
> 2686, at end of line
> syntax error at /kohadevbox/koha/Koha/Item.pm line 2686, at EOF
> 
> when running reset_all.

Thanks to you Paul.. I missed up the last rebase ;) that's fixed

The patch does not apply on the current main branch:


Bug 36271 - Bookings should have circulation rules

170197 - Bug 36271: Add circulation rules for booking

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 36271: Add circulation rules for booking
error: sha1 information is lacking or useless (Koha/Item.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 36271: Add circulation rules for booking

Created attachment 172866 [details] [review]
Bug 36271: Add circulation rules for booking

Welcome to 3 new circulation rules :)

- Bookings allowed (total) / bookings_allowed_total
- Bookings per item (total) / bookings_per_item
- Bookings period length (day) / bookings_period_length

Test plan:
1) Apply this patch
2) Set values for these new rules
3) Define a total to 2, 1 per item with 2 days for
   example for booking period.
4) Book an item and retry on the same item, normally an error message is
   showing in the pop up.
6) Book another item, and again another one, on the third an error
   message is showing (always in pop up)
7) You can also try to test with a longer period than bookings period
  length rule, an another error will be displayed.

Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS)

Created attachment 172924 [details] [review]
Bug 36271: Add circulation rules for booking

Welcome to 3 new circulation rules :)

- Bookings allowed (total) / bookings_allowed_total
- Bookings per item (total) / bookings_per_item
- Bookings period length (day) / bookings_period_length

Test plan:
1) Apply this patch
2) Set values for these new rules
3) Define a total to 2, 1 per item with 2 days for
   example for booking period.
4) Book an item and retry on the same item, normally an error message is
   showing in the pop up.
6) Book another item, and again another one, on the third an error
   message is showing (always in pop up)
7) You can also try to test with a longer period than bookings period
  length rule, an another error will be displayed.

Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS)

Signed-off-by: esther <esther@bywatersolutions.com>

Comment on attachment 172924 [details] [review]
Bug 36271: Add circulation rules for booking

Review of attachment 172924 [details] [review]:
-----------------------------------------------------------------

::: Koha/Booking.pm
@@ +53,5 @@
> +         { status => tooLongBookingPeriod, limit => $limit }, if the borrower has exceeded their maximum booking period.
> +         { status => bookingPeriodNotValid }, if booking period is not valid (undef or equal to 0).
> +=cut
> +
> +sub can_be_booked_in_advance {

Whilst this is great for the serverside logic, the rest of the bookings flow on the client allows for upfront notification as soon as possible regarding bookability.

We should update the bookings modal to react to the choices made as soon as possible as exemplified by recent bugs.

You are 100% correct to take some of the other circulation rules into account, age-related restrictions for example, but these should really have been reported as independent bugs and clear code submitted to block at the API level and also expose quickly in the Modal as soon as the patron is selected for example.

@@ +91,5 @@
> +    my $bookings_period_length = $rights->{bookings_period_length} || 0;
> +
> +    return { status => 'noBookingsAllowed' } if defined($bookings_allowed_total) && $bookings_allowed_total == 0;
> +
> +    my $bookings_per_item_count = Koha::Bookings->search( { patron_id => $patron->borrowernumber, item_id => $item->itemnumber } )->count();

Given that bookings are time bound and not a replenishing queue.. should this rule also be time bound or clarified somehow.

What does it mean?
* "Active" bookings waiting to be fulfilled?
* Bookings allowed in a year, a quarter, a term?

::: Koha/CirculationRules.pm
@@ +227,5 @@
> +    },
> +    bookings_per_item => {
> +        scope => [ 'branchcode', 'categorycode', 'itemtype' ],
> +    },
> +    bookings_period_length => {

With bug 37354 now implemented using existing loan rules to construct durations we probably don't need this one.. or we need to reconsider it concerning the handling introduced in the aforementioned bug.