Looking at bug history, I am asking for another signoff please.

I'm asking the customer whose sponsored this work to SO. We got a bit ahead of ourselves here.

Apply patches, on ktd, preparation:
- Run dbic
- Run reset_all
- Enable ILLModule sys pref
- At least one ILL batch status needs to exist, admin -> (bottom right) Interlibrary Loan batch statuses, add that (name:NEW status:NEW)
- At least one ILL Backend with "provide_batch_requests" capability needs to exist, add ReprintsDesk backend from https://github.com/PTFS-Europe/koha-ill-reprintsdesk to kohadevbox/koha/Koha/Illbackends with name 'ReprintsDesk'
- Update backend_directory in koha-conf.xml to <backend_directory>/kohadevbox/koha/Koha/Illbackends</backend_directory>
- At least one metadata enrichment plugin needs to exist, admin -> Manage plugins add that (https://github.com/PTFS-Europe/koha-plugin-api-pubmed/releases/tag/1.1.0)
- restart_all
- Run yarn build to update CSS

Test plan:
- Go to ILL Requests admin area, click "New ILL batch request" and select ReprintsDesk
- Create new batch (insert name, cardnumber and library) and press "continue"
- Add PubMed ID, one per line (example: 23668524\n29710265) and press "Process identifiers"
- After processing is complete, click "add items to batch" and then "finish and view batch"

Verify that:
- The items requested in the batch are now illrequests in Koha, and are part of the batch that was just created
- Clicking on "Batch requests" lists all existing batches
- From the previous step, clicking on a batch name lists only the requests from that batch
- If a batch is deleted, whatever requests that were previously on that batch are not deleted (their batch_id is now NULL)
- When editing a request that is part of a batch, it's possible to change which batch the request is part of, or none.

(Optional) Test logging:
- Before doing the above, enable illlog sys pref
- Do some CRUD actions on batch requests
- Check log viewer -> module "Interlibrary loans"
- Verify that the actions performed have been logged

(Optional) Test error messaging:
- Try creating a batch with an already existing batch name, verify it doesn't create and error message shows appropriately
- Try creating a batch with a non-existent borrower cardnumber, verifiy it doesn't create the batch and error message shows appropriately

Run tests and ensure all are passing:
prove -v t/db_dependent/Illbatches.t
prove -v t/db_dependent/api/v1/illbatches.t
prove -v t/db_dependent/IllbatchStatuses.t
prove -v t/db_dependent/api/v1/illbatchstatuses.t

Created attachment 151303 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151304 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151305 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151306 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151307 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Created attachment 151308 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151309 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151310 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151311 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151312 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Created attachment 151513 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151514 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151515 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151516 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151517 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Created attachment 151519 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151520 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151521 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151522 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Created attachment 151523 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Created attachment 151571 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151572 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151573 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151574 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151575 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151611 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151612 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151613 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151614 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151615 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

rebased after bug 33762

Created attachment 151805 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151806 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151807 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151808 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 151809 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

rebase for bug 33786

The dependency to bug 33716 is a bit of an issue here, we need to see how to resolve that. As I signed it off I can't QA, but it will send this one to BLOCKED even if passing.

I skipped the last patch (DBIC) and ran dbic manually for testing as there was a conflict.

1) QA script

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/ill-batch-modal.inc
   FAIL	  valid_template
		options_for_libraries: not found

 FAIL	installer/data/mysql/kohastructure.sql
   FAIL	  boolean_vs_tinyint
		WARNING - The new column (is_system) for table illbatch_statuses is using INT(1) as type, must be TINYINT(1) if it has a boolean purpose, see the SQL12 coding guideline


 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
   FAIL	  filters
		wrong_html_filter at line 669 (                                    <a href="/cgi-bin/koha/ill/ill-requests.pl?batch_id=[% request.batch.id | html %]">)

2a) Database update is not idempotent

Easy fix with our existing check methods:

DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_30719_add_ill_batches.pl  [11:39:04]: Bug 30719 - Add ILL batches
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate column name 'batch_id' at /kohadevbox/koha/C4/Installer.pm line 741

2b) Database update and kohastructure are not using COMMENT

We standardized on using COMMENT as they display in schema.koha-community.org etc. Could you please move the comments to COMMENT syntax?

3) Unit tests are failing (after running db Update and dbic)

Test Summary Report
-------------------
t/db_dependent/api/v1/ill_requests.t    (Wstat: 512 Tests: 2 Failed: 2)
  Failed tests:  1-2
  Non-zero exit status: 2
Files=5, Tests=33, 21 wallclock secs ( 0.03 usr  0.02 sys + 18.99 cusr  1.70 csys = 20.74 CPU)
Result: FAIL

4) Perltidy

It might be nice (not blocker) to run the new perltidy over the code when working on it, thinking especially of the new files. There are some small inconsistencies like spaces and such:

+use JSON qw( to_json );
+use base qw(Koha::Object);

5) FIXMEs

Koha/REST/V1/Illbatches.pm

+    #FIXME: This should be $c->objects-search
+    my @batches = Koha::Illbatches->search()->as_list;
+
+    #FIXME: Below should be coming from $c->objects accessors
+    # Get all patrons associated with all our batches

+# FIXME: This should be moved to Koha::Illbackend
+sub can_batch {

+            // TODO: need to also reset progress bar and already processed identifiers

+    // FIXME: This should be a kohaTable not KohaTable


Are these something we should change now or at least have a follow-up bug for filed?

6) API

If I understand correctly the API is not using the common/standardized terminology:

+  id:
+    type: string
+    description: Internal ILL batch identifier
Should be batch_id or maybe ill_batch_id? (compared with other API routes: hold_id, patron_id, etc.)

+  borrowernumber:
+    type: string
+    description: Borrower number of the patron of the ILL batch
Should be: patron_id

+  branchcode:
+    type: string
+    description: Branch code of the branch of the ILL batch
Should be library_id

+  branch:
+    type:
+      - object
+      - "null"
+    description: The branch associated with the batch
Should possibly be library

+  statuscode:
+    type: string
+    description: Code of the status of the ILL batch
Should probably be status_code

Similar for illbatchstatus.yaml:

id:
ill_batch_status_id ?


7a) Illbatch_statuses are not translatable

They should probably be moved to en, like we just did with the debit types and credit types too. For reporting it's nice when the description can be translated.

+++ b/installer/data/mysql/mandatory/illbatch_statuses.sql
@@ -0,0 +1,5 @@
+INSERT INTO illbatch_statuses ( name, code, is_system ) VALUES
+('New', 'NEW', 1),
+('In progress', 'IN_PROGRESS', 1),
+('Completed', 'COMPLETED', 1),
+('Unknown', 'UNKNOWN', 1);

7b) Strings can be transalted inside the .js files

We don't really need:
ill-batch-modal-strings.inc
ill-batch-table-strings.inc


This file is probably no longer needed as we can now translate strings in .js files. Not blocker, could still be moved later. It might simplify the code in koha-tmpl/intranet-tmpl/prog/js/ill-batch-modal.js quite a bit.

Also:
+    var ill_batch_item_remove = _("Are you sure you want to remove this item from the batch");

Missing question mark at the end?

7c) Avoid "building" sentences from multiple parts

+                <a href="#" aria-current="page">
+                    [% IF status.id %]
+                        Modify
+                    [% ELSE %]
+                        New
+                    [% END %] batch status
+                </a>


This will end up to be something like: %sModify%New%batch status. This is hard to read, but also harder to translate. Imaging having a language where you need to change the sequence, to be "Batch status edit". Better:

+                <a href="#" aria-current="page">
+                    [% IF status.id %]
+                        <span>Modify batch status</span>
+                    [% ELSE %]
+                        <span>New batch status</span>
+                    [% END %] 
+                </a>

The span would be to make both separate strings. The parsing looks for HTML tags. repeating the <a> inside the IF-ELSE would also work.

Also here:

+                    <h1>
+                        View ILL requests
+                        [% IF batch %]
+                        for batch "[% batch.name | html %]"
+                        [% END %]
+                    </h1>



7d) Not sure if this is translatable

I have doubts about this one here:

    <strong>[% status.is_system ? "Yes" : "No" | html %]</strong>

I think we need to verify or just move to an IF-ELSE construct.

8) Capitalization

+                <li><a href="/cgi-bin/koha/admin/ill_batch_statuses.pl">Interlibrary Loan batch statuses</a></li>

Interlibrary loan...

+  <th scope="col">Request Status</th>
Request status

<dt><a href="/cgi-bin/koha/admin/ill_batch_statuses.pl">Interlibrary Loan batch statuses</a></dt>

+    Interlibrary Loan batch statuses &rsaquo; Administration &rsaquo; Koha
+    <a href="/cgi-bin/koha/admin/ill_batch_statuses.pl">Interlibrary Loan batch statuses</a>

Maybe to a search replace for Interlibrary Loan? There are many more...


9) Terminology

+                <th scope="col">Branch</th>
Should be Library.


10) Page titles and breadcrumbs

Need updating to follow Owen's latest work using Template WRAPPER etc.

koha-tmpl/intranet-tmpl/prog/en/modules/admin/ill_batch_statuses.tt


11) Using page-section and btn-primary

This predates the staff interface redesign work and needs some touch ups for yellow main buttons and white backgrounds.

12) Null?

This looks like it might display "null" when either one is empty (unconfirmed):

+        var displayText = [data.firstname, data.surname].join(' ') + ' ( ' + data.cardnumber + ' )';

Katrin, thank you for your amazing QA.

I have 18 patches that address your observations here:
https://github.com/PTFS-Europe/koha/commits/new_30719

Please have another look and then let me know how to proceed, if there's more fixing required, should I submit all these patches here or provide a squashed version?

I did not address 11) directly because I'm not sure where exactly to apply it.

Thank you!

Sorry, but the database update doesn't work:

DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_30719_add_ill_batches.pl  [12:29:56]: Bug 30719 - Add ILL batches
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Can't create table `koha_kohadev`.`illbatches` (errno: 150 "Foreign key constraint is incorrectly formed") at /kohadevbox/koha/C4/Installer.pm line 741

I used the remote branch and did a rebase on master.

After a chat with Katrin, these are the QA points we're still missing:
- add mandatory batch statuses on the atomicupdate
- double-check description field for batch statuses
- add page section on the batch statuses list
- style "Save" button on batch status edit page
- "New ILL batch request" should be "New ill requests batch"
- Are you sure you want to remove this item from the batch should be "Are you sure you want to remove this request from the batch?"
- add requests modal - check for noExport class for the "remove" column
- batches list table - check for noExport class for the actions column
- add page-section to the ill batches list table
- perltidy

I hope to be able to finish this in the near future and comeback when that's done.

Thx for the summary Pedro, I believe we are close!

Created attachment 154046 [details] [review]
Bug 30719: (QA follow-up squash):

This is a squash of 25 QA patches located at:
    https://github.com/PTFS-Europe/koha/commits/new_30719

Bug 30719: (QA follow-up) Batch column should be hidden by default
Bug 30719: (QA follow-up) Fix wrong tt filter type
Bug 30719: (QA follow-up) Make atomicupdate idempotent
Bug 30719: (QA follow-up) Use COMMENT syntax in database files
Bug 30719: (QA follow-up) Fix tiny boolean is_system
Bug 30719: (QA follow-up) Add missing CONSTRAINT entries from kohastructure.sql to the atomicupdate file
Bug 30719: (QA follow-up) Add missing koha_object_class and
koha_objects_class methods
Bug 30719: (QA follow-up) Swap search to find
Bug 30719: (QA follow-up) Fix tests
Bug 30719: (QA follow-up) API terminology - id -> batch_id
Bug 30719: (QA follow-up) API terminology - borrowernumber -> patron_id
Bug 30719: (QA follow-up) API terminology - branchcode -> library_id
Bug 30719: (QA follow-up) Make mandatory illbatch_statuses translatable
Bug 30719: (QA follow-up) Improve translatability
Bug 30719: (QA follow-up) Fix capitalization of Interlibrary Loan
Bug 30719: (QA follow-up) Change Branch to Library in ILL batches table
Bug 30719: (QA follow-up) Add template WRAPPER to batch statuses breadrcrumbs
Bug 30719: (QA follow-up) Utilize patron_to_html function to display patron info in batches table
Bug 30719: (QA follow-up) Add mandatory batch statuses to the atomicupdate
Bug 30719: (QA follow-up) Add page-section to the batch statuses list page
Bug 30719: (QA follow-up) Style Save button on batch status edit page
Bug 30719: (QA follow-up) Add question mark to label string, rephrase new ILL batch button
Bug 30719: (QA follow-up) Add noExport class to action columns in batch list table and batch modal table
Bug 30719: (QA follow-up) Add page-section and headers to ILL batch table
Bug 30719: (QA follow-up) Perltidy

Created attachment 154047 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 154048 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 154049 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 154050 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 154051 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 154052 [details] [review]
Bug 30719: (QA follow-up squash):

    This is a squash of 25 QA patches located at:
    https://github.com/PTFS-Europe/koha/commits/new_30719

Bug 30719: (QA follow-up) Batch column should be hidden by default
Bug 30719: (QA follow-up) Fix wrong tt filter type
Bug 30719: (QA follow-up) Make atomicupdate idempotent
Bug 30719: (QA follow-up) Use COMMENT syntax in database files
Bug 30719: (QA follow-up) Fix tiny boolean is_system
Bug 30719: (QA follow-up) Add missing CONSTRAINT entries from kohastructure.sql to the atomicupdate file
Bug 30719: (QA follow-up) Add missing koha_object_class and
koha_objects_class methods
Bug 30719: (QA follow-up) Swap search to find
Bug 30719: (QA follow-up) Fix tests
Bug 30719: (QA follow-up) API terminology - id -> batch_id
Bug 30719: (QA follow-up) API terminology - borrowernumber -> patron_id
Bug 30719: (QA follow-up) API terminology - branchcode -> library_id
Bug 30719: (QA follow-up) Make mandatory illbatch_statuses translatable
Bug 30719: (QA follow-up) Improve translatability
Bug 30719: (QA follow-up) Fix capitalization of Interlibrary Loan
Bug 30719: (QA follow-up) Change Branch to Library in ILL batches table
Bug 30719: (QA follow-up) Add template WRAPPER to batch statuses breadrcrumbs
Bug 30719: (QA follow-up) Utilize patron_to_html function to display patron info in batches table
Bug 30719: (QA follow-up) Add mandatory batch statuses to the atomicupdate
Bug 30719: (QA follow-up) Add page-section to the batch statuses list page
Bug 30719: (QA follow-up) Style Save button on batch status edit page
Bug 30719: (QA follow-up) Add question mark to label string, rephrase new ILL batch button
Bug 30719: (QA follow-up) Add noExport class to action columns in batch list table and batch modal table
Bug 30719: (QA follow-up) Add page-section and headers to ILL batch table
Bug 30719: (QA follow-up) Perltidy

Rebased on top of master and fixed the DBIC conflict.
Katrin, all yours when you're available!

Thank you very much!

Created attachment 155421 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 155422 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 155423 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 155424 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 155425 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>

Created attachment 155426 [details] [review]
Bug 30719: (QA follow-up) Squash:

    This is a squash of 25 QA patches located at:
    https://github.com/PTFS-Europe/koha/commits/new_30719

Bug 30719: (QA follow-up) Batch column should be hidden by default
Bug 30719: (QA follow-up) Fix wrong tt filter type
Bug 30719: (QA follow-up) Make atomicupdate idempotent
Bug 30719: (QA follow-up) Use COMMENT syntax in database files
Bug 30719: (QA follow-up) Fix tiny boolean is_system
Bug 30719: (QA follow-up) Add missing CONSTRAINT entries from kohastructure.sql to the atomicupdate file
Bug 30719: (QA follow-up) Add missing koha_object_class and
koha_objects_class methods
Bug 30719: (QA follow-up) Swap search to find
Bug 30719: (QA follow-up) Fix tests
Bug 30719: (QA follow-up) API terminology - id -> batch_id
Bug 30719: (QA follow-up) API terminology - borrowernumber -> patron_id
Bug 30719: (QA follow-up) API terminology - branchcode -> library_id
Bug 30719: (QA follow-up) Make mandatory illbatch_statuses translatable
Bug 30719: (QA follow-up) Improve translatability
Bug 30719: (QA follow-up) Fix capitalization of Interlibrary Loan
Bug 30719: (QA follow-up) Change Branch to Library in ILL batches table
Bug 30719: (QA follow-up) Add template WRAPPER to batch statuses breadrcrumbs
Bug 30719: (QA follow-up) Utilize patron_to_html function to display patron info in batches table
Bug 30719: (QA follow-up) Add mandatory batch statuses to the atomicupdate
Bug 30719: (QA follow-up) Add page-section to the batch statuses list page
Bug 30719: (QA follow-up) Style Save button on batch status edit page
Bug 30719: (QA follow-up) Add question mark to label string, rephrase new ILL batch button
Bug 30719: (QA follow-up) Add noExport class to action columns in batch list table and batch modal table
Bug 30719: (QA follow-up) Add page-section and headers to ILL batch table
Bug 30719: (QA follow-up) Perltidy

Created attachment 155427 [details] [review]
Bug 30719: (QA follow-up) c->validation fixes

Created attachment 155428 [details] [review]
Bug 30719: DB and API

- Adds 'batch' accessor to Illrequest object
- New illbatches and illbatch_statuses tables
- New foreign key 'batch_id' in illrequests table
- Atomic update file
- Default illbatch_statuses
- Add 'add' ill_requests api method
- Add POST method in ill_requests path
- Add 'batch_id property to ill_request api definition
- Updated swagger.yml with new batches and batchstatuses endpoints

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155429 [details] [review]
Bug 30719: ILL Batches

- Add batch column to requests table
- Establish if there are any availability or metadata enrichment plugins and pass that to the template
- Verify if we have any backend that can support batches, if not, don't show the option
- Updates to the ILL toolbar
- New ILL batch modal
- New Koha classes
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155430 [details] [review]
Bug 30719: ILL Batch Statuses

- UI adding support for batch statuses in batch UI
- Admin UI for managing batch statuses
- API specs

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155431 [details] [review]
Bug 30719: Tests

Co-authored-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155432 [details] [review]
Bug 30719: dbic specific, DONT PUSH

Signed-off-by: Edith Speller <Edith.Speller@ukhsa.gov.uk>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155433 [details] [review]
Bug 30719: (QA follow-up) Squash:

    This is a squash of 25 QA patches located at:
    https://github.com/PTFS-Europe/koha/commits/new_30719

Bug 30719: (QA follow-up) Batch column should be hidden by default
Bug 30719: (QA follow-up) Fix wrong tt filter type
Bug 30719: (QA follow-up) Make atomicupdate idempotent
Bug 30719: (QA follow-up) Use COMMENT syntax in database files
Bug 30719: (QA follow-up) Fix tiny boolean is_system
Bug 30719: (QA follow-up) Add missing CONSTRAINT entries from kohastructure.sql to the atomicupdate file
Bug 30719: (QA follow-up) Add missing koha_object_class and
koha_objects_class methods
Bug 30719: (QA follow-up) Swap search to find
Bug 30719: (QA follow-up) Fix tests
Bug 30719: (QA follow-up) API terminology - id -> batch_id
Bug 30719: (QA follow-up) API terminology - borrowernumber -> patron_id
Bug 30719: (QA follow-up) API terminology - branchcode -> library_id
Bug 30719: (QA follow-up) Make mandatory illbatch_statuses translatable
Bug 30719: (QA follow-up) Improve translatability
Bug 30719: (QA follow-up) Fix capitalization of Interlibrary Loan
Bug 30719: (QA follow-up) Change Branch to Library in ILL batches table
Bug 30719: (QA follow-up) Add template WRAPPER to batch statuses breadrcrumbs
Bug 30719: (QA follow-up) Utilize patron_to_html function to display patron info in batches table
Bug 30719: (QA follow-up) Add mandatory batch statuses to the atomicupdate
Bug 30719: (QA follow-up) Add page-section to the batch statuses list page
Bug 30719: (QA follow-up) Style Save button on batch status edit page
Bug 30719: (QA follow-up) Add question mark to label string, rephrase new ILL batch button
Bug 30719: (QA follow-up) Add noExport class to action columns in batch list table and batch modal table
Bug 30719: (QA follow-up) Add page-section and headers to ILL batch table
Bug 30719: (QA follow-up) Perltidy

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155434 [details] [review]
Bug 30719: (QA follow-up) c->validation fixes

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Created attachment 155435 [details] [review]
Bug 30719: (QA follow-up) Fix "Request Status" capitalization

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Tortured Pedro long enough on this one ;) 

It's hard to take over someone else's code and I think we made a lot of improvements here trying to bring it closer to current standard. 

QA tools fail on tidiness (and a false positive for options_for_libraries), but are otherwise ok now. Given the size of this patch set and the age, I am willing to let that slip here. All new fiels have been perltidied.

Created attachment 156556 [details] [review]
Bug 30719: Atomic update fixes

Hi all. I know the history of this bug and am willing to overlook some things, but as-is I believe this requires some more work.

High

* The GET /illbatches endpoint is not /ill/batchess?
* Illbatches.pm is too old school. Entirely relying on fetching the whole resultset:
    #FIXME: This should be $c->objects-search
    my @batches = Koha::Illbatches->search()->as_list;
* And crafting the response manually, even the 'patron' attribute is done manually instead of an embed
* The request body needs to be manually translated into the DB model:
    my $batch = Koha::Illbatch->new($body);
 we have Koha::Illbatches->new_from_api to avoid that. But there's obviously a 
 reason it wasn't done like that.

Lower

* Table names, and column names: this is using old school naming for things. We can solve it on the API front, but even that is not entirely done.

Please let me know if there's anything I can do to help you with this.

Created attachment 156748 [details] [review]
Bug 30719: QA follow-up: Rename illbatches endpoint to ill/batches

Created attachment 156749 [details] [review]
Bug 30719: QA follow-up: Rewrite Illbatches list endpoint

Update accessors
Add +strings embed
Add x-koha-embed to batches list andpoint
Add embed to API call from the front-end
Update table to get data from _strings
Add x-koha-embed to tests
Add strings_map to Illbatch
Add to_api_mapping to Illbatch

(In reply to Tomás Cohen Arazi from comment #126)
> Hi all. I know the history of this bug and am willing to overlook some
> things, but as-is I believe this requires some more work.
> 
> High
> 
> * The GET /illbatches endpoint is not /ill/batchess?
> * Illbatches.pm is too old school. Entirely relying on fetching the whole
> resultset:
>     #FIXME: This should be $c->objects-search
>     my @batches = Koha::Illbatches->search()->as_list;
> * And crafting the response manually, even the 'patron' attribute is done
> manually instead of an embed
> * The request body needs to be manually translated into the DB model:
>     my $batch = Koha::Illbatch->new($body);
>  we have Koha::Illbatches->new_from_api to avoid that. But there's obviously
> a 
>  reason it wasn't done like that.
> 
> Lower
> 
> * Table names, and column names: this is using old school naming for things.
> We can solve it on the API front, but even that is not entirely done.
> 
> Please let me know if there's anything I can do to help you with this.

Hi Tomas, thanks for looking.
Can you please check my latest 2 patches and confirm that this is what's required?
More work will follow for the other endpoints and for illbatch statuses if so.

Created attachment 156752 [details] [review]
Bug 30719: QA follow-up: Rewrite ill batches get endpoint

Created attachment 156783 [details] [review]
Bug 30719: QA follow-up: Rewrite remaining ill batches endpoints

get, add and update rewrite

Created attachment 156785 [details] [review]
Bug 30719: QA follow-up: Rename illbatchstatuses

illbatchstatuses endpoint is now ill/batchstatuses

Created attachment 156811 [details] [review]
Bug 30719: (QA follow-up) Pick better column names and cleanup

This patch takes on normalizing the attribute names, embeds, and also
makes the whole API more kosher, in terms of using accessors for related
objects, using the standard structure for strings_map, etc.

Created attachment 156812 [details] [review]
Bug 30719: QA follow-up:

Update statuscode -> status_code on the js files
Update remaining batch_id -> ill_batch_id
Update batch object in Illrequest.pm strings_map

Created attachment 156827 [details] [review]
Bug 30719: QA follow-up: Updating remaining naming

Update statuscode -> status_code on the js files
Update remaining batch_id -> ill_batch_id
Update batch object in Illrequest.pm strings_map

Created attachment 156828 [details] [review]
Bug 30719: QA follow-up: DB updates

updates to atomicupdate and kohastructure.sql

Pushed to master for 23.11.

Nice work everyone, thanks!

Enhancement not pushed to 23.05.x