Bug 23269

Maybe this would be another candidate to be moved to the REST API?

(In reply to Katrin Fischer from comment #1)
> Maybe this would be another candidate to be moved to the REST API?

Johanna has actually worked on this (but is currently absent) and we've been using it for awhile now https://github.com/KohaSuomi/Koha-22x/commit/c26f53eeac4c12e13e1e595931b4ec279f054da7

However, this depends on bug 30660 and bug 30661.

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

(In reply to Emmi Takkinen from comment #2)
> (In reply to Katrin Fischer from comment #1)
> > Maybe this would be another candidate to be moved to the REST API?
> 
> Johanna has actually worked on this (but is currently absent) and we've been
> using it for awhile now
> https://github.com/KohaSuomi/Koha-22x/commit/
> c26f53eeac4c12e13e1e595931b4ec279f054da7
> 
> However, this depends on bug 30660 and bug 30661.

Hi all, is this still the most up to date work in progress?
Would you please add the relevant git repo branch URL to this bug's URL?
I'm available to help out here.

(In reply to Pedro Amorim from comment #4)
> (In reply to Emmi Takkinen from comment #2)
> > (In reply to Katrin Fischer from comment #1)
> > > Maybe this would be another candidate to be moved to the REST API?
> > 
> > Johanna has actually worked on this (but is currently absent) and we've been
> > using it for awhile now
> > https://github.com/KohaSuomi/Koha-22x/commit/
> > c26f53eeac4c12e13e1e595931b4ec279f054da7
> > 
> > However, this depends on bug 30660 and bug 30661.
> 
> Hi all, is this still the most up to date work in progress?
> Would you please add the relevant git repo branch URL to this bug's URL?
> I'm available to help out here.

Yes, I working on it at the moment. We have our tweaked version and I'm trying to improve it to work in main. I will add a URL tomorrow, after I have cleaned some code.

I added a branch URL to this bug. Still needs testing that everything works correctly with permissions, multiple records etc.

Hi Johanna, thank you. If this is ready for testing please set the bug as "Needs Signoff".

(In reply to Pedro Amorim from comment #7)
> Hi Johanna, thank you. If this is ready for testing please set the bug as
> "Needs Signoff".

I will improve it a bit and add a patch for testing after.

Created attachment 173770 [details] [review]
Bug 23269: use REST API for patron_holds_table

This patch uses the REST API to populate the holds table

Test plan:
1) Apply the patch
2) Go to a record with holds
3) Check that the holds table is populated correctly
4) Check that hold tables functions work correctly
5) Change HoldsSplitQueue and HoldsSplitQueueNumbering system preference, try all variations
6) Repeat steps 3 and 4
7) Search for records and place holds to multiple records
8) Repeat steps 3 and 4
9) Try with lesser permissions

Sponsored-by: Koha-Suomi Oy

Finally got the patch here! Splitting the results with HoldsSplitQueue system preference took more time than I expected.

Looking here

This is really impressive work. It works great from what I can tell.
I have some observations, though I don't think any one in particular is a blocker, and some may not need a follow-up and are simply observations:

- Entering an expiration date triggers the same API call twice
- Entering "suspend on" date triggers the same API call twice
- Date inputs are missing the "x" to clear the input
- Entering "suspend on" date immediately suspends the hold. On the previous table you'd have to click the 'suspend' button after inserting the date to effectively suspend it.
- The label for a suspended hold is "Resume", previously was "Unsuspend".
- Trash icon for "delete" icon missing hover pointer cursor.
- If all actions now update holds in real-time, do we still need the "Update hold(s)" button at the top? Previously this was needed to save the changes, but it's not anymore.
- "Print slip" button now always shows, previously it only showed if hold.intransit || hold.atdestination.
- Deleting a hold is triggering table redraw API call multiple times, to reproduce:
-- Place 3 or 4 holds on a patron.
-- Move the priority around multiple times for all holds.
-- Delete one of the holds.
-- Notice the network tab calling the same API endpoint multiple times (sometimes over 10 times)

I think at least the "Update holds" button question and the multiple API calls issues should be addressed. What do you think Johanna? Do you agree? Up to you how best we should follow-up here. I think this is really good as is already.

(In reply to Pedro Amorim from comment #12)
> This is really impressive work. It works great from what I can tell.
> I have some observations, though I don't think any one in particular is a
> blocker, and some may not need a follow-up and are simply observations:
> 
> - Entering an expiration date triggers the same API call twice
> - Entering "suspend on" date triggers the same API call twice
> - Date inputs are missing the "x" to clear the input
> - Entering "suspend on" date immediately suspends the hold. On the previous
> table you'd have to click the 'suspend' button after inserting the date to
> effectively suspend it.
> - The label for a suspended hold is "Resume", previously was "Unsuspend".
> - Trash icon for "delete" icon missing hover pointer cursor.
> - If all actions now update holds in real-time, do we still need the "Update
> hold(s)" button at the top? Previously this was needed to save the changes,
> but it's not anymore.
> - "Print slip" button now always shows, previously it only showed if
> hold.intransit || hold.atdestination.
> - Deleting a hold is triggering table redraw API call multiple times, to
> reproduce:
> -- Place 3 or 4 holds on a patron.
> -- Move the priority around multiple times for all holds.
> -- Delete one of the holds.
> -- Notice the network tab calling the same API endpoint multiple times
> (sometimes over 10 times)
> 
> I think at least the "Update holds" button question and the multiple API
> calls issues should be addressed. What do you think Johanna? Do you agree?
> Up to you how best we should follow-up here. I think this is really good as
> is already.

Thank you for reviewing, this was a big one!

I totally agree with you. We don't actually need the "Update hold(s)" button anymore, I doubted it while I was doing it. Good to hear that you think the same. 

I will optimize those API calls and improve other features as well. Good to get some other to look at this. I was getting blind to all these features.