Bug 29002

I think this is a great idea, and I have had other libraries ask about being able to do this.  It will be well received!

Created attachment 125456 [details] [review]
Bug 29002: Add bookings table to kohastructure

Created attachment 125457 [details] [review]
Bug 29002: DBIC Schema Build

Created attachment 125458 [details] [review]
Bug 29002: Add Booking objects and API classes

Created attachment 125459 [details] [review]
Bug 29002: Add ability to book material

This patch introduces a new modal to the biblio details page to allow
booking of materials.

Test plan
1) Navigate to the details page of a biblio
2) Note the new 'Place booking' button in the toolbar
3) Click the new button and note the new modal dialogue
4) Enter part of a patron name or cardnumber and then select from the
   presented results
5) Select a start date and end date from the calender
6) Submit

Lots more to do.. but the basics of creating a booking and being able to see the bookings for a biblio are now there..

Next steps...
1) Either add bookings to the 'Holds to pull' page or add a now 'Bookings to collect' page
2) Integrate booking fulfilment into circulation

Hello,
Just stumbling on that bug, we at BibLibre have been using HoldTitle and HoldItems with needed_before_date and expiry_date parameters to hack this functionality in our frontend software (Bokeh).
Althought the demo is in french you could see some visuals showing how it looks.

https://wiki.bokeh-library-portal.org/index.php?title=R%C3%A9servation_calendaire_de_lots

I guess those parameters are also available not only from ILS-DI but also from staff interface.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23531

If the functionality is going to be implemented in Koha separately from the reserve module, then it might be nice to make it available through the API as well.

Current work is distinct from reserves.. although I see many parallels there I felt the 'Queue' nature of reserves vs the 'Booked dates' nature of bookings distinguished them.

As for API, the entire thing is designed from the ground up using modern best-practice.. as such, it will all exposed via the RESTful API's.. see the patches on the gitlab branch ;)

This is all very much a work in progress ;)

(In reply to Martin Renvoize from comment #8)
> Current work is distinct from reserves.. although I see many parallels there
> I felt the 'Queue' nature of reserves vs the 'Booked dates' nature of
> bookings distinguished them.

Indeed, actually it seems even more relevant compared to the hack we made to bring this functionality to Bokeh!
Reserves means it can still be loaned on a conflicting period, or there can be conflicting reserves made from different users.
TLDR; I guess you made things in such a way that this situation cannot happen on your branch?
I admit i didn't go through all your commits... yet ;)

> As for API, the entire thing is designed from the ground up using modern
> best-practice.. as such, it will all exposed via the RESTful API's.. see the
> patches on the gitlab branch ;)

Seen that on https://gitlab.com/mrenvoize/Koha/-/commit/c2d3412d390b5a49cc2998ba9c8c25efd074d4fe

* thumbsup * Thanks for thinking API-wise :)

Also I didn't knew about that practice of linking to a git branch instead of uploading the commits... nice :) are we going to use more gitlab workflows like merge request? From my experience, code reviews are quite easy to read and process on gitlab!

> This is all very much a work in progress ;)

Nice work already! Wish you can make a demo for next hackfest, hopefully in March? :)

Interested to hear how this goes.

We've had calls for this at times, and I've personally worked in libraries where this would've been useful. We ended up using separate booking systems and it was a bit of a pain.

Please have a look at Bug 15261 looks similar
It would be great to share the code that gives same feature.

(In reply to Fridolin Somers from comment #11)
> Please have a look at Bug 15261 looks similar
> It would be great to share the code that gives same feature.

Bug 15261 tries to add this feature to the holds system with some global switches for the behavior, while this keeps it separate. I have to say I am in favor of keeping it separate as it won't be necessary to make it work/play nicely with all the hold related features we already have.

I'm interested in this too. We're using a combination of several reports and a special Withdrawn status right now. 

I tried to test the patch using ByWaters sandbox but I didn't see the button "Place booking" on the biblio detail page. Don't know if I did the setup wrong or something else, I'm new to testing but happy to learn.

*** Bug 31777 has been marked as a duplicate of this bug. ***

Highly interested in this one. We are currently using a separate catalog software for our Library of Things because it allows for reservations. Patrons and staff alike struggle with it and constantly are asking for everything to just be within Koha. Adding this ability would be a top priority for us.

We're having requests for this coming up again as well.

Git branch rebased.. commits a little tidying and functionality needs testing/reviewing.

OK.. I'm in need of help to progress this.. I've hit a point where I'm not sure how to proceed.

The basic code works, but the circ rules are not tied in as yet and I'm struggling to work out how to achieve that.

This may go in as a standalone feature, marked experimental for the 23.05 release

Remote branch is up to date and working on sandboxes at the moment..

# Test plan
1) Navigate to a biblio with at least one item attached
2) Navigate to the 'Items' tab for that biblio
3) Mark at least one item as 'Bookable' using the new select box
4) Note that a new 'Place booking' button should appear in the biblio toolbar
5) Confirm the button triggers a modal dialog allowing one to select a patron, item and booking period
6) Confirm that when creating subsequent bookings the booking period select greys out booking dates that would lead to overlapping bookings
7) Note the new 'Bookings' page available from the left menu
8) Confirm bookings appear correctly in both the table and timelines on this new page
9) Confirm cancelling a booking from the table updates both the table and timeline view
10) Click a booking on the timeline view and note it highlights in yellow and a read cross appears next to it
11) Confirm that clicking the red cross triggers the same cancellation confirmation as above
12) Confirm that is confirmed, the cancellation removes the booking from both the table and timeline
13) Click another booking on the timeline view and now attempt to drag the booking around to edit it
14) Confirm the booking cannot overlap with another booking on the same item
15) Attempt to checkout an item with a booking to a user other than the user who booked the item
16) Confirm that if the checkout would overlap the booking period you are notified of the clash and
16a) Blocked if you do not have permissions to override
16b) Asked to confirm a shorter loan period to ensure the item is returned to fulfil the booking promptly
17) Attempt to checkout an item with a booking to the user the booking is for (but prior to, but close to the booking period) and
17a) Confirm that if you can override the block, you are asked if you wish to checkout early
17b) Confirm that you are blocked from checkout if you are not allowed to override
18) Attempt to checkout the item to the user owning the booking during the booking period
19) Confirm the due date matches the end of the booked period

Martin, I'm testing in the sandbox. At step 5 (click Place booking button) the modal has an empty dropdown for the patron.

http://staff-bug29002.sandboxes.ptfs-europe.co.uk/cgi-bin/koha/catalogue/moredetail.pl?biblionumber=285&itemnumber=631#item631


Is there something I should activate for patrons to appear in there?

Ok I tested this and it's very interesting! Especially for schools. However, I'm not sure it's ready for release yet... Here are my conmments, and I apologize, I know there's a lot... I tried to separate into actual bugs vs comments.
I tested in the sandbox provided.

Bugs:

-  The patron search doesn't work in the modal when you click the Place booking button when in "Item" tab https://snipboard.io/EDpzBr.jpg

-  When you click on the Bookings tab, MARC and ISBD view disapears. 

-  In the bookings tab, the months appear in french (in the sandbox, which I assume only has english installed). https://snipboard.io/fCPapt.jpg Does it take the language from my computer? My Ubuntu is in English, and so is my Chrome...

-  It seems the next day after a booking is not available? Also the calendar view and the table don't show the same dates.
   To recreate:
   1. Click Place booking
   2. Place a booking for two days, for example May 22 and 23
   3. Click Place booking again for the same item
      --> Note that 22, 23 and 24 are greyed out
   4. Place the booking for May 25-26
   5. In the bookings tab, move the second booking to May 24-25
      --> It worked
   6. In the table, the dates are not updated
      --> First booking in the calendar is 22-23, in the table it's 22-24
      --> The second booking (the one that was moved) in the calendar is 24-25, in the table it's 25-27

-  If you checkout early to the correct patron, the due date is still the start of the booking?
   To recreate:
   1. Place a booking for a couple days in the future, for example May 22-23
   2. Attempt to check out to the same patron
   3. Confirm early collection
   4. Click Show checkotus
      --> Due date is May 22

Usability comments:

-  The patron search is not the same as elsewhere. I don't mind this version but for usability I think it would be best to have the same search everywhere.
   (Summary of IRC discussions 
   - Martin said the traditional search doesn't work well with modals
   - Katrin said the traditional search works better for large datasets, so you can limit by library or category
   - Martin said he'd work on a middle ground)
   Maybe an alternative would be to make it work like the holds, i.e. send to a page with the form, the calendar and the table...

-  I didn't catch right away that you had to choose two dates. It's nifty once you know, but elsewhere in Koha there are two field "From:" and "To:".

-  Will it eventually be possible to limit the number of days a patron can book for? Right now, a patron can hog an item for a very long time... I know you said circulation rules will be added later, is that what you meant?

-  The Holds tab shows how many holds there are, maybe the bookings tab should show if there are bookings and how many?

-  In the bookings tab, the calendar shows the booking id (?). This id is not displayed in the table, so it's not very clear what it refers to. I think it would be better to show the patron's name and cardnumber. Maybe with a hover display for short bookings.

-  The calendar should show a fixed number of days. Right now it tries to adapt to the bookings, but if you have a long one or one in the far future, it's all scrunched up https://snipboard.io/5Acelp.jpg I think the next 30 days is sufficient, and we can scroll as needed.

-  In the bookings tab, in the table, not sure why there is a search box for the Actions columns...

-  I would also like the patron's name to have their cardnumber, and maybe a link to their file like in the Holds tab

-  Not sure how I triggered the "It worked" message https://snipboard.io/0KNJzp.jpg I think it was by moving a booking around in the calendar, but I also got it while scrolling in the calendar (although I think the latter was a belated message for the former)
   If it was for updating a booking, it could say "Booking updated".

-  Bookings are not shown in the patron file?

Maybe for later versions...

Maybe in a later version, but it would be nice to be able to choose a period within a day. For example, I have a class from 8 to 12 for which I need some special equipment. I could book the equipment for those hours only and someone else could have it in the afternoon.

Another nice to have would be to be able to book recurrently. If I have my class every Tuesday from september to december, I could book the equipment "every tuesday" or "every other tuesday", etc.

All excellent feedback, thanks for testing Caroline.

The branch is being tested by various people and comments are coming directly to me at the moment.

Just wanted to highlight that this is still moving forward and that I've resolved most of the issues Caroline mentions above already.

I'm currently at the stage of writing unit tests and splitting the overall branch into the various bugs so they are more manageable in testing.

Hi Martin! 

Since you said you worked on it, I had another look at the sandbox with bookings. 

> 
> Bugs:
> 
> -  The patron search doesn't work in the modal when you click the Place
> booking button when in "Item" tab https://snipboard.io/EDpzBr.jpg
> 

This works now!

>
> -  When you click on the Bookings tab, MARC and ISBD view disapears. 
> 

This is also fixed!

>
> -  In the bookings tab, the months appear in french (in the sandbox, which I
> assume only has english installed). https://snipboard.io/fCPapt.jpg Does it
> take the language from my computer? My Ubuntu is in English, and so is my
> Chrome...
> 

I still see the months in French. It's not that big of a deal, but very mysterious.

>
> -  It seems the next day after a booking is not available? Also the calendar
> view and the table don't show the same dates.
>    To recreate:
>    1. Click Place booking
>    2. Place a booking for two days, for example May 22 and 23
>    3. Click Place booking again for the same item
>       --> Note that 22, 23 and 24 are greyed out
>    4. Place the booking for May 25-26
>    5. In the bookings tab, move the second booking to May 24-25
>       --> It worked
>    6. In the table, the dates are not updated
>       --> First booking in the calendar is 22-23, in the table it's 22-24
>       --> The second booking (the one that was moved) in the calendar is
> 24-25, in the table it's 25-27
> 

This is still the case.

My bookings are still taking one extra day than what I select. I'm guessing this is by design? For example, I booked something from Sept. 2 to Sept. 4 https://snipboard.io/X4wGIL.jpg In the calendar view, I see it is booked from the 2nd to the 4th, but in the table view, it says it's booked until the 5th. https://snipboard.io/zmD3fu.jpg 

When I move it manually, it is not updated in the table. https://snipboard.io/3fZbxj.jpg

>
> -  If you checkout early to the correct patron, the due date is still the
> start of the booking?
>    To recreate:
>    1. Place a booking for a couple days in the future, for example May 22-23
>    2. Attempt to check out to the same patron
>    3. Confirm early collection
>    4. Click Show checkotus
>       --> Due date is May 22
> 

This was fixed! Due date is now end date of the booking!

>
> Usability comments:
> 
> -  The patron search is not the same as elsewhere. I don't mind this version
> but for usability I think it would be best to have the same search
> everywhere.
>    (Summary of IRC discussions 
>    - Martin said the traditional search doesn't work well with modals
>    - Katrin said the traditional search works better for large datasets, so
> you can limit by library or category
>    - Martin said he'd work on a middle ground)
>    Maybe an alternative would be to make it work like the holds, i.e. send
> to a page with the form, the calendar and the table...
> 

I honestly don't remember what the patron search was before, if you changed it. It is still different from any other patron search in Koha, and doesn't allow to limit by library or category like Katrin mentioned. I don't think this would be a blocker, I think most people would scan the card or type in the name. But maybe something to think about in the future as we try to make Koha uniform and consistent.

>
> -  I didn't catch right away that you had to choose two dates. It's nifty
> once you know, but elsewhere in Koha there are two field "From:" and "To:".
> 

This is still the case, although this time I knew! :P

>
> -  Will it eventually be possible to limit the number of days a patron can
> book for? Right now, a patron can hog an item for a very long time... I know
> you said circulation rules will be added later, is that what you meant?
> 

I don't think it this would be a blocker for now. However, I think we'd need circulation rules shortly after this is added.

You said you weren't sure how to tie the circ rules in. I would see it as other columns added to the gigantic circ rules table. 

Minimally :
- Booking allowed (number of "open" bookings patrons of this category can have on items of this type at a time, default 0)
- Maximum booking period (maximum number of days (or hours if we eventually allow this) each booking can be)
- Total bookings allowed (in the second table, maximum number of bookings per patron regardless of category or item type)
- Total bookings allowed (in the third table, maximum number of bookings per patron of this category, regardless of item type)

I also thought bookings could be set by item type (like, all LAPTOP items could be bookable by default, etc.). Then you could use the flag on an item-per-item basis if needed. It's just that if you have a lot of items you want to be bookable, it's a pain to change them one by one as it is currently not an option in the items batch modification form. That could be in a follow-up.

>
> -  The Holds tab shows how many holds there are, maybe the bookings tab
> should show if there are bookings and how many?
> 

This was added, thank you! I noticed however that when you add a booking, the number doesn't automatically go up. You have to refresh the page. I don't think this is blocking for now.

> -  In the bookings tab, the calendar shows the booking id (?). This id is
> not displayed in the table, so it's not very clear what it refers to. I
> think it would be better to show the patron's name and cardnumber. Maybe
> with a hover display for short bookings.
> 

I still think it would be better to show something else than the id in the calendar. Or show the ID in the table so that we can know which is which. 

>
> -  The calendar should show a fixed number of days. Right now it tries to
> adapt to the bookings, but if you have a long one or one in the far future,
> it's all scrunched up https://snipboard.io/5Acelp.jpg I think the next 30
> days is sufficient, and we can scroll as needed.
> 

This was fixed! Thank you!

>
> -  In the bookings tab, in the table, not sure why there is a search box for
> the Actions columns...
> 

The search box is still there. Not a blocker, just weird.

>
> -  I would also like the patron's name to have their cardnumber, and maybe a
> link to their file like in the Holds tab
> 

The link was added! There is still no cardnumber. Not blocking, but it is not consistent with the rest of Koha.

>
> -  Not sure how I triggered the "It worked" message
> https://snipboard.io/0KNJzp.jpg I think it was by moving a booking around in
> the calendar, but I also got it while scrolling in the calendar (although I
> think the latter was a belated message for the former)
>    If it was for updating a booking, it could say "Booking updated".
> 

Still the case. I don't think this is blocking, but I would like to see more meaningful messages.

>
> -  Bookings are not shown in the patron file?
> 

This was added! Thank you!


New (?) blocker bugs

-  When I select 'Any item' (https://snipboard.io/X4wGIL.jpg), the booking is placed for the specific item (https://snipboard.io/zmD3fu.jpg). I thought at first it was because there was only one item that was bookable on the record, but I made more items bookable after the booking was made and it stayed on the specific item. And when I added another booking for 'Any item' (https://snipboard.io/BPw8iy.jpg) it put it for the same specific item (https://snipboard.io/w4YBk9.jpg). It also doesn't block the booked dates if I try to make another booking for 'Any item' (https://snipboard.io/iNVWS4.jpg).
I tried placing a new booking for 'Any item' with the same dates, and it just added it to the same row (https://snipboard.io/nJhEk0.jpg).

- I noticed in the Bookings tab of the patron file that there are hours to the start and end dates (https://snipboard.io/P85tHo.jpg). I don't know how I managed to make a booking from 4 to 3:58... If we permit bookings with hours, we should be able to choose them when making the booking, and show the hours everywhere. If not, then displaying only date would be sufficient (and have the saved date be 00:00 to 23:59?).

- I don't have access to the DB in the sandbox, but I made a report to see what it looked like. This might be philosophical, but I think we should stick to borrowernumber, biblionumber and itemnumber as column names. Unless it was decided otherwise in a dev meeting that now borrowers are patrons and everything is _id in the DB...


New usability comments

- Would it be possible to add an 'Edit' button? I know we can change the dates by double-clicking or long-clicking and then sliding the booking sideways in the calendar view, but I don't find this intuitive. I was trying to slide the booking and it was sliding the whole calendar (because I hadn't double-clicked on it first), and I expected double-clicking to open the modal to edit the booking... Mind you, I might just be weird :) . I wonder if people might also want to change the item on which the booking is. Mostly everywhere else in Koha there is an Edit button in the Actions column. 

- In the Bookings tab of the patron file :

  - If possible, could you add links to the bookings tab of the biblio record under the title (like in the Holds tab).

  - The table says it was filtered from X total entries, which I'm guessing is the total number of bookings (https://snipboard.io/P85tHo.jpg). I find this confusing, I'm expecting this patron to have 31 bookings but I somehow filtered it to show only two.

  - There is also a search box in the Actions column.  


This is it for now! Sorry, I know it's a lot again...

(In reply to Caroline Cyr La Rose from comment #31)
> I still see the months in French. It's not that big of a deal, but very
> mysterious.

Can this one wait for a follow-up bug.. it's in the depths of the library we're using for the timeline view.

 
> >
> > -  It seems the next day after a booking is not available? Also the calendar
> > view and the table don't show the same dates.
> >    To recreate:
> >    1. Click Place booking
> >    2. Place a booking for two days, for example May 22 and 23
> >    3. Click Place booking again for the same item
> >       --> Note that 22, 23 and 24 are greyed out
> >    4. Place the booking for May 25-26
> >    5. In the bookings tab, move the second booking to May 24-25
> >       --> It worked
> >    6. In the table, the dates are not updated
> >       --> First booking in the calendar is 22-23, in the table it's 22-24
> >       --> The second booking (the one that was moved) in the calendar is
> > 24-25, in the table it's 25-27

> My bookings are still taking one extra day than what I select. I'm guessing
> this is by design? For example, I booked something from Sept. 2 to Sept. 4
> https://snipboard.io/X4wGIL.jpg In the calendar view, I see it is booked
> from the 2nd to the 4th, but in the table view, it says it's booked until
> the 5th. https://snipboard.io/zmD3fu.jpg 

This is a sandboxes issue.. we can't set the correct location in the koha-conf to match your locality and as such timezone handling doesn't work.  I've got an ongoing discussion about this with Jonathan D.. it's not a new bug here.. it's just that this work highlights it particularly strongly.
 
> When I move it manually, it is not updated in the table.
> https://snipboard.io/3fZbxj.jpg

Working on this now, seems a regression in rebasing.

> >
> > Usability comments:
> > 
> > -  The patron search is not the same as elsewhere. I don't mind this version
> > but for usability I think it would be best to have the same search
> > everywhere.
> >    (Summary of IRC discussions 
> >    - Martin said the traditional search doesn't work well with modals
> >    - Katrin said the traditional search works better for large datasets, so
> > you can limit by library or category
> >    - Martin said he'd work on a middle ground)
> >    Maybe an alternative would be to make it work like the holds, i.e. send
> > to a page with the form, the calendar and the table...
> > 
> 
> I honestly don't remember what the patron search was before, if you changed
> it. It is still different from any other patron search in Koha, and doesn't
> allow to limit by library or category like Katrin mentioned. I don't think
> this would be a blocker, I think most people would scan the card or type in
> the name. But maybe something to think about in the future as we try to make
> Koha uniform and consistent.

I'm reverting to just using the patron_autocomplete function we have elsewhere and will deal with converting those to a nicer select2 implementation in a distinct bug.

> 
> I also thought bookings could be set by item type (like, all LAPTOP items
> could be bookable by default, etc.). Then you could use the flag on an
> item-per-item basis if needed. It's just that if you have a lot of items you
> want to be bookable, it's a pain to change them one by one as it is
> currently not an option in the items batch modification form. That could be
> in a follow-up.

The switch to item specific level was at ByWaters request.  I think adding options for itemtype level should be a later bug hanging off this one too.

> New (?) blocker bugs
> 
> -  When I select 'Any item' (https://snipboard.io/X4wGIL.jpg), the booking
> is placed for the specific item (https://snipboard.io/zmD3fu.jpg). I thought
> at first it was because there was only one item that was bookable on the
> record, but I made more items bookable after the booking was made and it
> stayed on the specific item. And when I added another booking for 'Any item'
> (https://snipboard.io/BPw8iy.jpg) it put it for the same specific item
> (https://snipboard.io/w4YBk9.jpg). It also doesn't block the booked dates if
> I try to make another booking for 'Any item'
> (https://snipboard.io/iNVWS4.jpg).
> I tried placing a new booking for 'Any item' with the same dates, and it
> just added it to the same row (https://snipboard.io/nJhEk0.jpg).
> 
> - I noticed in the Bookings tab of the patron file that there are hours to
> the start and end dates (https://snipboard.io/P85tHo.jpg). I don't know how
> I managed to make a booking from 4 to 3:58... If we permit bookings with
> hours, we should be able to choose them when making the booking, and show
> the hours everywhere. If not, then displaying only date would be sufficient
> (and have the saved date be 00:00 to 23:59?).
> 
> - I don't have access to the DB in the sandbox, but I made a report to see
> what it looked like. This might be philosophical, but I think we should
> stick to borrowernumber, biblionumber and itemnumber as column names. Unless
> it was decided otherwise in a dev meeting that now borrowers are patrons and
> everything is _id in the DB...
> 

Working on these now.

Sandbox updated.

(In reply to Caroline Cyr La Rose from comment #31)
> > Usability comments:
> > - Patron search inconsistency.

Working on this at the moment, reverting to 'patron_autocomplete' as per various places in the UI.

> >
> > -  The Holds tab shows how many holds there are, maybe the bookings tab
> > should show if there are bookings and how many?
> > 
> 
> This was added, thank you! I noticed however that when you add a booking,
> the number doesn't automatically go up. You have to refresh the page. I
> don't think this is blocking for now.

Fixed on the sandbox (Please check both booking placement and cancellation and try the cancellation from all locations (including the patron details page)

> > -  In the bookings tab, the calendar shows the booking id (?). This id is
> > not displayed in the table, so it's not very clear what it refers to. I
> > think it would be better to show the patron's name and cardnumber. Maybe
> > with a hover display for short bookings.
> 
> I still think it would be better to show something else than the id in the
> calendar. Or show the ID in the table so that we can know which is which. 

Fixed on the sandbox 

> >
> > -  In the bookings tab, in the table, not sure why there is a search box for
> > the Actions columns...
> > 
> 
> The search box is still there. Not a blocker, just weird.

This is weird, I've removed the search header for now.. seems that's an issue with the kohaTable implimentation.. all the rows are getting shifted one to the left for this header row.  I'll resolve that in a new bug.

> >
> > -  I would also like the patron's name to have their cardnumber, and maybe a
> > link to their file like in the Holds tab
> > 
> 
> The link was added! There is still no cardnumber. Not blocking, but it is
> not consistent with the rest of Koha.

Fixed on the sandbox

> >
> > -  Not sure how I triggered the "It worked" message
> > https://snipboard.io/0KNJzp.jpg I think it was by moving a booking around in
> > the calendar, but I also got it while scrolling in the calendar (although I
> > think the latter was a belated message for the former)
> >    If it was for updating a booking, it could say "Booking updated".
> > 
> 
> Still the case. I don't think this is blocking, but I would like to see more
> meaningful messages.

Message removed, I'm not sure how best to give feedback.. I feel like you don't need something to dismiss, but we don't really have a consistent way of doing this yet in Koha.. another one for a future followup I think.

> 
> New (?) blocker bugs
> 
> -  When I select 'Any item' (https://snipboard.io/X4wGIL.jpg), the booking
> is placed for the specific item (https://snipboard.io/zmD3fu.jpg). I thought
> at first it was because there was only one item that was bookable on the
> record, but I made more items bookable after the booking was made and it
> stayed on the specific item. And when I added another booking for 'Any item'
> (https://snipboard.io/BPw8iy.jpg) it put it for the same specific item
> (https://snipboard.io/w4YBk9.jpg). It also doesn't block the booked dates if
> I try to make another booking for 'Any item'
> (https://snipboard.io/iNVWS4.jpg).
> I tried placing a new booking for 'Any item' with the same dates, and it
> just added it to the same row (https://snipboard.io/nJhEk0.jpg).

The 'Any item' functionality was changed for now to autoselect an item for you at booking placement time.  The reason being, I could get the bookings clashing logic to work 100% of the time without selecting an item immediately.  I intend to revisit this once we've got the foundations in.

> - I noticed in the Bookings tab of the patron file that there are hours to
> the start and end dates (https://snipboard.io/P85tHo.jpg). I don't know how
> I managed to make a booking from 4 to 3:58... If we permit bookings with
> hours, we should be able to choose them when making the booking, and show
> the hours everywhere. If not, then displaying only date would be sufficient
> (and have the saved date be 00:00 to 23:59?).

I removed the time component on the table on this page now.. It was an error leaving it as $datetime instead of $date.

> - I don't have access to the DB in the sandbox, but I made a report to see
> what it looked like. This might be philosophical, but I think we should
> stick to borrowernumber, biblionumber and itemnumber as column names. Unless
> it was decided otherwise in a dev meeting that now borrowers are patrons and
> everything is _id in the DB...

I can do this. :(

>
> New usability comments
> 
> - Would it be possible to add an 'Edit' button? I know we can change the
> dates by double-clicking or long-clicking and then sliding the booking
> sideways in the calendar view, but I don't find this intuitive. I was trying
> to slide the booking and it was sliding the whole calendar (because I hadn't
> double-clicked on it first), and I expected double-clicking to open the
> modal to edit the booking... Mind you, I might just be weird :) . I wonder
> if people might also want to change the item on which the booking is. Mostly
> everywhere else in Koha there is an Edit button in the Actions column.

I used an edit button initially, but again it was difficult to work with.. However I am working on restoring it now.

>
> - In the Bookings tab of the patron file :
> 
>   - If possible, could you add links to the bookings tab of the biblio
> record under the title (like in the Holds tab).

Fixed on the sandbox

> 
>   - The table says it was filtered from X total entries, which I'm guessing
> is the total number of bookings (https://snipboard.io/P85tHo.jpg). I find
> this confusing, I'm expecting this patron to have 31 bookings but I somehow
> filtered it to show only two.

We filter out past bookings by default here.. perhaps that needs rethinking.

> 
>   - There is also a search box in the Actions column.  

Removed the header search row again, as above.
 
> 
> This is it for now! Sorry, I know it's a lot again...

Thanks for testing.. it's all great feedback.. but I must admit I'm struggling to keep on top of it all.

(In reply to Martin Renvoize from comment #33)
> (In reply to Caroline Cyr La Rose from comment #31)
> > > Usability comments:
> > > - Patron search inconsistency.
> 
> Working on this at the moment, reverting to 'patron_autocomplete' as per
> various places in the UI.

This actually became a lot more involved once I started with the work to expose an edit modal.. I've left it as a select, but made it display results more in line with what the patron_autocomplete box does.

> >
> > New usability comments
> > 
> > - Would it be possible to add an 'Edit' button? I know we can change the
> > dates by double-clicking or long-clicking and then sliding the booking
> > sideways in the calendar view, but I don't find this intuitive. I was trying
> > to slide the booking and it was sliding the whole calendar (because I hadn't
> > double-clicked on it first), and I expected double-clicking to open the
> > modal to edit the booking... Mind you, I might just be weird :) . I wonder
> > if people might also want to change the item on which the booking is. Mostly
> > everywhere else in Koha there is an Edit button in the Actions column.
> 
> I used an edit button initially, but again it was difficult to work with..
> However I am working on restoring it now.

Done and pushed to the remote branch.

Let me know when the sandbox is ready for testing again!

(In reply to Caroline Cyr La Rose from comment #35)
> Let me know when the sandbox is ready for testing again!

This should be up and ready

Tested on ptfs sandbox following the test plan.

13) Trying to drag the booking to edit it : in the modal, sometimes, the start date does not display, so editing fails : it is when you have several items booked, with periods covering some same days. For an item being the only one booked in the period, it works.
It seems necessary to refresh Bookings page after each modification done from the timeline : when trying to change another period, the start date does not display, even if it is the only booking for these dates

Bookings tab, in patron details or check out page, remains empty.

Other remarks : 
When an item is previously checked out, the period is still available in the calendar, allowing booking for the period when the item is checked out, I think we should also control the check out periods for an item.

When a booked item is checked out for the patron who asked for it, it remains in the bookings : is that wanted ?

The sandbox does not exist anymore, when I create a new sandbox or via koha-testing-docker there is no patch for 29002.
Could you install the patch again on a new sandbox ? Thanks in advance.

Hi all, 

Sorry for the dark here, I've been rebasing and fixing some further bugs highlighted during testing.

There are now a series of sandboxes setup for different timezones on the ptfs-e sandbox system.

I am continuing to iterate still as feedback comes in.

Sandbox updated.

We now handle checked out items and I've worked hard to remove the various issues reported around the async opening/editing of bookings so dragging and other editing should not work much more reliably and keep the timeline and table views updated.

Everything ok for me following the test plan.
We cannot add booking when item is already checked out

If we display a record, then on other tab check out item or change an existing booking period, we must refresh the record display so that available periods in the calendar of the booking form are updated. Useful to know when testing, but I think not a problem in real conditions of use in the library.

May still be a problem : when a booked item is checked out for the patron who asked for it, it remains in the bookings table.

Thanks for all the testing everyone, it's been really helpful having this feedback :)

(In reply to Martin Renvoize from comment #42)
> Thanks for all the testing everyone, it's been really helpful having this
> feedback :)

https://monosnap.com/file/IiPJSKVtDkBOoR0toNlxBkDydMXqt0

It looks like I was able to place an out of bounds booking? More importantly, it seems that one patron cannot book something twice, but there's no feedback other than the patron's name not being selectable.

Follow-up added to branch to deal with that case.. weirdly the recommended way to clear a select2 set doesn't actually clean up after itself fully so we need an additional call to .empty() to drop all the option tags it may have added in it's "first life".

Passing QA! I'll leave it to Martin and Tomas to deal with added sign-offs to the commits since they are on a gitlab branch ;)

Pushed to master for 23.11.

Nice work everyone, thanks!

Created attachment 158357 [details] [review]
Bug 29002: (follow-up) Fix Permissions test

(In reply to Martin Renvoize from comment #47)
> Created attachment 158357 [details] [review] [review]
> Bug 29002: (follow-up) Fix Permissions test

Follow-up pushed to master. Thanks!

Created attachment 158358 [details] [review]
Bug 29002: (follow-up) Make CanBook more resilient

This patch adds a check for biblio existance prior to method resolution
failure in Koha::Template::Plugin::Biblio.

This fixes a failure case in unit tests.

New feature not pushed to 23.05.x

Be aware that if you use the koha-plugin-room-reservations plugin, the plugin will create a database table titled "bookings", so the updatedatabase.pl Koha script will fail when it tries to create the table by the same name.
Here: 
https://github.com/Koha-Community/Koha/blob/0e6d576d70630ba2812ae02461be166ceb5d451f/installer/data/mysql/db_revs/230600061.pl#L13

checks only if the table exists so it skips the creation of the right structure of the table.

If a patron has HTML character in surname or firstname, Select2 will escapeHtml twice.
To recreate edit a patron, set firstname: <span>test</span>
Place booking, search for test
It shows correctly in the result, but when you select it you will see "&lt;", etc.

(In reply to Jonathan Druart from comment #52)
> If a patron has HTML character in surname or firstname, Select2 will
> escapeHtml twice.
> To recreate edit a patron, set firstname: <span>test</span>
> Place booking, search for test
> It shows correctly in the result, but when you select it you will see
> "&lt;", etc.

This should fix the problem.

diff --git a/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js b/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
index 4cf49eb0da5..e8a5b0982c5 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
@@ -100,7 +100,7 @@ $('#placeBookingModal').on('show.bs.modal', function(e) {
                 return patron.text;
             }
             return (
-                escape_str(patron.surname) + ", " + escape_str(patron.firstname)
+                patron.surname + ", " + patron.firstname
             );
         },
         placeholder: "Search for a patron"

Logged in patron does not have permission to view patron's info. The "Patron" column in the booking table is "()" which is a link to moremember?borrowernumber=null

https://snipboard.io/biXIHv.jpg

koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js

This file has tons of trailing whitespaces... Must have been caught by the QA script!

(In reply to George Veranis from comment #51)
> Be aware that if you use the koha-plugin-room-reservations plugin, the
> plugin will create a database table titled "bookings", so the
> updatedatabase.pl Koha script will fail when it tries to create the table by
> the same name.
> Here: 
> https://github.com/Koha-Community/Koha/blob/
> 0e6d576d70630ba2812ae02461be166ceb5d451f/installer/data/mysql/db_revs/
> 230600061.pl#L13
> 
> checks only if the table exists so it skips the creation of the right
> structure of the table.

The plugin itself has now been updated to no longer conflict.. I'm opening a bug to work on the Koha core side now.

Created attachment 160831 [details] [review]
Bug 29002: Remove whitespace only lines

Jonathan highlighted some trailing whitespace.. I only see a few cases
where a line only contains whitespace and I didn't see these caught by
the QA script at the time of submission.

Anyway, this removes the spaces

(In reply to Jonathan Druart from comment #55)
> koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
> 
> This file has tons of trailing whitespaces... Must have been caught by the
> QA script!

See patch above to resolve this

(In reply to Jonathan Druart from comment #53)
> (In reply to Jonathan Druart from comment #52)
> > If a patron has HTML character in surname or firstname, Select2 will
> > escapeHtml twice.
> > To recreate edit a patron, set firstname: <span>test</span>
> > Place booking, search for test
> > It shows correctly in the result, but when you select it you will see
> > "&lt;", etc.
> 
> This should fix the problem.
> 
> diff --git a/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
> b/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
> index 4cf49eb0da5..e8a5b0982c5 100644
> --- a/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
> +++ b/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
> @@ -100,7 +100,7 @@ $('#placeBookingModal').on('show.bs.modal', function(e) {
>                  return patron.text;
>              }
>              return (
> -                escape_str(patron.surname) + ", " +
> escape_str(patron.firstname)
> +                patron.surname + ", " + patron.firstname
>              );
>          },
>          placeholder: "Search for a patron"

Submitted your proposed patch to bug 35772

As this has already been released, I'd prefer to have the follow-ups on separate bugs. For example if the whitespace patch was to be backported for easier compatibility with other fixes, the release notes would be confusing.

What's the purpose of
  $biblio->bookable_items
?

It is simply calling $self->items->filter_by_bookable.
I would prefer to have it replaced with this filter_by call, it's more readable.

That's a LOT of methods not covered by tests!

Martin, I believe the last patch is not in master yet - could you please move it to a separate bug?

Moved into the Unit tests bug.. it's not at all related but it gets it done and I really don't think it needs an entirely new bug