Bug 26988 - Defer loading the hold pickup locations until the dropdown is selected
Summary: Defer loading the hold pickup locations until the dropdown is selected
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Tomás Cohen Arazi
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on: 26963
Blocks: 27015 27016 27177 27863
  Show dependency treegraph
 
Reported: 2020-11-10 13:35 UTC by Nick Clemens
Modified: 2022-06-06 20:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07


Attachments
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table (9.28 KB, patch)
2020-11-10 13:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table (9.34 KB, patch)
2020-11-10 16:53 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table (9.39 KB, patch)
2020-11-10 17:18 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26988: Make ->patron prefetchable (3.17 KB, patch)
2020-11-10 20:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26988: (QA follow-up) Make the API return proper library objects (2.38 KB, patch)
2020-11-10 20:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table (8.81 KB, patch)
2020-11-11 03:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26988: Make ->patron prefetchable (3.17 KB, patch)
2020-11-11 03:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26988: (QA follow-up) Make the API return proper library objects (2.38 KB, patch)
2020-11-11 03:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26988: (follow-up) Adjust code to use api field names (1.42 KB, patch)
2020-11-11 03:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26988: (follow-up) Fix filter and encode api values and escape rendered values (2.78 KB, patch)
2020-11-11 03:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26988: (follow-up) Change event to focus (1.08 KB, patch)
2020-11-11 15:37 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table (8.83 KB, patch)
2020-11-11 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26988: Make ->patron prefetchable (3.17 KB, patch)
2020-11-11 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26988: (QA follow-up) Make the API return proper library objects (2.38 KB, patch)
2020-11-11 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26988: (follow-up) Adjust code to use api field names (1.48 KB, patch)
2020-11-11 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26988: (follow-up) Fix filter and encode api values and escape rendered values (2.84 KB, patch)
2020-11-11 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26988: (follow-up) Change event to focus (1.14 KB, patch)
2020-11-11 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26988: No need to check if hold is linked to a patron (664 bytes, patch)
2020-11-13 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26988: Adjust for bug 26963 (893 bytes, patch)
2020-11-13 13:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26988: (QA follow-up) Unit tests (4.32 KB, patch)
2020-11-16 16:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-11-10 13:35:12 UTC
Bug 26963 deals with improving the efficiency of the pickup_locations routine.

Currently we are loading these dropdowns for every hold on a bib. It would save loading time to only load the dropdown if a library attempts to edit the pickup location
Comment 1 Nick Clemens 2020-11-10 13:38:13 UTC
Created attachment 113395 [details] [review]
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table

To test:
1 - Place a number of holds on a record
2 - Have different pickup locations for the holds
3 - Have some libraries that are not pickup locations
4 - Load the holds tab for the record and note libraries not pickup locations are not in dropdowns
5 - Apply patch and restart all things
6 - Reload the holds table
7 - Click on a dropdown, note the spinner, should load successfully
8 - Confirm the dropdown matches the options before the patch
9 - Confirm updating the hold location works
Comment 2 Andrew Fuerste-Henry 2020-11-10 16:53:13 UTC
Created attachment 113418 [details] [review]
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table

To test:
1 - Place a number of holds on a record
2 - Have different pickup locations for the holds
3 - Have some libraries that are not pickup locations
4 - Load the holds tab for the record and note libraries not pickup locations are not in dropdowns
5 - Apply patch and restart all things
6 - Reload the holds table
7 - Click on a dropdown, note the spinner, should load successfully
8 - Confirm the dropdown matches the options before the patch
9 - Confirm updating the hold location works

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 3 Andrew Fuerste-Henry 2020-11-10 17:18:05 UTC
Created attachment 113426 [details] [review]
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table

To test:
1 - Place a number of holds on a record
2 - Have different pickup locations for the holds
3 - Have some libraries that are not pickup locations
4 - Load the holds tab for the record and note libraries not pickup locations are not in dropdowns
5 - Apply patch and restart all things
6 - Reload the holds table
7 - Click on a dropdown, note the spinner, should load successfully
8 - Confirm the dropdown matches the options before the patch
9 - Confirm updating the hold location works

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Comment 4 Tomás Cohen Arazi 2020-11-10 20:39:37 UTC
Created attachment 113427 [details] [review]
Bug 26988: Make ->patron prefetchable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2020-11-10 20:39:42 UTC
Created attachment 113428 [details] [review]
Bug 26988: (QA follow-up) Make the API return proper library objects

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Nick Clemens 2020-11-11 03:02:53 UTC
Created attachment 113456 [details] [review]
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table

To test:
1 - Place a number of holds on a record
2 - Have different pickup locations for the holds
3 - Have some libraries that are not pickup locations
4 - Load the holds tab for the record and note libraries not pickup locations are not in dropdowns
5 - Apply patch and restart all things
6 - Reload the holds table
7 - Click on a dropdown, note the spinner, should load successfully
8 - Confirm the dropdown matches the options before the patch
9 - Confirm updating the hold location works

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Comment 7 Nick Clemens 2020-11-11 03:02:56 UTC
Created attachment 113457 [details] [review]
Bug 26988: Make ->patron prefetchable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Nick Clemens 2020-11-11 03:02:59 UTC
Created attachment 113458 [details] [review]
Bug 26988: (QA follow-up) Make the API return proper library objects

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Nick Clemens 2020-11-11 03:03:02 UTC
Created attachment 113459 [details] [review]
Bug 26988: (follow-up) Adjust code to use api field names
Comment 10 Nick Clemens 2020-11-11 03:13:40 UTC
Created attachment 113460 [details] [review]
Bug 26988: (follow-up) Fix filter and encode api values and escape rendered values

This patch makes the QA tools happy and follows correct procedures for passing data to
and rendering data from the api
Comment 11 Tomás Cohen Arazi 2020-11-11 14:37:01 UTC
This is working in Chrome but not on Firefox. We are trying to fix it!
Comment 12 Agustín Moyano 2020-11-11 15:37:40 UTC
Created attachment 113502 [details] [review]
Bug 26988: (follow-up) Change event to focus

Click event on select element if Firefox does not work as expected, so
this patch changes the event to focus for it to work seamlessly in both
browsers.

https://bugs.koha-community.org/show_bug.cgi?id=26899
Comment 13 Tomás Cohen Arazi 2020-11-11 19:16:03 UTC
Created attachment 113533 [details] [review]
Bug 26988: Add API route to fetch hold pickup locations and use it in the holds table

To test:
1 - Place a number of holds on a record
2 - Have different pickup locations for the holds
3 - Have some libraries that are not pickup locations
4 - Load the holds tab for the record and note libraries not pickup locations are not in dropdowns
5 - Apply patch and restart all things
6 - Reload the holds table
7 - Click on a dropdown, note the spinner, should load successfully
8 - Confirm the dropdown matches the options before the patch
9 - Confirm updating the hold location works

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2020-11-11 19:16:12 UTC
Created attachment 113534 [details] [review]
Bug 26988: Make ->patron prefetchable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2020-11-11 19:16:18 UTC
Created attachment 113535 [details] [review]
Bug 26988: (QA follow-up) Make the API return proper library objects

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2020-11-11 19:16:24 UTC
Created attachment 113536 [details] [review]
Bug 26988: (follow-up) Adjust code to use api field names

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi 2020-11-11 19:16:32 UTC
Created attachment 113537 [details] [review]
Bug 26988: (follow-up) Fix filter and encode api values and escape rendered values

This patch makes the QA tools happy and follows correct procedures for passing data to
and rendering data from the api

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2020-11-11 19:16:39 UTC
Created attachment 113538 [details] [review]
Bug 26988: (follow-up) Change event to focus

Click event on select element if Firefox does not work as expected, so
this patch changes the event to focus for it to work seamlessly in both
browsers.

https://bugs.koha-community.org/show_bug.cgi?id=26899
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Jonathan Druart 2020-11-13 10:08:52 UTC
Created attachment 113590 [details] [review]
Bug 26988: No need to check if hold is linked to a patron

Holds are deleted along with patrons.
Comment 20 Nick Clemens 2020-11-13 13:16:30 UTC
Created attachment 113601 [details] [review]
Bug 26988: Adjust for bug 26963
Comment 21 Jonathan Druart 2020-11-13 13:24:24 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 22 Tomás Cohen Arazi 2020-11-16 16:14:42 UTC
Created attachment 113669 [details] [review]
Bug 26988: (QA follow-up) Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2020-11-16 16:46:58 UTC
Dear RM, we forgot the unit tests for the route. The latest patch adds them.
Comment 24 Jonathan Druart 2020-11-17 11:27:12 UTC
(In reply to Tomás Cohen Arazi from comment #22)
> Created attachment 113669 [details] [review] [review]
> Bug 26988: (QA follow-up) Unit tests
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Patch pushed to master.
Comment 25 Andrew Fuerste-Henry 2020-12-08 17:33:20 UTC
Backported to 20.05.x for 20.05.07
Comment 26 Jonathan Druart 2020-12-09 12:20:22 UTC
(In reply to Andrew Fuerste-Henry from comment #25)
> Backported to 20.05.x for 20.05.07

There is a test failing on 20.05.x, Tomas can you have a look please?

t/db_dependent/api/v1/holds.t .. 9/10 
    #   Failed test 'exact match for JSON Pointer ""'
    #   at t/db_dependent/api/v1/holds.t line 768.
    #     Structures begin differing at:
    #          $got = HASH(0x5630131b9158)
    #     $expected = ARRAY(0x5630134b3bf8)
Comment 27 Victor Grousset/tuxayo 2020-12-09 17:00:03 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.
Comment 28 Tomás Cohen Arazi 2021-03-10 11:26:36 UTC
(In reply to Jonathan Druart from comment #26)
> (In reply to Andrew Fuerste-Henry from comment #25)
> > Backported to 20.05.x for 20.05.07
> 
> There is a test failing on 20.05.x, Tomas can you have a look please?
> 
> t/db_dependent/api/v1/holds.t .. 9/10 
>     #   Failed test 'exact match for JSON Pointer ""'
>     #   at t/db_dependent/api/v1/holds.t line 768.
>     #     Structures begin differing at:
>     #          $got = HASH(0x5630131b9158)
>     #     $expected = ARRAY(0x5630134b3bf8)

Is this still the case, Andrew?
Comment 29 Andrew Fuerste-Henry 2021-03-10 13:28:36 UTC
I am not getting that test failure on 20.05.x:
root@kohadevbox:koha(rmain2005)$ prove t/db_dependent/api/v1/holds.t
t/db_dependent/api/v1/holds.t .. ok     
All tests successful.
Files=1, Tests=12, 11 wallclock secs ( 0.03 usr  0.01 sys +  7.19 cusr  1.00 csys =  8.23 CPU)
Result: PASS