Bug 26633 - Add advanced editor for transfer limits
Summary: Add advanced editor for transfer limits
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 27673
Blocks:
  Show dependency treegraph
 
Reported: 2020-10-08 15:13 UTC by Kyle M Hall
Modified: 2021-12-13 21:12 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds an "advanced" editor for transfer limits that displays all to/from library combinations in a grid and allows them to be edited in a manner somewhat similar to the transport cost matrix editor.
Version(s) released in:
21.05.00


Attachments
Bug 26633: Add REST API for managing transfer limits (30.54 KB, patch)
2020-10-08 17:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Add advanced editor for transfer limits (22.33 KB, patch)
2020-10-08 17:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Add REST API for managing transfer limits (30.78 KB, patch)
2020-10-23 23:29 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 26633: Add REST API for managing transfer limits (30.78 KB, patch)
2020-10-23 23:30 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 26633: Add advanced editor for transfer limits (22.56 KB, patch)
2020-10-23 23:31 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 26633: Fix indirect object notation (736 bytes, patch)
2020-10-25 01:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Change label to "collection" as we display the description (1.22 KB, patch)
2020-10-25 01:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row (2.18 KB, patch)
2020-10-27 18:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Add REST API for managing transfer limits (30.84 KB, patch)
2020-10-31 23:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Add advanced editor for transfer limits (22.61 KB, patch)
2020-10-31 23:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Fix indirect object notation (793 bytes, patch)
2020-10-31 23:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Change label to "collection" as we display the description (1.27 KB, patch)
2020-10-31 23:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row (2.23 KB, patch)
2020-10-31 23:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Add REST API for managing transfer limits (30.88 KB, patch)
2020-11-03 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26633: Add advanced editor for transfer limits (22.66 KB, patch)
2020-11-03 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26633: Fix indirect object notation (850 bytes, patch)
2020-11-03 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Change label to "collection" as we display the description (1.33 KB, patch)
2020-11-03 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row (2.28 KB, patch)
2020-11-03 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Tests on error condition (2.62 KB, patch)
2020-11-03 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26633: POD for the exceptions file (1.70 KB, patch)
2020-11-03 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26633: Remove jquery.checkboxes.min.js include (123.54 KB, patch)
2020-11-05 09:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26633: Add REST API for managing transfer limits (30.88 KB, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Add advanced editor for transfer limits (22.67 KB, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Fix indirect object notation (864 bytes, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Change label to "collection" as we display the description (1.34 KB, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row (2.29 KB, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Tests on error condition (2.63 KB, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: POD for the exceptions file (1.71 KB, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Remove jquery.checkboxes.min.js include (1.12 KB, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Convert transfer_limit.json to YAML (19.08 KB, patch)
2020-11-06 18:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Add API documentation (2.92 KB, patch)
2020-11-06 18:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Add ability to bookmark the page you are viewing (1.43 KB, patch)
2020-11-06 18:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633 (QA follow-up) Add error handling for loading transfer limits table (2.49 KB, patch)
2020-11-16 17:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633 (QA follow-up) Add error handling for loading transfer limits table (2.56 KB, patch)
2020-11-16 17:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633 - Add advanced editor link on admin home page (1.60 KB, patch)
2020-11-24 17:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Add error handling for loading transfer limits table (2.56 KB, patch)
2021-02-09 19:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Add advanced editor link on admin home page (1.60 KB, patch)
2021-02-09 19:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26633: Add REST API for managing transfer limits (30.95 KB, patch)
2021-02-17 19:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Add advanced editor for transfer limits (22.72 KB, patch)
2021-02-17 19:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Fix indirect object notation (906 bytes, patch)
2021-02-17 19:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Change label to "collection" as we display the description (1.38 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row (2.34 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Tests on error condition (2.67 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: POD for the exceptions file (1.75 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Remove jquery.checkboxes.min.js include (1.16 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Convert transfer_limit.json to YAML (19.13 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Add API documentation (2.96 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Add ability to bookmark the page you are viewing (1.48 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: (QA follow-up) Add error handling for loading transfer limits table (2.61 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26633: Add advanced editor link on admin home page (1.64 KB, patch)
2021-02-17 19:04 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-10-08 15:13:57 UTC
The current transfer limits editor works great for Koha instances with small numbers of branches. However, for consortiums with dozens or even hundreds of libraries, the editor does not work well or effectively.

We should provide an "advanced" editor displays all to/from library combinations in a grid and allows them to be edited in a manner somewhat similar to the transport cost matrix editor.
Comment 1 Kyle M Hall 2020-10-08 17:42:57 UTC
Created attachment 111380 [details] [review]
Bug 26633: Add REST API for managing transfer limits

Test Plan:
1) prove t/db_dependent/api/v1/transfer_limits.t
Comment 2 Kyle M Hall 2020-10-08 17:43:29 UTC
Created attachment 111381 [details] [review]
Bug 26633: Add advanced editor for transfer limits

The current transfer limits editor works great for Koha instances with
small numbers of branches. However, for consortiums with dozens or even
hundreds of libraries, the editor does not work well or effectively.

We should provide an "advanced" editor displays all to/from library
combinations in a grid and allows them to be edited in a manner somewhat
similar to the transport cost matrix editor.

Test Plan:
1) Apply this patch
2) Browse to the transfer limits editor
3) Click the new "Switch to advanced editor" link
4) Select a collection code/item type to edit limits for
5) Test the "Check all" function
6) Test the "Uncheck all" function
7) Test the "Check" column function
8) Test the "Uncheck" column function
9) Test the "Check" row function
10) Test the "Uncheck" row function
11) Test individual checkboxes/table cells
Comment 3 Lisette Scheer 2020-10-23 23:29:24 UTC
Created attachment 112327 [details] [review]
Bug 26633: Add REST API for managing transfer limits

Test Plan:
1) prove t/db_dependent/api/v1/transfer_limits.t


Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 4 Lisette Scheer 2020-10-23 23:30:04 UTC
Created attachment 112328 [details] [review]
Bug 26633: Add REST API for managing transfer limits

Test Plan:
1) prove t/db_dependent/api/v1/transfer_limits.t

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 5 Lisette Scheer 2020-10-23 23:31:32 UTC
Created attachment 112330 [details] [review]
Bug 26633: Add advanced editor for transfer limits

The current transfer limits editor works great for Koha instances with
small numbers of branches. However, for consortiums with dozens or even
hundreds of libraries, the editor does not work well or effectively.

We should provide an "advanced" editor displays all to/from library
combinations in a grid and allows them to be edited in a manner somewhat
similar to the transport cost matrix editor.

Test Plan:
1) Apply this patch
2) Browse to the transfer limits editor
3) Click the new "Switch to advanced editor" link
4) Select a collection code/item type to edit limits for
5) Test the "Check all" function
6) Test the "Uncheck all" function
7) Test the "Check" column function
8) Test the "Uncheck" column function
9) Test the "Check" row function
10) Test the "Uncheck" row function
11) Test individual checkboxes/table cells

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 6 Lisette Scheer 2020-10-23 23:35:51 UTC
Worked great, our consortium will be happy to have this, especially if we have to close again like we did last spring.
Comment 7 Katrin Fischer 2020-10-25 01:04:44 UTC
Created attachment 112437 [details] [review]
Bug 26633: Fix indirect object notation
Comment 8 Katrin Fischer 2020-10-25 01:04:50 UTC
Created attachment 112438 [details] [review]
Bug 26633: (QA follow-up) Change label to "collection" as we display the description
Comment 9 Katrin Fischer 2020-10-25 01:06:07 UTC
Hi Kyle, 

I have posted 2 small follow-ups, but then got stuck on an issue:

Whenever you click any of the buttons ("Uncheck all" , "Check all") a second time, all the buttons get greyed out and no other action can be performed until the page is reloaded.
Comment 10 Kyle M Hall 2020-10-27 18:02:50 UTC
Created attachment 112587 [details] [review]
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row
Comment 11 Katrin Fischer 2020-10-31 23:30:21 UTC
Created attachment 112778 [details] [review]
Bug 26633: Add REST API for managing transfer limits

Test Plan:
1) prove t/db_dependent/api/v1/transfer_limits.t

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2020-10-31 23:30:26 UTC
Created attachment 112779 [details] [review]
Bug 26633: Add advanced editor for transfer limits

The current transfer limits editor works great for Koha instances with
small numbers of branches. However, for consortiums with dozens or even
hundreds of libraries, the editor does not work well or effectively.

We should provide an "advanced" editor displays all to/from library
combinations in a grid and allows them to be edited in a manner somewhat
similar to the transport cost matrix editor.

Test Plan:
1) Apply this patch
2) Browse to the transfer limits editor
3) Click the new "Switch to advanced editor" link
4) Select a collection code/item type to edit limits for
5) Test the "Check all" function
6) Test the "Uncheck all" function
7) Test the "Check" column function
8) Test the "Uncheck" column function
9) Test the "Check" row function
10) Test the "Uncheck" row function
11) Test individual checkboxes/table cells

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2020-10-31 23:30:30 UTC
Created attachment 112780 [details] [review]
Bug 26633: Fix indirect object notation

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2020-10-31 23:30:34 UTC
Created attachment 112781 [details] [review]
Bug 26633: (QA follow-up) Change label to "collection" as we display the description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2020-10-31 23:30:37 UTC
Created attachment 112782 [details] [review]
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2020-10-31 23:31:24 UTC
Do we need any documentation for the API routes here?
Comment 17 Jonathan Druart 2020-11-03 13:38:28 UTC
Tomas, can I get your stamp on this one please?
Comment 18 Tomás Cohen Arazi 2020-11-03 13:45:23 UTC
(In reply to Jonathan Druart from comment #17)
> Tomas, can I get your stamp on this one please?

I've been tracking this dev closely with Kyle and he responded to my suggestions, on the API part of it. I will review it altogether now.
Comment 19 Tomás Cohen Arazi 2020-11-03 14:16:05 UTC
Created attachment 112923 [details] [review]
Bug 26633: Add REST API for managing transfer limits

Test Plan:
1) prove t/db_dependent/api/v1/transfer_limits.t

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2020-11-03 14:16:10 UTC
Created attachment 112924 [details] [review]
Bug 26633: Add advanced editor for transfer limits

The current transfer limits editor works great for Koha instances with
small numbers of branches. However, for consortiums with dozens or even
hundreds of libraries, the editor does not work well or effectively.

We should provide an "advanced" editor displays all to/from library
combinations in a grid and allows them to be edited in a manner somewhat
similar to the transport cost matrix editor.

Test Plan:
1) Apply this patch
2) Browse to the transfer limits editor
3) Click the new "Switch to advanced editor" link
4) Select a collection code/item type to edit limits for
5) Test the "Check all" function
6) Test the "Uncheck all" function
7) Test the "Check" column function
8) Test the "Uncheck" column function
9) Test the "Check" row function
10) Test the "Uncheck" row function
11) Test individual checkboxes/table cells

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2020-11-03 14:16:15 UTC
Created attachment 112925 [details] [review]
Bug 26633: Fix indirect object notation

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2020-11-03 14:16:21 UTC
Created attachment 112926 [details] [review]
Bug 26633: (QA follow-up) Change label to "collection" as we display the description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2020-11-03 14:16:26 UTC
Created attachment 112927 [details] [review]
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2020-11-03 14:16:30 UTC
Created attachment 112928 [details] [review]
Bug 26633: (QA follow-up) Tests on error condition

This patch highlights a problem with the try/catch conditions in the
ad() controlled method. Tests are added, and the controller is fixed to
reflect the right behaviour.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/transfer_limits.t
=> SUCCESS: Tests pass! 409 is returned as appropriate!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2020-11-03 14:16:35 UTC
Created attachment 112929 [details] [review]
Bug 26633: POD for the exceptions file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2020-11-03 14:18:30 UTC
(In reply to Jonathan Druart from comment #17)
> Tomas, can I get your stamp on this one please?

Yes, we need them, Kyle?
Comment 27 Jonathan Druart 2020-11-05 09:42:12 UTC
Created attachment 113053 [details] [review]
Bug 26633: Remove jquery.checkboxes.min.js include

No longer exists and not used in this template
Comment 28 Jonathan Druart 2020-11-05 10:04:10 UTC
1. Please provide documentation for the API

2. There is no error handling when the page is loaded. I first tried with D11 (which has the REST API broken) and the "loading..." was running forever, the table was loaded but without any checkboxes.
Maybe we should not display anything in that case.

3. I am a bit concern that there is no "save" or "undo" button. If you click "Check all" or "Uncheck all", you may have lost your work!
It's not blocker but worth noting it here.

4. There is no way to bookmark or share the URL to display a given table. That would have been useful. Is that something we could implement easily?

5.
(In reply to Kyle M Hall from comment #0)
> The current transfer limits editor works great for Koha instances with small
> numbers of branches. However, for consortiums with dozens or even hundreds
> of libraries, the editor does not work well or effectively.

I've added around 30 libraries and it creates a huge table. I am not sure we are answering the need as the table is not readable easily if you are starting to scroll. Maybe it would have been useful to have "floating th"'s, which are always displayed when you scroll.
Not blocker however, but feel free to submit if you think it would be useful.
Comment 29 Kyle M Hall 2020-11-06 14:03:06 UTC
(In reply to Jonathan Druart from comment #28)
> 1. Please provide documentation for the API

I'll see what I can do!

> 2. There is no error handling when the page is loaded. I first tried with
> D11 (which has the REST API broken) and the "loading..." was running
> forever, the table was loaded but without any checkboxes.
> Maybe we should not display anything in that case.

I'm not sure why we'd do this here, when we don't do it for any other ajax table in Koha.

> 4. There is no way to bookmark or share the URL to display a given table.
> That would have been useful. Is that something we could implement easily?

I'll see what I can do!
 
> 5.
> (In reply to Kyle M Hall from comment #0)
> > The current transfer limits editor works great for Koha instances with small
> > numbers of branches. However, for consortiums with dozens or even hundreds
> > of libraries, the editor does not work well or effectively.
> 
> I've added around 30 libraries and it creates a huge table. I am not sure we
> are answering the need as the table is not readable easily if you are
> starting to scroll. Maybe it would have been useful to have "floating th"'s,
> which are always displayed when you scroll.
> Not blocker however, but feel free to submit if you think it would be useful.

I really like that idea! I'll give it a shot but I might need some help from Lucas or Owen ;)
Comment 30 Jonathan Druart 2020-11-06 14:18:22 UTC
(In reply to Kyle M Hall from comment #29)
> (In reply to Jonathan Druart from comment #28)
> > 2. There is no error handling when the page is loaded. I first tried with
> > D11 (which has the REST API broken) and the "loading..." was running
> > forever, the table was loaded but without any checkboxes.
> > Maybe we should not display anything in that case.
> 
> I'm not sure why we'd do this here, when we don't do it for any other ajax
> table in Koha.

I think the other tables (with the checkout list in mind) will raise a DataTable alert box if the svc script/REST API route does not return something valid (or 500).
So we have a the interface telling us something went wrong.
Comment 31 Kyle M Hall 2020-11-06 18:32:17 UTC
Created attachment 113265 [details] [review]
Bug 26633: Add REST API for managing transfer limits

Test Plan:
1) prove t/db_dependent/api/v1/transfer_limits.t

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Kyle M Hall 2020-11-06 18:32:36 UTC
Created attachment 113266 [details] [review]
Bug 26633: Add advanced editor for transfer limits

The current transfer limits editor works great for Koha instances with
small numbers of branches. However, for consortiums with dozens or even
hundreds of libraries, the editor does not work well or effectively.

We should provide an "advanced" editor displays all to/from library
combinations in a grid and allows them to be edited in a manner somewhat
similar to the transport cost matrix editor.

Test Plan:
1) Apply this patch
2) Browse to the transfer limits editor
3) Click the new "Switch to advanced editor" link
4) Select a collection code/item type to edit limits for
5) Test the "Check all" function
6) Test the "Uncheck all" function
7) Test the "Check" column function
8) Test the "Uncheck" column function
9) Test the "Check" row function
10) Test the "Uncheck" row function
11) Test individual checkboxes/table cells

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Kyle M Hall 2020-11-06 18:32:40 UTC
Created attachment 113267 [details] [review]
Bug 26633: Fix indirect object notation

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Kyle M Hall 2020-11-06 18:32:43 UTC
Created attachment 113268 [details] [review]
Bug 26633: (QA follow-up) Change label to "collection" as we display the description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Kyle M Hall 2020-11-06 18:32:46 UTC
Created attachment 113269 [details] [review]
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Kyle M Hall 2020-11-06 18:32:50 UTC
Created attachment 113270 [details] [review]
Bug 26633: (QA follow-up) Tests on error condition

This patch highlights a problem with the try/catch conditions in the
ad() controlled method. Tests are added, and the controller is fixed to
reflect the right behaviour.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/transfer_limits.t
=> SUCCESS: Tests pass! 409 is returned as appropriate!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Kyle M Hall 2020-11-06 18:32:53 UTC
Created attachment 113271 [details] [review]
Bug 26633: POD for the exceptions file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 Kyle M Hall 2020-11-06 18:32:56 UTC
Created attachment 113272 [details] [review]
Bug 26633: Remove jquery.checkboxes.min.js include

No longer exists and not used in this template
Comment 39 Kyle M Hall 2020-11-06 18:32:59 UTC
Created attachment 113273 [details] [review]
Bug 26633: Convert transfer_limit.json to YAML
Comment 40 Kyle M Hall 2020-11-06 18:33:03 UTC
Created attachment 113274 [details] [review]
Bug 26633: Add API documentation
Comment 41 Kyle M Hall 2020-11-06 18:33:06 UTC
Created attachment 113275 [details] [review]
Bug 26633: Add ability to bookmark the page you are viewing
Comment 42 Kyle M Hall 2020-11-16 15:50:14 UTC
(In reply to Jonathan Druart from comment #30)
> (In reply to Kyle M Hall from comment #29)
> > (In reply to Jonathan Druart from comment #28)
> > > 2. There is no error handling when the page is loaded. I first tried with
> > > D11 (which has the REST API broken) and the "loading..." was running
> > > forever, the table was loaded but without any checkboxes.
> > > Maybe we should not display anything in that case.
> > 
> > I'm not sure why we'd do this here, when we don't do it for any other ajax
> > table in Koha.
> 
> I think the other tables (with the checkout list in mind) will raise a
> DataTable alert box if the svc script/REST API route does not return
> something valid (or 500).
> So we have a the interface telling us something went wrong.

Ah, I see. I'll see what I can do about that!
Comment 43 Kyle M Hall 2020-11-16 17:17:09 UTC
Created attachment 113676 [details] [review]
Bug 26633 (QA follow-up) Add error handling for loading transfer limits table
Comment 44 Kyle M Hall 2020-11-16 17:18:56 UTC
Created attachment 113677 [details] [review]
Bug 26633 (QA follow-up) Add error handling for loading transfer limits table
Comment 45 Kyle M Hall 2020-11-24 17:24:46 UTC
Created attachment 113961 [details] [review]
Bug 26633 - Add advanced editor link on admin home page
Comment 46 Katrin Fischer 2021-02-07 12:59:16 UTC
Hi Kyle,

I have a QA script fail here:

 FAIL	api/v1/swagger/paths/transfer_limits.json
   FAIL	  git manipulation
		The file has been added and deleted in the same patchset

 FAIL	api/v1/swagger/paths/transfer_limits.yaml
   OK	  git manipulation
   FAIL	  yaml_valid
		YAML Error: Invalid element in map


Can you please check?

Can you please also fix the last 2 commit messages subjects while you are on it? Bug xxxx: I would have done it, but need your help with the other fail here :)

	* Commit title does not start with 'Bug XXXXX: ' - a6713283b5
	* Commit title does not start with 'Bug XXXXX: ' - 20f1ef7e08
Comment 47 Katrin Fischer 2021-02-07 12:59:38 UTC
The first fail can be ignored (obv.)
Comment 48 Kyle M Hall 2021-02-09 19:06:55 UTC
Created attachment 116612 [details] [review]
Bug 26633: (QA follow-up) Add error handling for loading transfer limits table
Comment 49 Kyle M Hall 2021-02-09 19:07:08 UTC
Created attachment 116613 [details] [review]
Bug 26633: Add advanced editor link on admin home page
Comment 50 Kyle M Hall 2021-02-09 19:12:40 UTC
(In reply to Katrin Fischer from comment #46)
> Hi Kyle,
> 
> I have a QA script fail here:
> 
>  FAIL	api/v1/swagger/paths/transfer_limits.json
>    FAIL	  git manipulation
> 		The file has been added and deleted in the same patchset
> 
>  FAIL	api/v1/swagger/paths/transfer_limits.yaml
>    OK	  git manipulation
>    FAIL	  yaml_valid
> 		YAML Error: Invalid element in map
> 
> 
> Can you please check?
> 
> Can you please also fix the last 2 commit messages subjects while you are on
> it? Bug xxxx: I would have done it, but need your help with the other fail
> here :)
> 
> 	* Commit title does not start with 'Bug XXXXX: ' - a6713283b5
> 	* Commit title does not start with 'Bug XXXXX: ' - 20f1ef7e08

I've checked and the YAML is valid, so I think those are false flags from the QA script!
Comment 51 Jonathan Druart 2021-02-10 06:44:05 UTC
How did you check, Kyle?

The script is only returning the error from YAML::LoadFile

YAML Error: Can't parse double quoted string
   Code: YAML_PARSE_ERR_BAD_DOUBLE
   Line: 149
   Document: 1
 at /usr/share/perl5/YAML/Loader.pm line 574.
Comment 52 Kyle M Hall 2021-02-10 11:30:03 UTC
(In reply to Jonathan Druart from comment #51)
> How did you check, Kyle?
> 
> The script is only returning the error from YAML::LoadFile
> 
> YAML Error: Can't parse double quoted string
>    Code: YAML_PARSE_ERR_BAD_DOUBLE
>    Line: 149
>    Document: 1
>  at /usr/share/perl5/YAML/Loader.pm line 574.

I used yamllint. Looks like I'll have to do some more digging!
Comment 53 Kyle M Hall 2021-02-10 12:20:57 UTC
(In reply to Kyle M Hall from comment #52)
> (In reply to Jonathan Druart from comment #51)
> > How did you check, Kyle?
> > 
> > The script is only returning the error from YAML::LoadFile
> > 
> > YAML Error: Can't parse double quoted string
> >    Code: YAML_PARSE_ERR_BAD_DOUBLE
> >    Line: 149
> >    Document: 1
> >  at /usr/share/perl5/YAML/Loader.pm line 574.
> 
> I used yamllint. Looks like I'll have to do some more digging!

So the problem is the YAML module is insufficient for running this test. I would recommend we switch to YAML::XS. The current YAML module is being deprecated anyway and is being move to YAML::Old.

root@kohadevbox:koha(bug26633-qa)$ perl -e "use YAML qw(LoadFile); LoadFile('api/v1/swagger/paths/transfer_limits.yaml');"
YAML Error: Invalid element in map
   Code: YAML_LOAD_ERR_BAD_MAP_ELEMENT
   Line: 8
   Document: 1
 at /usr/share/perl5/YAML/Loader.pm line 350.
root@kohadevbox:koha(bug26633-qa)$ perl -e "use YAML::XS qw(LoadFile); LoadFile('api/v1/swagger/paths/transfer_limits.yaml');"
root@kohadevbox:koha(bug26633-qa)$ 

Basically, YAML only supports YAML 1.0, where YAML::XS ( and Mojolicious::Plugin::OpenAPI ) support YAML 1.1.

I would recommend as a first step that QohA::File::YAML be updated to use YAML::XS as a first step.
Comment 54 Kyle M Hall 2021-02-10 12:29:43 UTC
I'm not sure where we handle change requests to the qa test tools, so I've created a pull request on GitHub: https://github.com/Koha-Community/qa-test-tools/pull/2
Comment 55 Jonathan Druart 2021-02-10 13:35:59 UTC
(In reply to Kyle M Hall from comment #54)
> I'm not sure where we handle change requests to the qa test tools, so I've
> created a pull request on GitHub:
> https://github.com/Koha-Community/qa-test-tools/pull/2

It's https://gitlab.com/koha-community/qa-test-tools

IMO we should first switch to YAML::XS in Koha code before adding it to the QA script.
Comment 56 Katrin Fischer 2021-02-15 21:07:58 UTC
Reran the QA tools with the dependency applied, but still get:

 FAIL	api/v1/swagger/paths/transfer_limits.yaml
   OK	  git manipulation
   FAIL	  yaml_valid
		YAML Error: Invalid element in map


Do I need to install something new or do we need an update to the QA test tools?
Comment 57 Jonathan Druart 2021-02-16 07:42:25 UTC
The changes from this merge request must be applied to the qa-test-tools repo:
https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/35
Comment 58 Katrin Fischer 2021-02-17 19:03:47 UTC
Created attachment 116952 [details] [review]
Bug 26633: Add REST API for managing transfer limits

Test Plan:
1) prove t/db_dependent/api/v1/transfer_limits.t

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 59 Katrin Fischer 2021-02-17 19:03:52 UTC
Created attachment 116953 [details] [review]
Bug 26633: Add advanced editor for transfer limits

The current transfer limits editor works great for Koha instances with
small numbers of branches. However, for consortiums with dozens or even
hundreds of libraries, the editor does not work well or effectively.

We should provide an "advanced" editor displays all to/from library
combinations in a grid and allows them to be edited in a manner somewhat
similar to the transport cost matrix editor.

Test Plan:
1) Apply this patch
2) Browse to the transfer limits editor
3) Click the new "Switch to advanced editor" link
4) Select a collection code/item type to edit limits for
5) Test the "Check all" function
6) Test the "Uncheck all" function
7) Test the "Check" column function
8) Test the "Uncheck" column function
9) Test the "Check" row function
10) Test the "Uncheck" row function
11) Test individual checkboxes/table cells

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 60 Katrin Fischer 2021-02-17 19:03:56 UTC
Created attachment 116954 [details] [review]
Bug 26633: Fix indirect object notation

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 61 Katrin Fischer 2021-02-17 19:04:00 UTC
Created attachment 116955 [details] [review]
Bug 26633: (QA follow-up) Change label to "collection" as we display the description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 62 Katrin Fischer 2021-02-17 19:04:10 UTC
Created attachment 116956 [details] [review]
Bug 26633: (QA follow-up) Action buttons should not remain grayed out if the same action is used multiple times in a row

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 63 Katrin Fischer 2021-02-17 19:04:15 UTC
Created attachment 116957 [details] [review]
Bug 26633: (QA follow-up) Tests on error condition

This patch highlights a problem with the try/catch conditions in the
ad() controlled method. Tests are added, and the controller is fixed to
reflect the right behaviour.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/transfer_limits.t
=> SUCCESS: Tests pass! 409 is returned as appropriate!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 64 Katrin Fischer 2021-02-17 19:04:19 UTC
Created attachment 116958 [details] [review]
Bug 26633: POD for the exceptions file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 65 Katrin Fischer 2021-02-17 19:04:23 UTC
Created attachment 116959 [details] [review]
Bug 26633: Remove jquery.checkboxes.min.js include

No longer exists and not used in this template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 66 Katrin Fischer 2021-02-17 19:04:28 UTC
Created attachment 116960 [details] [review]
Bug 26633: Convert transfer_limit.json to YAML

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 67 Katrin Fischer 2021-02-17 19:04:32 UTC
Created attachment 116961 [details] [review]
Bug 26633: Add API documentation

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 68 Katrin Fischer 2021-02-17 19:04:37 UTC
Created attachment 116962 [details] [review]
Bug 26633: Add ability to bookmark the page you are viewing

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 69 Katrin Fischer 2021-02-17 19:04:42 UTC
Created attachment 116963 [details] [review]
Bug 26633: (QA follow-up) Add error handling for loading transfer limits table

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 70 Katrin Fischer 2021-02-17 19:04:46 UTC
Created attachment 116964 [details] [review]
Bug 26633: Add advanced editor link on admin home page

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 71 Jonathan Druart 2021-03-16 15:10:46 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 72 Fridolin Somers 2021-03-19 15:56:03 UTC
Enhancement not pushed to 20.11.x